RTU: Calculate length from response

[christiansandberg]

When the response length is unknown, try to calculate the frame size using the first bytes of the response.

Fixes #708.

[janiversen]

Are you sure this also works for messages with subrequests?

[christiansandberg]

Which messages have subrequests? I believe it should work since I just used the existing method for calculating frame size.

[christiansandberg]

I think ReadFifoQueueResponse would require 4 bytes to be read. But all RTU frames should contain at least 4 bytes (address + function code + CRC) so that should not be a problem to do.

[christiansandberg]

Also ReadDeviceInformationResponse requires even more, so I should catch IndexError as well in case the information is stored further down the byte stream.

[theobarkerh]

Have you tested file transfers with devices from multiple vendors devices? Did you verify the timing? I.e., it could succeed only because a timeout occurred when it should complete based on byte count.

[christiansandberg]

I am only able to test it on our own devices unfortunately. File records are not as common in the other Modbus devices we use. I'm hoping someone else could verify that it works on their devices.

I'm not sure what you mean by timing. We currently have issues together with the FTDI driver which by default sends data from the device to the USB-host every 16 ms, but since Pymodbus checks for data every 10 ms it often thinks that the frame has ended in the middle of the stream. But with this change it works perfectly. The same method is used on other Modbus stacks I've encountered. The byte count for a specific frame must always be correct, but may be independent from the request.

[dhoomakethu]

@christiansandberg but since Pymodbus checks for data every 10 ms it often thinks that the frame has ended in the middle of the stream Can you expand a bit on this please ?

[christiansandberg]

This is a basic flow chart of how pymodbus works (sorry it's not very good looking and maybe not 100% accurate). So if it is not possible to reliably determine the response length from the request, pymodbus periodically checks for incoming data. If no data has been received since last 10 ms, the frame is assumed to be complete.

graph TD
    known_length[get_response_pdu_size method exists?] -- yes --> read
    read[Read expected response size] --> parse
    known_length -- no --> check
    check{New data received?} -- yes --> sleep
    check -- no --> parse
    sleep[Sleep 10 ms] --> check
    parse[Parse message]

Loading

https://github.com/riptideio/pymodbus/blob/d79c9c6526b14e938346b9bd1bca3829d78838b4/pymodbus/client/sync.py#L715-L734

The issue is that it's not reliable on certain USB to serial converters like FTDI. Data is sent in batches every 16 ms so there are often occasions where no data has been received within 10 ms because it is being buffered somewhere else.

[janiversen]

You are correct the receive function is flawed. The first problem is that we try to guess the response size instead of using the information in the response headers.

Once I have finalized getting the code to pep8 standard (see black PRs) making _recv failure tolerant is next on my list, utilizing the PR trying different ways to solve the same problem.

[dhoomakethu]

@christiansandberg thanks for the explanation, I see what you mean.

[dhoomakethu]

For sync clients, we also calculate response size for most of the requests here https://github.com/riptideio/pymodbus/blob/2c9f186bdf28ffa0fdcacb906d02a1c8084c70e4/pymodbus/transaction.py#L147
https://github.com/riptideio/pymodbus/blob/2c9f186bdf28ffa0fdcacb906d02a1c8084c70e4/pymodbus/transaction.py#L83

We will have to definitely handle this in a better way

[janiversen]

yup, it is in the works.

When stress testing pymodbus, I have identified a couple of problems. We have 2 separate problems (reproduced on tcp, with a hacked server):

• a single message is divided into multiple reads, causing _recv to deliver an incomplete message
• multiple messages are delivered in a single read, causing _recv to loose part of the message.
  As far as I can tell the problem is identical for both sync and async (remark I am only testing asyncio)
• ,

[theobarkerh]

Perhaps we should address the problematic USB/Serial interfaces (16ms batch) as an exception, rather than mainline code.

[janiversen]

I politely disagree, the current code makes assumptions how the packets are read and I believe that to be wrong.

[theobarkerh]

Perhaps we agree more than you realize. The more code paths which can use the get_response_pdu_size method, the better. Timeouts should be exceptional path. I agree current code is broken, thus I submitted PR #708, based on my limited knowledge of the entire structure, present problem and limited time available.

[janiversen]

This is a basic flow chart of how pymodbus works (sorry it's not very good looking and maybe not 100% accurate). So if it is not possible to reliably determine the response length from the request, pymodbus periodically checks for incoming data. If no data has been received since last 10 ms, the frame is assumed to be complete.

https://github.com/riptideio/pymodbus/blob/d79c9c6526b14e938346b9bd1bca3829d78838b4/pymodbus/client/sync.py#L715-L734

The issue is that it's not reliable on certain USB to serial converters like FTDI. Data is sent in batches every 16 ms so there are often occasions where no data has been received within 10 ms because it is being buffered somewhere else.

There is something I do not get here. On a serial line (RS-485) Modbus defines the delay between messages to be 3.5Char, and no delay within the message itself, so to me it looks as if your 16ms is the break between packets (of course it depends on the baud rate you use).

We only want to receive 1 message and not by accident start receiving part of a new message, hence we need to sleep for the time it takes to receive one byte at the slowest baud rate.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.1% Duplication