Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change battery_power to S16 #58

Merged
merged 1 commit into from
Jul 8, 2022
Merged

Change battery_power to S16 #58

merged 1 commit into from
Jul 8, 2022

Conversation

bjeanes
Copy link
Contributor

@bjeanes bjeanes commented Jul 8, 2022

The docs seem to be incorrect here. When the battery is charging the battery_power is negative.

See https://discord.com/channels/936031869001158666/936031869001158669/994762394511347802

The docs seem to be incorrect here. When the battery is _charging_ the `battery_power` is negative.

See https://discord.com/channels/936031869001158666/936031869001158669/994762394511347802
@bjeanes bjeanes changed the title Change battery_power so S16 Change battery_power to S16 Jul 8, 2022
@bohdan-s bohdan-s merged commit b3ae696 into bohdan-s:main Jul 8, 2022
@bjeanes bjeanes deleted the patch-1 branch July 8, 2022 00:35
@michbeck100
Copy link
Collaborator

This doesn't seem to be correct. My battery is charging at the moment but the value is still positive. I'm using 13001 (battery_charging) with some mapping (-1 for discharge and 1 for charge) and multiply with 13022 to get a negative value if discharging.

Maybe the value is negative when discharging? I will see this evening.

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

Hmm perhaps despite their documentation it is not consistent across models. What inverter do you have @michbeck100?

@michbeck100
Copy link
Collaborator

michbeck100 commented Jul 8, 2022

SH10RT. What about you?

I would expect at least the models SHxRT to have the same registers. Or I need a firmware update?

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

I am on SH5.0RS, which I suspect should be pretty similar as they are the same series, just three-phase vs single-phase, right?

@michbeck100
Copy link
Collaborator

Yes, right. At least the change doesn't break anything.

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

Screen Shot 2022-07-08 at 3 24 41 pm

image

These screenshots were taken 30s apart which could account for the measurement difference here. However, the value when an unsigned number is just shy of the max value, which is clearly not a realistic number (I'm not drawing ~65 kW of power from battery!)

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

Screen Shot 2022-07-08 at 3 29 14 pm

Here's the graph from home assistant before and after the fix. Clearly only the latter makes sense, but it's certainly possible that there is a bug across inverters here.

My battery is charging at the moment but the value is still positive.

Is it a reasonable positive value? If it's equivalent to my negative value then I'd say it's definitely a bug across inverter models or firmware versions 🙄

@michbeck100
Copy link
Collaborator

michbeck100 commented Jul 8, 2022

The values on my side make totally sense. But it seems that some of your values are at least questionable. Do you really have a battery state of health of 10 %? And battery_current seems to be a little high. Actually I would expect battery_voltage * battery_current = battery_power, but that would be 1.8 MW for your system 😳. And total_battery_charge_from_pv is just twice as daily_battery_charge_from_pv. That would mean your battery is 2 days old 🤔?
Maybe the reason for the negative values lies in your battery?

What type of battery are you using?

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

Do you really have a battery state of health of 10 %?

No, this is a separate issue. It's correct if I connect over modbus (but then grid frequency is ~500.0) but incorrect over http. It's a bug in their modbus implementation, as near as I can tell.

Actually I would expect battery_voltage * battery_current = battery_power, but that would be 1.8 MW for your system 😳

TBH, I actually think battery_current is also overflowing. Ignoring the formatting, the digits are 65397, which are a stones throw from the max value for 16-bit unsigned number. When I look at the WiNet-S web interface, the battery_current is negative when battery is charging

Screen Shot 2022-07-08 at 3 55 13 pm

I changed it to an S16 locally, and it looks correct (assuming the amperage is ~-4.86, but it truncated to -4.8):

Screen Shot 2022-07-08 at 3 58 40 pm

That would mean your battery is 2 days old 🤔?

That is not far off from true. It was certified and turned on only yesterday, installed about 2 weeks ago.

Maybe the reason for the negative values lies in your battery?

Yes, possibly this is battery-specific. It's hard to say without more knowledge about Sungrow's internal COMs architecture. Definitely seems plausible, though.

What type of battery are you using?

Sungrow's SBR line. 12.8 kWh in modules, so that'd make it an SBR128.

What about you?

@michbeck100
Copy link
Collaborator

What about you?

I use a BYD battery. The values seem to be completely correct.

When I look at the WiNet-S web interface, the battery_current is negative when battery is charging

Which seems to make sense for DC, since it's the opposite of discharging. And this would explain why the battery_power is negative as well.

My guess: these values come directly from the battery instead of being measured by the inverter. That's why we have a different leading sign.

It's correct if I connect over modbus (but then grid frequency is ~500.0) but incorrect over http.

What do you mean by "connect over modbus" and "incorrect over http". Are you referring to the different output channels of SunGather? These values don't differ as the accuracy is applied directly after reading the registers from modbus. So the values are the same for all outputs.

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

Are you referring to the different output channels of SunGather?

No, I am referring to the connection type

connection: sungrow # [Required] options: modbus, sungrow, http

The values returned by the WiNet-S modbus TCP vs the http workaround by @bohdan-s differ. I can't fathom why other than a bug in Sungrow firmware somewhere (either on the WiNet-S's modbus implementation, web UI, or elsewhere).

It's 100% reproducible. Connecting over modbus I get grid_frequency that floats around 500.0 (instead of 50.0) and battery health of 100%. If I connect to the inverter with http, I get grid_frequency correctly shown as 50.0 but the battery health is 10.0%. It's bonkers!

@michbeck100
Copy link
Collaborator

michbeck100 commented Jul 8, 2022

Ok, understand. I would suggest you use a custom version of SunGather and change the accuracy of grid_frequency to 0.01. And btw: the grid frequency is not always fixed at 50 Hz, it's normal that it floats around. Or you remove the accuracy of battery_state_of_healthy for http.

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

I would suggest you use a custom version of SunGather and change the accuracy of grid_frequency to 0.01

Yep I'm doing that. I'm only going into it since you seemed to be surprised by some values. I just wanted to show why I don't believe it is related to the battery_power issue.

And btw: the grid frequency is not always fixed at 50 Hz, it's normal that it floats around.

Of course. That doesn't concern me, only the order of magnitude difference did!

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

As to this specific issue: if the register data type is varying by battery that's pretty hard to handle robustly in this library. I'm not sure what the answer is here without more information.

@michbeck100
Copy link
Collaborator

I think it's better to use signed integers instead of unsigned. And then it's up to the user to work with these values. As long as data type stays stable for the used setup.

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

Then I imagine battery_current probably needs to change to S16 too...

@michbeck100
Copy link
Collaborator

obviously

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

@bohdan-s there are two battery_currents in the registers file. Both seem to apply to SH#K models, but I'm not sure if both should change to S16. WDYT?

@michbeck100
Copy link
Collaborator

michbeck100 commented Jul 8, 2022

The models for 13021 are wrong. According to the documentation:

image

This means, these models only apply to 13109

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 8, 2022

@michbek100 I'm not sure what your screenshot shows there. You are showing 13109 which is consistent with the registers file. Maybe I am misinterpreting the PDF, but 13021 doesn't list any models, which I take to mean "all hybrid inverters" (at least which are mentioned under "valid device types" header on page 2.

@michbeck100
Copy link
Collaborator

@bjeanes Probably you are right. That means that these inverters support the same value on register 13109 and 13021. But is there somebody to confirm that?

@bjeanes
Copy link
Contributor Author

bjeanes commented Jul 12, 2022

Interestingly, I just updated my inverter firmware to fix a different issue, and now battery_power is positive, but battery_current stays negative when charging:

Screen Shot 2022-07-12 at 11 11 25 am

This is only over modbus connection from SunGather. Under http, both are still negative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants