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

Battery SoH, BMS Cell Temp, BMS kWh remaining updates #760

Open
canton7 opened this issue Feb 2, 2025 · 17 comments
Open

Battery SoH, BMS Cell Temp, BMS kWh remaining updates #760

canton7 opened this issue Feb 2, 2025 · 17 comments

Comments

@canton7
Copy link
Collaborator

canton7 commented Feb 2, 2025

Following the chaos in #756, we need to make sure that we understand exactly which inverters + firmware versions support these registers, before we go and break people.

See also: #703 (adds Battery SoH to all), #704 (adds BMS registers to H3_SET).

My current best understanding is this:

Inverter Battery SoH BMS Cell Temp BMS kWh remaining
H1 G1 x All All
H1 G2 All? All All
H3 1.81 (not 1.56) 1.80 1.80
Kuara H3 x x x
KH 1.37 (not 1.21) All All

FYI @MartB, @WyndStryke

@canton7
Copy link
Collaborator Author

canton7 commented Feb 2, 2025

We should probably also get #680 in before we try to release this again.

@FozzieUK
Copy link
Contributor

FozzieUK commented Feb 2, 2025

On the H1 G2 the cell temps, kwh remaining, SoH (& batvolt/current) have been tested on Manager 1.44, but there are register read fails with 1.37 so best to assume that you need minimum 1.44 for these.

@jaal2001
Copy link

jaal2001 commented Feb 2, 2025

Can we please consider introducing a beta channel for this integration?
Have 2-3 updates per day, while the updates are not fully tested to a broader audience is not the best idea imho.
I assume there are willing participants for beta tests, so most people can stay on the stable release.

@canton7
Copy link
Collaborator Author

canton7 commented Feb 2, 2025

@jaal2001 We do have a beta release system. The problem was that we needed to push out a proper release quickly as HA had broken the integration.

I'd been assured that these changes had been tested on all inverters they applied to.

@jaal2001
Copy link

jaal2001 commented Feb 2, 2025

Okay, understood.

@R1200G
Copy link

R1200G commented Feb 2, 2025

H3-12.0-E

SOH, both battery entities and BMS kWh remaining work perfectly.

Firmware:
Master: 2.04
Manager: 1.81
BMS: 2.002

@canton7
Copy link
Collaborator Author

canton7 commented Feb 2, 2025

Thanks, updated the table. Looks like the H3 introduced all 3 somewhere around v1.80.

@maddin4711
Copy link

Yes, v1.80 also works.

After Rollback to 1.13.1 SOH, both battery entities and BMS kWh remaining work again

H3-12.0-E Firmware:
Master: 2.01
Slave: 1.03
Manager: 1.80
Battery Master: 1.014
Battery Slaves: 1.11

@MartB
Copy link
Contributor

MartB commented Feb 2, 2025

1.80 was the first version i tested these with for the H3.
1.84 is the current version and confirmed to be working too.


I would strongly advice everyone running outdated manager firmwares (e.g < 1.81 for H3) to contact the installers of these systems and ask for an update of the master and manager firmwares. In case they wont do it, contact FoxESS Support to get them updated. FoxESS heavily relies on these firmware updates to be applied.


Back on topic:
Supporting very old versions like 1.56 for H3 indefinitely may not be practical in the long run.
To limit future breaking changes if FoxESS decides to go hay-wire with the Modbus definitions we should probably

  • Follow up and integrate version checks based on Adds additional BMS entities for H1 G2 and KH inverters on latest firmware #735
  • Alternatively, enhance it with a feature flag system that allows toggling on certain (new) features instead of enabling them by default for a hardcoded list of versions. This could also go along with a compatibility check action that (automatically?) verifies which flags are supported if existing users want to check for support. Also comes in handy when collecting compatibility data on firmware versions from beta testers.
  • Clearly communicate a minimum and tested version for each update for the inverters used by devs and volunteers. (Along with any feature flags newly added and the versions they were confirmed with, if the system is implemented.)

This integration is already fragmented when it comes to the different inverters, introducing yet another variable with a lot of unknowns will only make it harder to maintain in the future as new updates and therefor new features become available over Modbus.

Ideally this urges users to update their inverters which benefits everyone and stops breaking old installations by default.

P.S. If additional update motivation is required for people reading this thread, FoxESS recently overhauled the battery charging algorithms for more battery safety.

@canton7
Copy link
Collaborator Author

canton7 commented Feb 2, 2025

I strongly believe that supporting users on older firmware versions is important. I wish we had stats on how many users we have, but the charge period card for the H1 has 1000 downloads, so it's at least a few thousand. If we drop a release which intentionally breaks an older firmware version, then that's a lot of new tickets (because people don't look for duplicates), and someone (i.e me) has to go through and close them all.

We also risk people just not upgrading if upgrading means breaking their HA, which means they'll sit in an older version until HA breaks us, at which point they're trapped.That's how you get forks - we've seen that happen already - and forks is how you fragment this ecosystem. I already have a lot of integrations that I'll won't dare update unless I have a free weekend to fix the inevitable breakage, and it's very annoying.

We also have no reason to drop support for old inverters. Their register maps are already known and programmed: frankly it's more effort to remove them than keep them. We have a system for adding new inverters and firmware version with minimal boilerplate. We have tests to make sure we don't accidentally break anything.

Manual firmware version selection is a good start, but ideally we want that to be automatic. It's slightly annoying to do, but probably worth it. We should also finish up #680 so that people don't get broken completely if we mess up (or they have the wrong firmware version selected), and we can prompt them to take the appropriate corrective action.

Where we messed up here was adding new registers to pre-existing inverters without enough testing. I know that the Kuara is more limited, and I know that the register map docs we have don't have these registers, so I should have been much more insistent that we were very clear on what exactly had been tested and what hadn't. I was in a rush and felt bad that the PRs had been sitting around for months, so I trusted them too much. Then spent hours this weekend which I didn't have fire-fighting, which was very much a false economy!

It would also be good to have a register on the wiki of volunteers with various inverters + firmware versions that we can call on to test things.

@MartB
Copy link
Contributor

MartB commented Feb 2, 2025

I appreciate the time you spend on this project and I completely understand the frustration of dealing with a flood of new tickets after a release, and avoiding that is definitely a must.

My PRs clearly showed that even if things are out for months and tested by select volunteers they can still catastrophically impact the existing user base. I even asked people on photovoltaikforum.com to test my changes with simplified instructions and got one reply and confirmation for H3 Manager 1.80 in 11/2024, but thats still not enough engagement.

An additional insight we got from this release is that people willing to test / frequenting this repo seem to have recent versions installed and we will fail to find these incompatibilities for every new register set that doesnt have a proper "added by Fox in version xyz". So yes, as you said, it is probably wise to avoid backporting features from now on unless explicitly tested. That one is on me for not taking into account that these undiscovered registers might not have existed in prior modbus protocol revisions.

I agree that getting #680 to work is def. the easiest way to avoid these failures in the future and turn releases less into scream tests and more into "hey i got a strange error/warning in my log". Maybe we can work on whats missing over there to push it over the finish line?

I also really like the idea of a volunteer register with inverter + firmware versions. That could help avoid issues like the Kuara register mess, ensuring we have proper testing coverage before merging changes.

Formalizing that in the wiki or setting up a low entry barrier solution for coordinated testing (Discord/Slack/...) sounds helpful indeed.

@canton7
Copy link
Collaborator Author

canton7 commented Feb 3, 2025

So yes, as you said, it is probably wise to avoid backporting features from now on unless explicitly tested

The approach has always been to only make changes to a particular inverter model + manager version if we have tested on that model + manager version. FoxESS are too inconsistent for us to be able to do anything else. The fact that your PRs made changes to inverter models which weren't tested is due to a failure of the existing policy: it's not an indication that we need a new policy. And that's on me for not being critical enough.

Granted, we learned that the H3 introduced new registers in a particular firmware version, which we might not have caught otherwise (I'd have to go back through the H3 protocol docs I have), but we should have avoided breaking the Kuara or old KH.

Maybe we can work on whats missing over there to push it over the finish line?

I was thinking about that this morning, and I think the approach I was taking won't work, as it only tracks whether a register we're intending to poll is actually unavailable, and not whether a register we're spanning as part of a larger read isn't available (which does happen). I've added more notes to #680, feel free to have a crack at it!

Formalizing that in the wiki or setting up a low entry barrier solution for coordinated testing (Discord/Slack/...) sounds helpful indeed.

I'd like to avoid fragmenting the community over yet more communication channels (we're already split across Facebook and here). I think a wiki page is fine.

@WyndStryke
Copy link

WyndStryke commented Feb 4, 2025

Tried 1.13.4 (KH on Version: Manager 1.37, Version: Master 1.41, Version: Slave 1.01)

Battery SoH

This entity is no longer being provided by the foxess_modbus integration. If the entity is no longer in use, delete it in settings.

Everything works other than Battery SoH, which is showing as unavailable.

I'm not actually using SoH in any of my automations or monitoring, but it would be nice to know it.

Regarding the 'Supported Features' page on the wiki. This does not currently break down functionality by firmware versions (presumably it assumes that people have the most recent firmware) ... should it? Or perhaps add a "with recent firmware" footnote where relevant?

@canton7
Copy link
Collaborator Author

canton7 commented Feb 4, 2025

Yep, I had to back out the Battery SoH as it broke some inverters, see the linked issues.

The plan is to get it back into a beta, but I'm just holding off a bit until the dust settles: I'm having to do frequent releases to fix other breakages on old inverters at the moment, and I can't do that if I have potentially-breaking changes in beta.

@canton7
Copy link
Collaborator Author

canton7 commented Feb 4, 2025

Regarding the 'Supported Features' page on the wiki. This does not currently break down functionality by firmware versions (presumably it assumes that people have the most recent firmware) ... should it? Or perhaps add a "with recent firmware" footnote where relevant?

Yeah, that page predates us having support for multiple versions, each of which has different capabilities. If you fancy reworking it to be more accurate (maybe a proper section per inverter, and sub-section per version, with a separate table in each?), please go ahead.

@WyndStryke
Copy link

If you fancy reworking it to be more accurate (maybe a proper section per inverter, and sub-section per version, with a separate table in each?), please go ahead.

I'll give it a go, but I'll do it on a copy of the wiki first since I'm not very familiar with github, and don't want to accidentally ruin it.

I'll make a few variants and then post back here.

@canton7
Copy link
Collaborator Author

canton7 commented Feb 5, 2025

Thanks! There's a page history, so you can't permanently break it

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

No branches or pull requests

7 participants