From a8fb3aaa13388544214a79abcf60f267c87e402e Mon Sep 17 00:00:00 2001 From: Justin Standring Date: Sat, 14 Sep 2024 23:39:50 +1200 Subject: [PATCH 1/2] Fix encoding of ReadFileRecordResponse 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. --- pymodbus/pdu/file_message.py | 2 +- test/test_file_message.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pymodbus/pdu/file_message.py b/pymodbus/pdu/file_message.py index 37adbd5b6..232ce699c 100644 --- a/pymodbus/pdu/file_message.py +++ b/pymodbus/pdu/file_message.py @@ -170,7 +170,7 @@ def encode(self): total = sum(record.response_length + 1 for record in self.records) packet = struct.pack("B", total) for record in self.records: - packet += struct.pack(">BB", record.record_length, 0x06) + packet += struct.pack(">BB", record.response_length, 0x06) packet += record.record_data return packet diff --git a/test/test_file_message.py b/test/test_file_message.py index 36ef6ee0e..a4270e9c7 100644 --- a/test/test_file_message.py +++ b/test/test_file_message.py @@ -163,7 +163,7 @@ def test_read_file_record_response_encode(self): records = [FileRecord(record_data=b"\x00\x01\x02\x03\x04\x05")] handle = ReadFileRecordResponse(records) result = handle.encode() - assert result == b"\x08\x03\x06\x00\x01\x02\x03\x04\x05" + assert result == b"\x08\x07\x06\x00\x01\x02\x03\x04\x05" def test_read_file_record_response_decode(self): """Test basic bit message encoding/decoding.""" From 7c42911fe1ecfecef0df58910e3ecaea1a13fd7b Mon Sep 17 00:00:00 2001 From: Justin Standring Date: Sat, 14 Sep 2024 23:47:57 +1200 Subject: [PATCH 2/2] Fix decoding of ReadFileRecordResponse 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. --- pymodbus/pdu/file_message.py | 7 +++++-- test/test_file_message.py | 13 +++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/pymodbus/pdu/file_message.py b/pymodbus/pdu/file_message.py index 232ce699c..954b4e89f 100644 --- a/pymodbus/pdu/file_message.py +++ b/pymodbus/pdu/file_message.py @@ -185,11 +185,14 @@ def decode(self, data): response_length, reference_type = struct.unpack( ">BB", data[count : count + 2] ) - count += response_length + 1 # the count is not included + count += 2 + + record_length = response_length - 1 # response length includes the type byte record = FileRecord( response_length=response_length, - record_data=data[count - response_length + 1 : count], + record_data=data[count : count + record_length], ) + count += record_length if reference_type == 0x06: self.records.append(record) diff --git a/test/test_file_message.py b/test/test_file_message.py index a4270e9c7..ab2aae876 100644 --- a/test/test_file_message.py +++ b/test/test_file_message.py @@ -167,13 +167,18 @@ def test_read_file_record_response_encode(self): def test_read_file_record_response_decode(self): """Test basic bit message encoding/decoding.""" - record = FileRecord( + record1 = FileRecord( file_number=0x00, record_number=0x00, record_data=b"\x0d\xfe\x00\x20" ) - request = b"\x0c\x05\x06\x0d\xfe\x00\x20\x05\x05\x06\x33\xcd\x00\x40" + record2 = FileRecord( + file_number=0x00, record_number=0x00, record_data=b"\x33\xcd\x00\x40" + ) + response = b"\x0c\x05\x06\x0d\xfe\x00\x20\x05\x06\x33\xcd\x00\x40" handle = ReadFileRecordResponse() - handle.decode(request) - assert handle.records[0] == record + handle.decode(response) + + assert handle.records[0] == record1 + assert handle.records[1] == record2 def test_read_file_record_response_rtu_frame_size(self): """Test basic bit message encoding/decoding."""