-
Notifications
You must be signed in to change notification settings - Fork 14
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
Proof of concept - using givtcp modbus library #62
Conversation
revert partial refresh flag to False
Thanks for the prototype! I had a partial go at this myself a few weeks ago, but didn't get to the end of it. Your code will almost certainly be useful, but it's going to take me a while to find the time to polish off the changes and package everything up properly. |
To add to the conversation, I too make a fork of givenergy-modbus, but cherry picked the minimal changes to version 0.10.1 to work with the new inverter firmware as a stopgap. Changing the givenergy-modbus requirement to "givenergy-modbus @ https://github.com/gavanfantom/givenergy-modbus/archive/refs/tags/v0.10.1b.zip" in manifest.json ought to work, although I did end up manually installing it with pip in the homeassistant docker container. |
If I'm understanding this correctly, we shouldn't update to 911 firmware as this is a breaking change for this integration (currently), is that right? |
Damn it, I now see I saw this issue and completely forgot about it. I can now confirm that yes, if you update beyond 909 givenergy-local will not work :-( |
So - in desperation 😁 I've tried @gavanfantom's modbus version as described above which didn't work for me, I saw the same errors as with the original integration. I've also then tried installing @reidjr's fork with changes for 911, and when that didn't work I tried forking my own version of that (something i've never done before) and merging the handful of changes to the official integration since then. In both the latter cases (@reidjr's fork, and after I merged the latest @cdpuk changes) I got the following error when reloading the integration or restarting Home Assistant:
After I removed the isAIO=False from line 25 (reverting to the original) the error later changed to: 2023-12-08 11:33:37.214 ERROR (MainThread) [custom_components.givenergy_local.coordinator] Error fetching Inverter data: Error communicating with API: GivEnergyClient.refresh_plant() got an unexpected keyword argument 'isAIO' I removed all other traces of this flag, and had the same error, so replaced them all again and still have this error (which doesn't make a whole lot of sense to me). I don't suppose that is something anyone can help me easily fix? |
I'm going to close this down as the discussion on #61 seems to be reaching a stable state. The current approach is to avoid using the Once that's done, new issues to start bolting in extra features (such as those discussed here) are very much welcome. |
I don't expect you to accept this, but just to start the conversation. I updated my Gen2 inverter to 911 ( pre-release) and givenergy -local then fails. 909 is still supported, but after that the crc changes.
CRC and new features require an update to givenergy-modbus 10.1, which britkat has now forked to within givtcp.
To get this working ( with my basic skills) I cloned givenergy-modbus-10.1, and replaced the modules with the britkat versions. There are then breaking changes, including requirement to add an AIO flag to some of the module calls. I have stubbed those out, and worked around the others. Changed the manifest to require my github version of givenergy modbus, and it all ( sort of ) works.
I have moved my production HA to GivTCP, but wanted to get this working, and might be usefull if you want to do a major rewrite.
( I also added switch to reboot inverter, which was one of the new features)