search

hoggyhoggy / givenergy-modbus-async

A python library to access GivEnergy inverters via Modbus TCP on a local network, with no dependency on the GivEnergy Cloud.
Other
1 stars 2 forks source link

inverter class changes #29

Closed divenal closed 1 month ago

divenal commented 2 months ago

Next round of structural changes, for review/comment. closes https://github.com/hoggyhoggy/givenergy-modbus-async/issues/20 closes https://github.com/hoggyhoggy/givenergy-modbus-async/issues/16

divenal commented 2 months ago

Sorry about that - having another go These are again relative to the prepare-for-1.0 branch rather than the dev branches.

Main changes are getting rid of pydantic for inverter and battery classes, so that the classes now convert values on the fly when individual attributes are accessed, rather than converting everything to friendly format in python attributes when the inverter object itself is instantiated.

And then subsequent to that, introduce an optional 'valid' field to the master copy of the holding register definitions in inverter.py. Provide a mechanism to remove the register list in commands.py but I haven't actually gone ahead and converted the existing commands yet.

(Just realised that the one timeslot register I had aliased - charge_slot_2 - was actually defined incorrectly in inverter.py !)

divenal commented 2 months ago

Being profoundly lazy, I didn't relish the prospect of manually entering dozens of trivial setter methods in commands.py Nor maintaining such a suite. Fortunately, I think it can be done on the fly... (We'd still keep the non-trivial ones as explicit methods.)

In a similar way to how (with this change), access to inverter.xyz results in a call to inverter.get('xyz'), I'm pretty sure I can set things up so that Commands.set_xyz(value) turns into a call to Commands.write_named_register('xyz', value)

And I can probably special-case set_xxx_slot_y() and assume it's a timeslot, and so (try to) write to 'xxx_slot_y_start' and 'xxx_slot_y_end'.

divenal commented 2 months ago

(Just noticed that I had managed to break the conversion of the system time to a datetime. Updated now.)

divenal commented 2 months ago

Any progress on reviewing or merging these ? I have more changes in the pipeline - I could add them to this branch, which (I'm guessing) they would get included in any pull request, or I could hang on for now.

Merging changes is relatively benign in git - if you decide you don't like a change, you can create a retrospective branch before it and resume development as if it was never incorporated.

hoggyhoggy commented 1 month ago

Sorry, Been incredibly hectic at work so had no free time lately. Let me have a quick look through but suspect i'll just do a merge and all will be fine. (Same goes for why I've been quiet elsewhere on here)