GivTCP Compatability #23
Conversation
givenergy-local merged a copy of the code into their tree at this point. Lots of changes made before the initial commit. Step 1 is just running most of the code through the 'black' util with standard line length and quote processing. So it should be a no-op change. Some files seem to have been excluded from the string processing: lut.py (appears to be no longer used) code.py framer.py __init__.py Was it just top-level py files that escaped ? Manually wrap a few import lines that aren't long in this tree, but become long when 'custom_component.givenergy-local.' was prepended to all the import locations.
This changes logger call style from (f'message with {variable}') to
('message with %s', variable).
Should have no net effect, but should make things a bit faster
since, when logging is turned off, it doesn't have to waste
effort fully formatting a string that will not be used.
For some reason, only client.py was done, but we can convert some more later.
(It's a recommendation from pylint - W1203.)
The byte-swap to little-endian is implemented in a rather ugly way, but we can fix that later.
Adds a new conversion exception. Defines some input registers. Adds some "interesting" ways to interpret some of the lower-numbered registers - derive hardware generation from arm firmware number - derive output power from model number
Some changes to make the client connection more robust. Other than the prefixes of custom_components.givenergy_local on all the givenergy_modbus imports, this makes the two trees identical.
This takes some of the changes from the v2.1.0 beta branch, to define the register and the command, but not the mechanism for extending the set of registers to ask for. I think we can do it a bit more tidily.
Additional registers for AIO/Gen 3 and new write commands
|
This approach requires "Detect" to be used to grab appropriate registers for the inverter type, then use refresh_plant to grab data. |
|
I have been pondering adding back in a synchronous client - it's a real pain trying to do interactive debugging on asyncio code. The most annoying thing I've found is that if a background task gets an exception, it just seems to get suspended. I guess the problem is that it doesn't have an easy way to report it. So eg I would find the app would stop, with all operations timing out. Hitting ^C then reveals that the task had this pending exception. It's actually only the client.py that needs to be explicitly asyncio-aware - everything else is just normal code. So rather than renaming the whole module async, perhaps just introduce an extra layer givenergy_modbus.async.client To implement a synchronous client, I was thinking that the application code would just call client.run(s) instead of using sleep when it's idle or waiting for something. s could be 0 if you just want to give the client a chance to service pending socket data before returning. Then client.run() would have a work queue that it would operate on, so it can handle timeouts, retries, etc. (I've got a pretty good idea of how that would look.) I had been playing with the givenergy_modbus in giv_tcp, but found it really didn't handle the socket very well. Perhaps I was just doing it wrong, though. commands.py could probably be made to be agnostic wrt sync or async client. |
|
I've not used watch_plant, but it looks like it's intended to be run as a background task, just issuing periodic requests to the inverter. You'd just get the data from client.plant.inverter as usual. No need to synchronise - asyncio isn't doing concurrent threads or anything, it's entirely co-operative multi-tasking. So until your code does an await on something, everything else is blocked. asyncio.sleep() is how you give give other pending tasks a chance to proceed. (To cycle back to the sync/async client, the async client would provide a run() which merely calls await sleep(), whereas the sync client version of run() would do active socket management.) I do have a change to get rid of the pydantic dependence from inverter.py. See #18 But doesn't change the interface, just converts registers lazily, rather than en-masse. |
|
On the AIO thing... what I'd been considering was, I think, something rubikcube suggested. (Or maybe I just misinterptered.) : always send the first request to slave address 0x11, which should work for all inverters. Perhaps that would be a request for the first 60 holding registers. Then you can inspect the appropriate registers and determine the hardware type. If it's not AIO, switch to a different slave address. Or maybe detect() always uses slave address 0x11. Something along those lines, anyway. |
|
On your AIO suggestion, that’s what I’ve done. Detect uses 0x11 to determine the type, then it uses 0x32 for future requests if it’s not an AIO. Just haven’t been able to test it yet |
|
Do you perceive the name change to _async to be permament, or just a temporary thing while transitioning over to the newer version. The standalone givenergy_modbus on pip is pretty much obsolete so I assume one goal is to replace that. I added some comments on the change in github. Unfortunately, not sure how you'd actually find them ..? (Hope you take them as intended - code reviews are just standard part of my day job, but I appreciate that that's quite rare, even in professional circles.) |
|
Just FYI I've got a bunch of changes pipelined over in https://github.com/divenal/givenergy-modbus-async I think we're both really just experimenting a bit at the moment in our respective dev trees. I also wanted the very first round of changes in the prepare branch to be getting into sync with the givenergy-local copy of the tree, since they started at exactly this point. (Haven't approached them yet, but want to able to say "look, our tree is identical to yours at this point", just in case they wanted to collaborate.) |
|
On watch_plant... another way to use it might be to 'await' it directly, but provide a handler callback which will be invoked each time round. |
|
Right, sorry, been away and would be getting glared at getting the laptop out. Edit: Merge done for Divenal's proposed changes to bring the tree up to parity with Givenergy_Local. |
Brings us up to parity with Givernergy_Local changes to the library. Changes as per pull request: "As promise, a sequence of changes to bring getting-ready branch up to givenergy-local. The first 5 get our tree identical to their one, except for the import statements. The last one takes some of the battery-pause support, but not all of it. (I'm hoping the merge will keep them as separate changes.) Don't know about you, but I was finding it almost impossible to separate the significant changes from the reformatting ones when it was all in one big lump."
Is there any merit in creating a blanket function that basically uses the slave address bug to our advantage? I'm not saying to call it indefinitely but one that could be called to "resync" with the portal? Perhaps spoofing the app/heartbeats request? I've not actually tried this yet but the fact there is no real way to resync without either pummelling the GE API or refreshing every register by hand in the remote control page has always bugged me. I'll have a play around and see what happens. |
In general, don't worry to much about introducing clashes with any of my changes - I think I know git sufficiently well that I can rebase things without too much difficultly. (It is slightly frowned on in git circles to rewrite history on changes, but I'm not terribly bothered about that.) |
Personally, I'm not convinced by the rename. I don't imediately see how it helps giv_tcp since the original givenergy_modbus is at the top level, whereas givenergy_modbus_async is down in GivTCP directory, so they don't immediately appear to clash ? If nothing else, the rename will get in the way of other merges. Probably won't affect the givenergy-local people too much, though, since they're already editing all the import statements, and would have to edit them anyway if they were to switch to an external dependency. |
|
I don't see ditching the rename causing too much chaos does it Britkat? (givenergy_modbus_async > givenergy_modbus ) |
I think the rename is the other way round: the inner directory changes from givenergy_modbus to givenergy_modbus_async As well as the project name that gets published on pip ? |
Run framer.py through the 'black' utility - for some reason, this was skipped by the givenergy-local project. This should bring everything into a canonical form which hopefully we can maintain. Also, convert the logging to use the % format, as client.py In battery and inverter, tidy up the imports - when doing multiple imports from one module, use one list rather than one list per name. Should have no net effect on code.
Not unless I need to. Ideally I just import the async lib from pip... I'm only doing it now while I keep the old sync clone I have in place as I iron out the kinks in using async |
The clone of the givenergy-modbus code I have in my GivTCP repo is heavily modified and works with most modern inverters. So its what's used for almost every GivTCP user. I'm keen to move across to the async lib and I'm testing it now with a few people and on the test kit in GE lab I don't think we specifically need a sync client, once the async one is stable. |
givenergy_modbus/client/client.py
Outdated
| # Some devices support additional registers | ||
| # When unsupported, devices appear to simple ignore requests | ||
| possible_additional_holding_registers = [180, 240, 300, 360] | ||
| for hr in possible_additional_holding_registers: | ||
| try: | ||
| reqs = commands.refresh_additional_holding_registers(hr) | ||
| reqs = commands.refresh_additional_holding_registers(hr,self.plant.slave_address) | ||
| await self.execute(reqs, timeout=timeout, retries=retries) |
There was a problem hiding this comment.
Not your change, but couldn't/shouldn't these probes be happening in parallel (by sending them all to a single exec() call), rather than sequential ?
|
What are 'hvbmu' and 'hvbcu' ? hv = high voltage => AIO perhaps bcu = 'battery control unt;, bmu = 'battery module unit' ? |
|
In the HV battery systems (AIO and 3phase) The battery management is very different. There is a BCU (Battery control unit) which manages a number of batteries (BMUs). So to get the relevant data you need to interrogate the BMS to see how many BCUs there are and for each BCU determine how many BMUs there are. |
Inverter class changes as per thread: hoggyhoggy#29 Closes Issues 16 & 20.
|
There's a lot of data in the new kit, AIO - 96 cells (24 per module) plus temps + pack volt, currents etc. Note sure how the 3 phase scales but probably multiples of those. |
|
I can only see one mention of Mosfet in the protocol and its register 81 |
|
Perhaps 3-phase is really intended for commercial / grid-scale systems, and so can have a very large number of batteries ? |
|
Sadly not (that's just how hot the parts i'm after are, which is close but I just can't find them and assume undocumented?), it's the below I'm curious to know how they know this status underlined in red (they should for instance switch states if the battery voltage is too low or high): |
They have 3 phase domestic now: https://givenergy.co.uk/products/3-phase-battery-storage/ |
| @property | ||
| def bcu(self) -> list[HVBCU]: | ||
| """Return HV Battery models for the Plant.""" | ||
| if 0x50 in self.register_caches.keys(): |
There was a problem hiding this comment.
Should that have been 0x70 in caches.keys() ?
Also, is it sufficient to test 0x70 in self.register_caches ? No need to extract the keys as a list and then check that the key is in that list - you can just test for the key directly 'in' the dictionary, AFAIK
New LUT for additional registers
Would it be possible to record the responses coming out of any of this test kit ? I have a trivial script which I can use to replay a responses data stream through the plant machinery to test changes without having to actually connect to my inverter. I typically just connect and then use the apps to click around to generate traffic. Several ways to do it: can use an external man-in-the-middle agent which records data passing through (eg 'socat'); can make an independent tcp connection to the inverter and just take advantage of the fact that all responses are broadcast to all connections; but I have been planning to add into client.py an optional binary stream parameter to Oh - I see you've already been playing with such a thing, since you have code to record all frames: Though I would just write directly to the file rather than using a queue and an asyncronous writer, and write in binary rather than hex. But you do get timestamps this way, so that can be useful. Easy enough to turn a hex dump back into a binary file. Or I could modify replay script to be able to read hexdumps too. |
The use of modbus addresses 0x50 to 0x6f seems to imply a limit of 32 battery modules in total ? Mght have been more scalable if the bcu had its own modbus to its modules, and then presented them all as a big bank of registers. (eg 0-59 = module 0, 60-119 = module 2, etc.) Then only the bcu consumes a modbus address. But maybe the slightly cryptic comment means you can also access them some other way ? |
| #possible_additional_holding_registers = [180, 240, 300, 360, 2040] | ||
| for hr in possible_additional_holding_registers: | ||
| try: | ||
| if hr == 2040: #For EMS there are only 36 regs in the 2040 block |
There was a problem hiding this comment.
And they couldn't just have padded it to 60 ? Dear oh dear...
givenergy_modbus_async/model/ems.py
Outdated
| class Generation(StrEnum): | ||
| """Known Generations""" | ||
|
|
||
| GEN1 = "Gen 1" |
There was a problem hiding this comment.
seems to be a bit of duplication of these simple enums. Perhaps put them in a shared file like __init__ or something ?
givenergy_modbus_async/model/ems.py
Outdated
| FLASHING_FIRMWARE_UPDATE = 4 | ||
| @classmethod | ||
| def _missing_(cls, value): | ||
| """Default to 0.""" |
There was a problem hiding this comment.
There is an unused
class DefaultUnknownIntEnum(IntEnum):
"""Enum that returns unknown instead of blowing up."""
@classmethod
def _missing_(cls, value):
return cls.UNKNOWN # type: ignore[attr-defined] # must be defined in subclasses because of Enum limits
in __init__.py which could reduce duplication. I might be inclined to use -1 for the default unknown / missing value, since that can't appear in an unsigned int16 register. Don't like defaulting to a legal value, since you can't distinguish missing from genuinely 0.
(To be honest I'm not really very big on enums - I tend to just stick to ints for everything !)
Rather than explicitly specifying givenergy_modbus. on every import, use relative naming with . and .. This means that, while developing, it is possible to locally rename the directory containing the tree and, with no source changes required, it will happily run as the new package name. This allows people to more easily compare old and new behaviour by having concurrent installs with different names. Also, simplify an import in register_cache.py - it was conditionally importing TimeSlot at the top of the file, but then unconditionally importing it later on. So just do the unconditional import at the top. No net effect on runtime behaviour.
Arrow is a "better" datetime, with better parsing, etc. However, givenergy_modbus doesn't use any of these enhanced features - it simply accepts an instance as a parameter to commands.set_system_date_time() and reads out the year/month/day etc. Fortunately, Arrow implements the same interface as the standard datetime class, so we can trivially change the declaration to accept a plain datetime.datetime instance. An application is still perfectly free to use Arrow for date manipulation, and can pass an instance of that to commands.set_system_date_time() (Some type-checking tools might complain, though.)
| "enable_standard_self_consumption_logic": Def(C.bool, None, HR(199)), | ||
| "cmd_bms_flash_update": Def(C.bool, None, HR(200)), | ||
| "charge_target_soc_1": Def(C.uint16, None, HR(242)), | ||
| "charge_slot_2": Def(C.timeslot, None, HR(243), HR(244)), |
There was a problem hiding this comment.
This seems to be a duplicate "charge_slot_2" - it's also at HR(31)/HR(32) ???
(reported by pylint)
In order to be able to write timeslots, we need to be able to address the start and end registers directly. Use a script to add a start and end register alias for each timeslot pair, with valid settings to mark them writable. Also add valid=(4, 100) to charging soc registers.
Inverter and Battery generate the class docstring dynamically, so that it includes a generated list of all the register names.
givenergy_modbus/model/battery.py
Outdated
| @@ -71,6 +71,7 @@ class BatteryRegisterGetter(RegisterGetter): | |||
| "t_max": Def(DT.deci, None, IR(103)), | |||
| "t_min": Def(DT.deci, None, IR(104)), | |||
| # IR(105-109) unused | |||
|
|
|||
There was a problem hiding this comment.
Perhaps this edit suggests you intended adding registers here ?
...
| @@ -312,6 +347,12 @@ class InverterRegisterGetter(RegisterGetter): | |||
| "temp_battery": Def(C.deci, None, IR(56)), | |||
| "i_grid_port": Def(C.centi, None, IR(58)), | |||
| "battery_percent": Def(C.uint16, None, IR(59)), | |||
| "e_battery_discharge_total": Def(C.deci, None, IR(105)), | |||
There was a problem hiding this comment.
... But perhaps you added them here by mistake ?
105 and 106 overlap the battery input register block 60-119.
|
I've merged the PR from Divenal into my getting ready branch, and tweaked it to make it work with different types. I'm going to close this PR and push another one to that branch. Not sure if that's the right thing to do, but I've not got the hang of git... |
I've changed folder name to givenergy_modbus_async to allow it to run alongside (and be differentiated with) the sync version
Added all updates I've made of the the last 18months to registers and control mechanisms
Should accomodate AIO/Gen 3 etc... but untested at the moment.
I have it running in read and write and GivTCP dev version 2.4.97+ uses this code
Happy for feedback on changes I can make