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

Allow retrieval of multiple different types in single call #1

Open
YodaDaCoda opened this issue Jan 17, 2022 · 9 comments
Open

Allow retrieval of multiple different types in single call #1

YodaDaCoda opened this issue Jan 17, 2022 · 9 comments

Comments

@YodaDaCoda
Copy link

Love the work mate! 👍 I'm writing a HomeAssistant plugin to leverage this with my solar inverter.

Looking at read_holding_registers and the like and how they accept quantity and return a list of numbers - I'd like instead to pass a variable of some kind that defines the types of variables to read, and calculate the quantity dynamically, returning a list/tiple of the parsed types, probably using struct.unpack.

e.g.

conn = PySolarmanV5("192.168.2.51", 1111111111, port=8899, mb_slave_id=1, verbose=1)
modbus.read_holding_registers(register_addr=0, format='>10H2I')

Internally, pysolarmanv5 should then be able to construct a struct.Struct from the provided format and use the .size property in place of the existing quantity parameter.

May need to have a class to allow for the existing scale/signed/bitmask/bitshift stuff to be handled nicely.

I hope to get time to look at this myself in the coming days, but if not this will serve as a springboard / reminder.

@jmccrohan
Copy link
Owner

Thanks for the feedback. I'm particularly interested in your work on a HomeAssistant plugin; I haven't gotten around to writing one for this myself, so keep me updated on that please. For info, @NosIreland has used pysolarmanv5 as part of a MQTT integration here. However, a native HomeAssistant integration would be very useful as it would allow users to write to their hybrid inverter to set SOC/charge times etc.

As regards your proposal, I'm not adverse to this change, but I'm keen to understand your usecase a little better. For example within HomeAssistant, the native Modbus integration uses a similar-ish approach to pysolarmanv5:
https://community.home-assistant.io/t/solis-inverter-modbus-integration/292553/25

@YodaDaCoda
Copy link
Author

I'm keen to understand your usecase a little better

For efficiency purposes, I want to be able to read all the registers at once, and perform data translation on the result. Currently, I'd have to read each register individually in order for pysolarmanv5 to perform the bit-shifting/dividing/etc as required for each piece of data. I could, of course, do it myself, but it makes sense to me to enable this functionality with the library.

Since it's returning a list of ints, I'm also a little unclear how those ints translate to the bytes returned from the modbus protocol, since reading 40 registers gives me a list of 40 elements but some of them have values >255 which indicates umodbus.client.serial.rtu is giving us 2 bytes per value, so I should maybe be reading 20 registers. It's been a few years since I was playing with modbus, and I was working with bytes directly, so perhaps this is normal behaviour, but some of the bits of data we want to read are 4 bytes, so I think we need more flexibility in this.

within HomeAssistant, the native Modbus integration uses a similar-ish approach

I think that the native implementation is really designed around reading a single register at a time, while we've got much greater flexibility when setting up a custom integration. Performing a single network request is, IMO, a much better way of doing things, and I think adding this type of data translation will make pysolarmanv5 easier to use too.

I'm particularly interested in your work on a HomeAssistant plugin

It's my first time trying to make a plugin for HomeAssistant so it's been a bit of a learning curve. I've got it configuring an entity and sensors, and reading values from the logger, but need to get the data translated properly (and orchestrating data refreshes in a nice async way). I only have an hour or so per day I can work on it before the sun goes down and the inverter powers off so development isn't going to be particularly rapid.

I'm confident I can get it reading values from the data logger for display in a dashboard and hopefully integration with HomeAssistant's new Energy Management stuff, but writing values (and probably making it handle errors better and such) is something I'd have to leave up to the community.

@jmccrohan
Copy link
Owner

For efficiency purposes, I want to be able to read all the registers at once, and perform data translation on the result. Currently, I'd have to read each register individually in order for pysolarmanv5 to perform the bit-shifting/dividing/etc as required for each piece of data. I could, of course, do it myself, but it makes sense to me to enable this functionality with the library.

That makes sense. I have done more digging on the V5 protocol so need to update the frame creation/validation logic with my findings first. If you want to fork and create a PR for this, that would be great. Otherwise I'll work on this after.

Performing a single network request is, IMO, a much better way of doing things, and I think adding this type of data translation will make pysolarmanv5 easier to use too.

Agreed, but we'll need to consider usability for new users too. The Format Strings notation is a bit opaque for newbies. A helper program to take an input file of Modbus registers and output the Format Strings notation might be helpful, particularly if you intend on using that in the HA integration config.

Out of interest, what inverter/data logging stick do you have? I'm using a Solis Hybrid. It would be good to get a list of known compatible devices.

@YodaDaCoda
Copy link
Author

Out of interest, what inverter/data logging stick do you have?

I have an LSW-3 Stick Logger. I wonder if the different devices have their information at the same registers - I hope they do for ease of data retrieval. Otherwise I'll have to add something to handle different devices.

The Format Strings notation is a bit opaque for newbies. A helper program to take an input file of Modbus registers and output the Format Strings notation might be helpful

I was thinking a class that defines how a register is read/translated would be a nice way of doing this. I wouldn't expose the format strings to the end-user in the integration. At its most basic we'd pass a type to this class (e.g. modbus.read_holding_registers(register_addr=0, format=[PySolarmanV5.Dataframe(type=PySolarmanV5.Integer)] - Dataframe may not be the best name but good enough for an example/discussion). pysolarmanv5 would build up the full format string from the list that's passed to it and after reading the modbus data, would struct.unpack the bytes and give each Dataframe instance its raw unpacked value. It'd then build up the list of values to return to the end user by retrieving the parsed value from the Dataframes.

If you want to fork and create a PR for this, that would be great.

I'd love to, just gotta find the time.

@jmccrohan
Copy link
Owner

I have an LSW-3 Stick Logger. I wonder if the different devices have their information at the same registers - I hope they do for ease of data retrieval. Otherwise I'll have to add something to handle different devices.

Unfortunately the Modbus mappings will vary from manufacturer to manufacturer (and sometimes even vary between the various models from same manufacturer), so any solution will need to account for this.

pysolarmanv5 would build up the full format string from the list that's passed to it and after reading the modbus data, would struct.unpack the bytes and give each Dataframe instance its raw unpacked value.

Okay. In terms of polling the inverter for readings, you'll have to take into account the maximum number of registers that can be queried in a single request (125), and the fact that the registers may not be contiguous or be defined in that range. I need to look again, but last time I looked, I think the information I was interested in pulling into HA requires a minimum of 3 requests in its most optimised format.

@YodaDaCoda
Copy link
Author

Unfortunately the Modbus mappings will vary from manufacturer to manufacturer

I've updated my component to handle different device definitions, but it may need more work for more complex inverters (such as yours).

maximum number of registers that can be queried in a single request (125)

It'd be nice to have pysolarmanv5 abstract that away, but that may be rather complex (and it may be necessary to read different register areas anyway if they're significantly far apart, i.e. non-contiguous as you suggest). For my model, I have managed to get most of the information out of it in a single request of only ~40 registers. Your inverter appears to be significantly more fancy than mine!

image

NB: I'm only pulling information from the holding registers, I believe there's other information that can be read from the input registers such as firmware versions and the like, but I haven't looked at reading that info yet.

@YodaDaCoda
Copy link
Author

I've just pushed my custom component to github. It's a bit rough, but it's functional. Have a look and let me know what you think? https://github.com/YodaDaCoda/hass-solarman-modbus

@jmccrohan
Copy link
Owner

Thanks for this. Will take a look over the coming days.

@jmccrohan
Copy link
Owner

Hey, just getting a chance to come back to you on this now.

This looks good. Two immediate things things come to mind;

  1. Ability configure the modbus registers through a config file
  2. Ability to make multiple modbus requests (for non-contiguous modbus registers like mine).

P.S. I haven't forgotten about the struct.pack() type interface, I just haven't gotten a chance to work on it yet.

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

2 participants