GivTCP Compatability

[britkat1980]

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

[britkat1980]

This approach requires "Detect" to be used to grab appropriate registers for the inverter type, then use refresh_plant to grab data. I haven't worked out how to use watch_plant and access the updated data to process in an async manner (help would be great there)

[divenal]

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 Or maybe just givenergy_modbus.async_client. (The framer is currently marked as async, but there's absolutely no need for it to be - it's trivial to turn it back into an ordinary synchronous generator, which then allows it to be used in standalone snooping / replay scripts.)

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.

[divenal]

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 https://github.com/hoggyhoggy/givenergy-modbus-async/discussions/18 But doesn't change the interface, just converts registers lazily, rather than en-masse.

[divenal]

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.

[britkat1980]

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

[divenal]

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.)

[divenal]

Just FYI I've got a bunch of changes pipelined over in https://github.com/divenal/givenergy-modbus-async But I want to clean them up rather than just merging them directly into the prepare-for-1.0 tree.

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.)

[divenal]

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.

[hoggyhoggy]

Right, sorry, been away and would be getting glared at getting the laptop out. So...how do you want to play this - the GivTCP mods from Britkat currently will merge no worries BUT this will likely cause problems with Divenal's proposed merges that streamline the current code?

Edit: Merge done for Divenal's proposed changes to bring the tree up to parity with Givenergy_Local.

[hoggyhoggy]

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

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.

[divenal]

BUT this will likely cause problems with Divenal's proposed merges that streamline the current code?

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.)

[divenal]

So...how do you want to play this - the GivTCP mods from Britkat currently will merge no worries

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.

[hoggyhoggy]

I don't see ditching the rename causing too much chaos does it Britkat? (givenergy_modbus_async > givenergy_modbus )

[divenal]

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 Which means every internal import changes to givenergy_modbus_async

As well as the project name that gets published on pip ? Now maybe that's a good thing, since it's probably not backwards compatible with previous incarnations. But a major version number change is usually sufficient to indicate that, AFAIK.

[britkat1980]

I'm happy with whatever is decided for naming, I'm just currently running both in dev versions of GivTCP, so its easier to have them with different import names

[britkat1980]

I've also been adding the other battery models for the HV batteries and making watch_plant more usable in GivTCP. I'll commit these to the PR shortly. Shall I just change the inner folder name to givernegy_modbus_async

[divenal]

I've just learned that you can do relative imports:

from .abc import xyz
from ..mno import pqr

which would significantly reduce the disruption (since the source would no longer have to specify the givenergy_modbus bit in imports) Preferably as a preparatory change before the pull, rather than as part of the pull, though.

[britkat1980]

Yep, sounds sensible, so are we proposing to have the givenergy-modbus project with two sub folders, one with the sync version one with the original?

[divenal]

so are we proposing to have the givenergy-modbus project with two sub folders, one with the sync version one with the original?

No, I don't think so... client.py is the only async file. (Framer is marked up as async, but that's completely unnecessary, and one of the changes in my current pull request is to remove that: https://github.com/hoggyhoggy/givenergy-modbus-async/pull/29/commits/a43de1adb12741aa5d5d47f3783ad3f212968759 )

So there's absolutely no point cloning the entire thing as "async".

By "original", do you mean the clone you have in the GivTCP project ? Because the original standalone one is pretty much dead AFAIK (doesn't work with modern inverters). The givenergy-local people were using it, but when crcs were introduced it stopped working, and they merged in a copy of (more or less) this code.

Do you think there is a need for a synchronous client implementation. I've got a pretty clear idea how I could implement that, if you think it would be useful. Certainly a lot easier to debug synchronous code, so it might be useful to have just for that. But I assumed home assistant might prefer asyncio code ?

(BTW, the existing one on the GivTCP project - does it respond to heartbeat requests ? I couldn't see any code that did that.)

[divenal]

Yep, sounds sensible, so are we proposing to have the givenergy-modbus project with two sub folders, one with the sync version one with the original?

Are you planning on keeping an in-tree copy of the modbus code in GivTCP, or using this as an external dependency ?

[divenal]

I'm happy with whatever is decided for naming, I'm just currently running both in dev versions of GivTCP, so its easier to have them with different import names

Pondering the relative imports a bit more, I reckon it removes the need for a rename in this tree. All internal references to the name of the module are removed, so when you copy it into your GivTCP tree, you can call it anything you lke, and no code changes are required. So we can keep the name givenerg_modbus here, and you can call it async_modbus or new_modbus or anything else, and it will work fine.

Just trying locally with a temporary rename, I can do

>>> import async_modbus.client.commands
>>> async_modbus.client.commands.set_charge_target(44)
[<async_modbus.pdu.write_registers.WriteHoldingRegisterRequest object at 0x7fa4d95899d0>, <async_modbus.pdu.write_registers.WriteHoldingRegisterRequest object at 0x7fa4d93e7090>, <async_modbus.pdu.write_registers.WriteHoldingRegisterRequest object at 0x7fa4d95a1390>]

and it's perfectly happy. https://github.com/divenal/givenergy-modbus-async/commit/0c408d02b3ab3dfb828181f094fb15e4ce40152f

[britkat1980]

Yep, sounds sensible, so are we proposing to have the givenergy-modbus project with two sub folders, one with the sync version one with the original?

Are you planning on keeping an in-tree copy of the modbus code in GivTCP, or using this as an external dependency ?

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

[britkat1980]

so are we proposing to have the givenergy-modbus project with two sub folders, one with the sync version one with the original?

No, I don't think so... client.py is the only async file. (Framer is marked up as async, but that's completely unnecessary, and one of the changes in my current pull request is to remove that: a43de1a )

So there's absolutely no point cloning the entire thing as "async".

By "original", do you mean the clone you have in the GivTCP project ? Because the original standalone one is pretty much dead AFAIK (doesn't work with modern inverters). The givenergy-local people were using it, but when crcs were introduced it stopped working, and they merged in a copy of (more or less) this code.

Do you think there is a need for a synchronous client implementation. I've got a pretty clear idea how I could implement that, if you think it would be useful. Certainly a lot easier to debug synchronous code, so it might be useful to have just for that. But I assumed home assistant might prefer asyncio code ?

(BTW, the existing one on the GivTCP project - does it respond to heartbeat requests ? I couldn't see any code that did that.)

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.

[divenal]

What are 'hvbmu' and 'hvbcu' ? hv = high voltage => AIO Code seems to assume one hvbcu and multiple hvbmu.

perhaps bcu = 'battery control unt;, bmu = 'battery module unit' ?

[britkat1980]

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. For the AIO its always 1 BCU and 4 BMUs, but for three-phase it can be any number. I haven't got that far yet!

[hoggyhoggy]

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 do know however that for the older packs atleast, Giv appear to have more visibility on the batteries (such as Mosfet status - On or Off which basically indicates if the battery is able to charge/discharge or not) although I can find no register that this could be in documentation to hand. Do you know which registers they are? I'd quite like that one to debug something if I can.

[britkat1980]

I can only see one mention of Mosfet in the protocol and its register 81

[divenal]

Perhaps 3-phase is really intended for commercial / grid-scale systems, and so can have a very large number of batteries ?

[hoggyhoggy]

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):

[hoggyhoggy]

Perhaps 3-phase is really intended for commercial / grid-scale systems, and so can have a very large number of batteries ?

They have 3 phase domestic now: https://givenergy.co.uk/products/3-phase-battery-storage/ Battery stacks (1x BMU) can be anything from 3 to 6 modules per stack with unknown max multiple stacks all on one inverter!

[divenal]

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

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 __init__ which will just record everything it sees.

Oh - I see you've already been playing with such a thing, since you have code to record all frames:

    async def _task_network_consumer(self):
        """Task for orchestrating incoming data."""
        while hasattr(self, "reader") and self.reader and not self.reader.at_eof():
            frame = await self.reader.read(300)
            # await self.debug_frames['all'].put(frame)
            async for message in self.framer.decode(frame):

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.

[divenal]

They have 3 phase domestic now: https://givenergy.co.uk/products/3-phase-battery-storage/ Battery stacks (1x BMU) can be anything from 3 to 6 modules per stack with unknown max multiple stacks all on one inverter!

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

Adder: 0x50~0x6F
Register start address = (Register base NO) + 120 *  (BAMS_Addr - 0x90)  * 32 + 120* (BCU_Addr - 0x70);

means you can also access them some other way ?

[britkat1980]

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...