ReadRegisterAddressSplitter should split when addresses are having a "gap".

[weerdenburg]

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.

sendRequests resulted with modbus error. msg: Illegal data 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.

$readRegistersBuilder = ReadRegistersBuilder::newReadHoldingRegisters('tcp:https://192.168.100.6:502', 100);
        $reflectionClass = new ReflectionClass($readRegistersBuilder);
        $reflectionClass->getProperty('addressSplitter')
            ->setValue($readRegistersBuilder, new MyReadRegisterAddressSplitter(ReadHoldingRegistersRequest::class));

class MyReadRegisterAddressSplitter extends \ModbusTcpClient\Composer\Read\Register\ReadRegisterAddressSplitter
{
    protected function shouldSplit(Address $currentAddress, int $currentQuantity, Address $previousAddress = null, int $previousQuantity = null): bool
    {
        // These 3 lines of code fix the issue that I am having with requesting the data from my Victron Cerbo GX.
        if ($previousAddress && $previousAddress->getAddress() +1 < $currentAddress->getAddress()) {
            return true;
        }

        return parent::shouldSplit($currentAddress, $currentQuantity, $previousAddress, $previousQuantity);
    }
}

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.

[aldas]

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 *AddressSplitter fields protected and after that probably add way to add unaddressable gaps.
I have have to come up proper API for it. So first PR will be just changing fields to protected

[aldas]

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.

[weerdenburg]

I'll make *AddressSplitter fields protected and after that probably add way to add unaddressable gaps. I have have to come up proper API for it. So first PR will be just changing fields to protected

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

800
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
[GAP]
840
841
842
843
844
845
846
[GAP]
850
851
[GAP]
855
[GAP]
860
[GAP]
865
866

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:
Error processing function code 3, unit id 100, start address 800, quantity 67, src 192.168.100.105: Modbus address 827 is not registrered.

The only way how I am able to read all the needed register addresses is by splitting them up into 6 requests.
As I was also doing in the example code of this issue.

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.

[aldas]

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.

[weerdenburg]

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.

[aldas]

no problem, using Victron data over MQTT is pretty cool and anyway - PHP for long running processes is not a perfect fit.