-
Notifications
You must be signed in to change notification settings - Fork 54
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
ReadRegisterAddressSplitter should split when addresses are having a "gap". #135
Comments
I have seen these "gaps" with WAGO controllers. This is one of the reasons why address splitter is composable class as I meant to implement something to work with these gaps - but never got to it and for own stuff I was able to convince automation engineers at work to use different address range so we do not hit the caps. I'll make |
NB: having smalls gaps between addresses is OK as sometimes you want ala register 1 and register 41 values which fit into single request. Returning registers 2,3...40 with 1,41 is OK and more performant than doing two request. But there could be address ranges that are not meant to be requested and splitter should split request quantity so that we would not overlaps these ranges. |
I had seen the same issue with ModbusTCP when I was building up one request for multiple addresses. But I was hoping that your more robust modbus client was able to handle it in the correct way. A real example of register address id's that should give me all the needed info of the dbus-service-name com.victronenergy.system (unit-id 100).
The ReadRegistersBuilder now builds up an array with one ModbusTcpClient\Composer\Read\Register\ReadRegisterRequest in it. That request starts with address 800 and uses a quantity of 67, this results in the following error on my Cerbo GX: The only way how I am able to read all the needed register addresses is by splitting them up into 6 requests. Thanks for looking deeper into this, it would be great if there could be a clean way to handle these "gaps" out of the box, without having to implement any extra 'tricks'. Simply setting some request option value would be great for this. |
Yep, this is same what I am talking - sometimes we need to split requests before we hit the quantity limit (~124 registers if I recall correctly). I'll add some method to builder that you can register unaddressable ranges for IP+PORT+UNITID combinations. so when there is a gap between current address and previous address we split or when current address falls into gap we throw exception. probably this weekend, you have unearthed thing that have been in my mind for a long time. |
Thanks for the update/fix. I honestly have to tell you that I am not going to use the code. I found out that Victron made it very easy for me. I can just use NODE-RED with all the devices as nodes in NODE-RED on my Cerbo-GX device. They get real-time updates and can be used as triggers to do other stuff. |
no problem, using Victron data over MQTT is pretty cool and anyway - PHP for long running processes is not a perfect fit. |
Is your feature request related to a problem? Please describe.
Yes, I am trying to read all registers of my Victron Energy Solar system (Cerbo GX) via Modbus TCP.
This works correctly when you request one specific address, but when you request multiple addresses at once, the ReadRegisterAddressSplitter is handling the registers in a way that the Victron Modbus server can't handle.
For example:
You request register addresses 800 and 806, then \ModbusTcpClient\Composer\Read\ReadRegistersBuilder::allFromArray will create one request for start address 800 with a quantity of 6, but Victron will send back an error that 801 is an invalid address.
Describe the solution you'd like
I found out that the \ModbusTcpClient\Composer\AddressSplitter::shouldSplit method could be simply modified so it will split into multiple requests when the addresses have a "gap". But there is no clean way to extend ReadRegisterAddressSplitter and inject (my own version) of shouldSplit
This is because a lot of properties like for example \ModbusTcpClient\Composer\Read\ReadRegistersBuilder::$addressSplitter are marked private instead of protected!
So simply extending the ReadRegistersBuilder and setting my own ReadRegisterAddressSplitter class in the construct is not possible :-(
Describe alternatives you've considered/are now using
I am now using reflection to "inject" my own class, but I hate to use this in my code.
Additional context
It would be great if some simple option parameter could be set, like "no-gaps" or something. That would then by default handle all addresses that are not an ascending +1 of the former one as a new request.
But otherwise, I think it would be very good to change all 'private' properties to 'protected' properties so that an adjustment by extending the code is possible.
The text was updated successfully, but these errors were encountered: