Fix coil read problem.

[glcj]

Modbus driver does not read Coil & Discrete Inputs correctly.

1. It does not return Coil & Discrete Inputs
2. The bits are returned in order from top bit to bottom bit and must be inverted.

Coil writing needs to be tested.

[chrisdutz]

I implemented this in a way that it worked with the setup my coleagues were working with ... I do expect that these changes would break things with them.

For the different endianess we explicitly introduced the: 'default-payload-byte-order' configuration option.
Have you checked that none of the existing encoding options:

• BIG_ENDIAN
• LITTLE_ENDIAN
• BIG_ENDIAN_BYTE_SWAP
• LITTLE_ENDIAN_BYTE_SWAP
  Help in your case?

[chrisdutz]

Having a more detailed look it seems you half-overrode the functionality I have in getReadBuffer in your adapted getResponseDataForTag method. Also removing the entire coil handing a bit higher in the class will 100% break things in the usecase that I tested things with.

[glcj]

Hello,

Thanks for your time in checking,

I performed the tests with default configuration values ​​since the bit return order does not correspond to the order of the requested registers (not BYTES).

Eventually the modification corrects the reading of bit arrays in coils "0x00001:BOOL[48]". The driver version 0.13.0-SNAPSHOT does not return the values ​​(at least until the tests performed last night).

Regarding the Inputs Register, it does return the bit array "1x00001:BOOL[48]" but not in the correct order.

If the people you point out are using the driver, they are probably only using the registers that do work. If you can discuss this with that team, that would be excellent.

Many applications use only registers.

I compared the traces against Applicom, CAS and ModbusPoll, so the protocol dialog is not the problem. I've already posted the screenshots on Slack with both types of records 0x0 and 1x0 requested and received in the correct order.

Please tell me which tests do not pass so I can compare apples with apples.

Thanks again,

[chrisdutz]

So when working on the optimizer I added test-cases for the situations I was implementing ... it sort of seems as if you didn't run the testsuites. If I switch to your branch I see several of the tests failing. Please specially pay attention to the tests in the
plc4x/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer directory. I particularly modelled exactly the request structutes we were reading. So I guess the LittleEndianByteSwapTest is a perfect test for that usecase and it clearly states that your patch breaks this scenario.

Please pay a bit more attention to the tests that we have ... they are not for being skipped.

[glcj]

Hi Chris,

I think my added value here is that I can test the driver against various devices and I can tell you again that Modbus for 0x and 1x functions has problems in the 0.13.0-SNAPSHOT development branch and a solution proposal is in this PR.

Here we are talking about bits and bytes, but in my mind I get an image of a valve that has CO2 on one side and NaOH on the other. Unit tests help, but my tests will be with real equipment. If I find any strange ones I will notify the team immediately, as I am actually doing.

Attached test with the Applicom driver as MODBUS server, the interesting thing about this driver is that it can vary the protocol depending on various TSX QUATUM models or standard Modbus.

Thank you for your support,

[chrisdutz]

Well... The test is the data from a real world use case... I can have a more detailed look, but I am quite sure you're not setting the right endianess. I would expect your changes to not break my test case. Otherwise they break my usecase.

[chrisdutz]

@glcj could you please simply whip up a unit test like the one I created for my test case? I guess only this way will we be able to implement something that works with both devices.

[glcj]

Hi Chris,

I've already placed in the "fix/modbus" branch the test based on the frame returned by one of my devices and according to the pattern of the existing tests . (OpenModSim, Applicom and Odot gateway).

I tried to evaluate the frame of your reference test but none of the Modbus parsers could interpret it, I leave you in the notes the link of the tool (CAS doesn't interpret it either).

If you have the device model that you used to generate that pattern it would be interesting to have it as a general knowledge.

I'll be attentive to any observations,

Best regards,

[chrisdutz]

So ... Even if we ignore the test that I built for the setup of my colleagues, I still have failing tests:
The ModbusTypAllDatatypesIT fails for the boolean register
Also the ManualModbusTCPDriverTest fails for the BOOL address

[glcj]

Hi Chris,

To continue with the tests I ask you to work with a common device so we can compare apples with apples. I propose to you:

https://github.com/serhmarch

The results obtained with this Simulator are the same with the Applicom driver, ModbusPull, etc.

Or if you have another emulator of your choice that I can access, I will gladly download it.

We are going to run the driver without any type of BYTE exchange since the responses of these devices are standard. Regarding this point, I left you an observation in the test that I added.

It would also be helpful if you put the complete captures of the device under test.

Thank you for your time,

[chrisdutz]

So ... having a more detailed look on your PR and the code you changed ... so in the ModbusOptimizer you removed the handling of the Coils and treat everything like a register ... however for a register one element consumes 2 bytes and a coil it consumes one bit. These "matchesRegister" and "matchesCoil" calls should produce quite different results. Not sure how it even works without it, admittedly.

I also think the main difference is: The optimizer is made for grouping multiple tags together and to reduce the number of requests sent to the PLC. In your case you simply read one tag, which is an array. I think I should probably have a look if there's anything special with that.

Right now merging this PR will effectively break the optimizer ... how about disabling/enabling it via configuration?

[chrisdutz]

So I don't have the time to address this ... so if you ensure the test-suite is green, feel free to merge ... if things break for my colleagues (which I expect) I might get the time to fix it again, but with my current 4h/week ... no chance.

[glcj]

Ok, I'll check everything again and verify the tests. With everything in green I'll perform the merge. If anything comes up I'll be on the lookout.

[glcj]

The following changes were included in this modification.

1. Reading of a BOOL type from a HoldingRegister is enabled. In this particular case the HR must have a value of 0/1.
2. In the case of Coils and InputRegisters, they are always treated as BIG_ENDIAN, as specified by the standard.
3. In the case of optimized readings the value of the bit is interpreted directly from the acquired buffer and transferred as a short.
4. In all cases of bit readings in arrays or from an optimized call, the bits are returned in a consistent order.
5. All tests provided for the driver pass.

[chrisdutz]

Giving up on this ... merge if you like.