708 Implement ReadFileRecordRequest.get_response_pdu_size() to ensure complete record

[theobarkerh]

Possible fix to #708

ReadFileRecordRequest does not include an implementation of get_response_pdu_size() so the rtu_framer completes prior to a full response received. Causes many errors during a file read operation.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[dhoomakethu]

I haven't used the ReadFileRecordRequest so just dropping the spec for future reference

[theobarkerh]

Is this ready to merge? I.e., the failed checks are from upstream code unrelated to this pull request.

[theobarkerh]

Suggestions on how to resolve??

[christiansandberg]

I really think pymodbus should use the response data length (byte after the function code) instead to determine how many bytes to read next. I don't think you should rely on the server returning the requested length because some files may behave like in a file system where reading past EOF will return less bytes than requested.

[theobarkerh]

I really think pymodbus should use the response data length (byte after the function code) instead to determine how many bytes to read next. I don't think you should rely on the server returning the requested length because some files may behave like in a file system where reading past EOF will return less bytes than requested.

Have you tried transferring files from devices produced by multiple vendors?

[christiansandberg]

This change will for sure break compatibility with certain devices. The method used currently, although unstable, is correct since it doesn't make assumptions about the response length based on the request. I'm guessing this was the reason why this method is missing from file records in the first place.

Using the byte count instead will not change the current behavior, as long as the byte count given by the device is correct of course.

[theobarkerh]

This change will for sure break compatibility with certain devices. The method used currently, although unstable, is correct since it doesn't make assumptions about the response length based on the request. I'm guessing this was the reason why this method is missing from file records in the first place.

Using the byte count instead will not change the current behavior, as long as the byte count given by the device is correct of course.

The current method is unusable. Thus the PR.
This was the best solution I found based on my limited knowledge of the code base and specification. Please add code changes you would suggest to the diff on this PR.

[janiversen]

@theobarkerh can you please test against the current dev, there have been multiple corrections, that leads me to believe that your PR might no longer be relevant, however you are the best to say so.

[theobarkerh]

@theobarkerh can you please test against the current dev, there have been multiple corrections, that leads me to believe that your PR might no longer be relevant, however you are the best to say so.

Thanks Jan! I'll test it as soon as a get a break to do that. Hopefully today or tomorrow.

[theobarkerh]

@janiversen, I was able to test with the code which tripped over the implementation in the last release.
The 'dev' branch ran much better. I didn't have a chance to instrument it to verify that the timing is optimal.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
11.7% Duplication

[theobarkerh]

@janiversen, I was able to test with logging.setLevel(logging.DEBUG) and compare this implementation with 'dev'. They do appear to have the same efficiency.

[janiversen]

I am confused you talk about efficiency, but your pr is about errors when reading files, I do not see the relation.

I never said that dev was more efficient, but partial reads have been reduced.

[theobarkerh]

I am confused you talk about efficiency, but your pr is about errors when reading files, I do not see the relation.

I never said that dev was more efficient, but partial reads have been reduced.

Partial reads [requiring retries] is the efficiency problem to which I referred. Thank you. As you noted, this PR may not add any improvement over the current 'dev' implementation.

[janiversen]

Ok, but it still makes sense to have a correct get_response_pdu_size()

[christiansandberg]

Unfortunately this will cause communication with our devices to fail since the size indication encoded in the response gets ignored. I know that the specification says that there probably should be a relation between request and response but the protocol also has an explicit byte count for the actual response size and this should take priority over any implicit calculations.

One example of a use case where the spec is not always followed is when transferring large files using Write File Record. The response should contain the exact same data as in the request but this wastes a lot of bandwidth and many implementations choose to not send the data back. The Modbus protocol supports this behavior both by indicating the byte size and the underlying framing layer.

In this regard the previous behavior was better since it was more flexible in response sizes it accepted. Now it will cause a timeout on perfectly valid responses.