Skip to content
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

Fix encoding & decoding of ReadFileRecordResponse #2319

Merged
merged 2 commits into from
Sep 14, 2024

Conversation

justinsg
Copy link
Contributor

Encode fix:

The sub-req file response length is in bytes, not registers, and needs
to also count the reference type byte. We already calculate the correct
value in `FileRecord.response_length` so make use of it here.

Decode fix:

The `count` increment was incorrect, because `response_length` already
includes the 1 byte for reference type. This moved the pointer too far
ahead, leading to incorrect slicing of record_data.

Also corrected the unit test for this; it seems to be copied from the
Modbus spec example but had a typo (repeated \x05) and did not assert
correctness of the second sub-req in the PDU.

The sub-req file response length is in bytes, not registers, and needs
to also count the reference type byte. We already calculate the correct
value in `FileRecord.response_length` so make use of it here.
The `count` increment was incorrect, because `response_length` already
includes the 1 byte for reference type. This moved the pointer too far
ahead, leading to incorrect slicing of record_data.

Also corrected the unit test for this; it seems to be copied from the
Modbus spec example but had a typo (repeated \x05) and did not assert
correctness of the second sub-req in the PDU.
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@janiversen janiversen merged commit aa7e556 into pymodbus-dev:dev Sep 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants