From 66d0bb286150498cc503f7466b2618b46c458f57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 15 Jun 2025 18:09:53 +0200 Subject: [PATCH 1/9] Avoid truncating unknown types in SDO upload. When an OD entry is found in SdoClient.upload(), the truncation is currently skipped only for those types explicitly listed as variable length data (domain, strings). That causes unknown types to be truncated to whatever their length indicates, which is usually one byte. Invert the condition to check all types with an explicitly known size, a.k.a. those listed in STRUCT_TYPES where the required length can be deduced from the structure format. Re-enable the unit tests which were skipped based on the previously buggy behavior. --- canopen/objectdictionary/__init__.py | 6 ++++++ canopen/sdo/client.py | 4 +--- test/test_sdo.py | 2 -- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/canopen/objectdictionary/__init__.py b/canopen/objectdictionary/__init__.py index 09fe6e03..c67b3b4a 100644 --- a/canopen/objectdictionary/__init__.py +++ b/canopen/objectdictionary/__init__.py @@ -419,6 +419,12 @@ def add_bit_definition(self, name: str, bits: List[int]) -> None: """ self.bit_definitions[name] = bits + @property + def fixed_size(self) -> bool: + """Indicate whether the amount of needed data is known in advance.""" + # Only for types which we parse using a structure. + return self.data_type in self.STRUCT_TYPES + def decode_raw(self, data: bytes) -> Union[int, float, str, bytes, bytearray]: if self.data_type == VISIBLE_STRING: # Strip any trailing NUL characters from C-based systems diff --git a/canopen/sdo/client.py b/canopen/sdo/client.py index 6d92588e..fd5fd99b 100644 --- a/canopen/sdo/client.py +++ b/canopen/sdo/client.py @@ -126,9 +126,7 @@ def upload(self, index: int, subindex: int) -> bytes: var = self.od.get_variable(index, subindex) if var is not None: # Found a matching variable in OD - # If this is a data type (string, domain etc) the size is - # unknown anyway so keep the data as is - if var.data_type not in objectdictionary.DATA_TYPES: + if var.fixed_size: # Get the size in bytes for this variable var_size = len(var) // 8 if response_size is None or var_size < response_size: diff --git a/test/test_sdo.py b/test/test_sdo.py index fea52b6a..78012a30 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -814,7 +814,6 @@ def test_unknown_od_112(self): def test_unknown_datatype32(self): """Test an unknown datatype, but known OD, of 32 bits (4 bytes).""" - return # FIXME: Disabled temporarily until datatype conditionals are fixed, see #436 # Add fake entry 0x2100 to OD, using fake datatype 0xFF if 0x2100 not in self.node.object_dictionary: fake_var = ODVariable("Fake", 0x2100) @@ -829,7 +828,6 @@ def test_unknown_datatype32(self): def test_unknown_datatype112(self): """Test an unknown datatype, but known OD, of 112 bits (14 bytes).""" - return # FIXME: Disabled temporarily until datatype conditionals are fixed, see #436 # Add fake entry 0x2100 to OD, using fake datatype 0xFF if 0x2100 not in self.node.object_dictionary: fake_var = ODVariable("Fake", 0x2100) From 28c9ed8b6dd7ab3664ccfbf17c324111fc3837f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 15 Jun 2025 19:02:00 +0200 Subject: [PATCH 2/9] Disregard OD variable description in SdoClient.upload(). The upload method should behave as a raw producer of byte data. Interpretation of the returned data based on the Object Dictionary is the responsibility of the SdoVariable class, which delegates to the generic ODVariable decoding functions. Move the data truncation to the method SdoVariable.get_data(), where access to the ODVariable is certain. The only truncation that still happens is based on the response size specified by the server, which might be smaller for e.g. expedited upload. Extend the upload() function docstring to clarify the changed behavior. Adjust the test case for expedited upload with unspecified size in the response, which is the only incompatible change. --- canopen/sdo/base.py | 13 ++++++++++++- canopen/sdo/client.py | 17 ++++++----------- test/test_sdo.py | 4 ++-- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/canopen/sdo/base.py b/canopen/sdo/base.py index ddc75ed9..9cade76d 100644 --- a/canopen/sdo/base.py +++ b/canopen/sdo/base.py @@ -148,7 +148,18 @@ def __init__(self, sdo_node: SdoBase, od: objectdictionary.ODVariable): variable.Variable.__init__(self, od) def get_data(self) -> bytes: - return self.sdo_node.upload(self.od.index, self.od.subindex) + data = self.sdo_node.upload(self.od.index, self.od.subindex) + response_size = len(data) + + # If size is available through variable in OD, then use the smaller of the two sizes. + # Some devices send U32/I32 even if variable is smaller in OD + if self.od.fixed_size: + # Get the size in bytes for this variable + var_size = len(self.od) // 8 + if response_size is None or var_size < response_size: + # Truncate the data to specified size + data = data[0:var_size] + return data def set_data(self, data: bytes): force_segment = self.od.data_type == objectdictionary.DOMAIN diff --git a/canopen/sdo/client.py b/canopen/sdo/client.py index fd5fd99b..fef8bdae 100644 --- a/canopen/sdo/client.py +++ b/canopen/sdo/client.py @@ -105,6 +105,10 @@ def abort(self, abort_code=0x08000000): def upload(self, index: int, subindex: int) -> bytes: """May be called to make a read operation without an Object Dictionary. + No validation against the Object Dictionary is performed, even if an object description + would be available. The length of the returned data depends only on the transferred + amount, possibly truncated to the size indicated by the server. + :param index: Index of object to read. :param subindex: @@ -121,17 +125,8 @@ def upload(self, index: int, subindex: int) -> bytes: response_size = fp.size data = fp.read() - # If size is available through variable in OD, then use the smaller of the two sizes. - # Some devices send U32/I32 even if variable is smaller in OD - var = self.od.get_variable(index, subindex) - if var is not None: - # Found a matching variable in OD - if var.fixed_size: - # Get the size in bytes for this variable - var_size = len(var) // 8 - if response_size is None or var_size < response_size: - # Truncate the data to specified size - data = data[0:var_size] + if response_size and response_size < len(data): + data = data[:response_size] return data def download( diff --git a/test/test_sdo.py b/test/test_sdo.py index 78012a30..58a766b1 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -102,9 +102,9 @@ def test_size_not_specified(self): (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00') ] - # Make sure the size of the data is 1 byte + # This method used to truncate to 1 byte, but returns raw content now data = self.network[2].sdo.upload(0x1400, 2) - self.assertEqual(data, b'\xfe') + self.assertEqual(data, b'\xfe\x00\x00\x00') self.assertTrue(self.message_sent) def test_expedited_download(self): From 9c517f57b2f663fc8c2f314039ed528dcd88a060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 15 Jun 2025 22:09:25 +0200 Subject: [PATCH 3/9] Simplify slice. --- canopen/sdo/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/canopen/sdo/base.py b/canopen/sdo/base.py index 9cade76d..53a093ec 100644 --- a/canopen/sdo/base.py +++ b/canopen/sdo/base.py @@ -158,7 +158,7 @@ def get_data(self) -> bytes: var_size = len(self.od) // 8 if response_size is None or var_size < response_size: # Truncate the data to specified size - data = data[0:var_size] + data = data[:var_size] return data def set_data(self, data: bytes): From bfbfa7c160225216df1390f1f14e3a5f0fcb0fed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 15 Jun 2025 22:09:35 +0200 Subject: [PATCH 4/9] Test expedited upload with 8 byte padded frame. The existing test response is actually wrong, since the standard mandates four bytes "data" regardless of the other flags. --- test/test_sdo.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/test_sdo.py b/test/test_sdo.py index 58a766b1..c46c107b 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -95,6 +95,14 @@ def test_expedited_upload(self): ] trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw self.assertEqual(trans_type, 254) + + # Same with padding to a full SDO frame + self.data = [ + (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), + (RX, b'\x4f\x00\x14\x02\xfe\x00\x00\x00') + ] + trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw + self.assertEqual(trans_type, 254) self.assertTrue(self.message_sent) def test_size_not_specified(self): From 03cee34184f0a66378d0597463048b5a763ddc47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 15 Jun 2025 22:20:59 +0200 Subject: [PATCH 5/9] Really unspecified size. --- test/test_sdo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_sdo.py b/test/test_sdo.py index c46c107b..7dceb542 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -99,7 +99,7 @@ def test_expedited_upload(self): # Same with padding to a full SDO frame self.data = [ (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), - (RX, b'\x4f\x00\x14\x02\xfe\x00\x00\x00') + (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00') ] trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw self.assertEqual(trans_type, 254) From f1fb47e707735a16f0d2bbabca0dcda6e384617b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 15 Jun 2025 22:32:15 +0200 Subject: [PATCH 6/9] Another test? --- test/test_sdo.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/test_sdo.py b/test/test_sdo.py index 7dceb542..d8e20f0f 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -97,6 +97,13 @@ def test_expedited_upload(self): self.assertEqual(trans_type, 254) # Same with padding to a full SDO frame + self.data = [ + (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), + (RX, b'\x4f\x00\x14\x02\xfe\x00\x00\x00') + ] + trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw + self.assertEqual(trans_type, 254) + self.data = [ (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00') From 2ab61c74b901250c31cfe29cc372393614062837 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Mon, 16 Jun 2025 20:08:19 +0200 Subject: [PATCH 7/9] Coverage for conditional response_size < len(data). --- test/test_sdo.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/test_sdo.py b/test/test_sdo.py index d8e20f0f..812b9490 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -97,13 +97,6 @@ def test_expedited_upload(self): self.assertEqual(trans_type, 254) # Same with padding to a full SDO frame - self.data = [ - (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), - (RX, b'\x4f\x00\x14\x02\xfe\x00\x00\x00') - ] - trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw - self.assertEqual(trans_type, 254) - self.data = [ (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00') @@ -146,6 +139,17 @@ def test_segmented_upload(self): device_name = self.network[2].sdo[0x1008].raw self.assertEqual(device_name, "Tiny Node - Mega Domains !") + def test_segmented_upload_too_much_data(self): + # Server sends 5 bytes, but indicated size 4 + self.data = [ + (TX, b'\x40\x08\x10\x00\x00\x00\x00\x00'), + (RX, b'\x41\x08\x10\x00\x04\x00\x00\x00'), + (TX, b'\x60\x00\x00\x00\x00\x00\x00\x00'), + (RX, b'\x05\x54\x69\x6E\x79\x20\x00\x00'), + ] + device_name = self.network[2].sdo[0x1008].raw + self.assertEqual(device_name, "Tiny") + def test_segmented_download(self): self.data = [ (TX, b'\x21\x00\x20\x00\x0d\x00\x00\x00'), From 3f19dd6b755edab0bd39fb53fad2d66c3aca7548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 22 Jun 2025 22:31:15 +0200 Subject: [PATCH 8/9] Add comments to test traffic. --- test/test_sdo.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test_sdo.py b/test/test_sdo.py index 812b9490..1ab4529b 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -90,16 +90,16 @@ def test_expedited_upload(self): # UNSIGNED8 without padded data part (see issue #5) self.data = [ - (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), - (RX, b'\x4f\x00\x14\x02\xfe') + (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), # SDO upload 0x1400:02 + (RX, b'\x4f\x00\x14\x02\xfe'), # expedited, size=1 ] trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw self.assertEqual(trans_type, 254) # Same with padding to a full SDO frame self.data = [ - (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), - (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00') + (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), # SDO upload 0x1400:02 + (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00'), # expedited, no size indicated ] trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw self.assertEqual(trans_type, 254) From b431e9145131ed31c82b98c9f594b2aaf0f23628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 22 Jun 2025 22:39:55 +0200 Subject: [PATCH 9/9] Add better comments to test traffic. --- test/test_sdo.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/test_sdo.py b/test/test_sdo.py index 1ab4529b..9764a6d3 100644 --- a/test/test_sdo.py +++ b/test/test_sdo.py @@ -90,7 +90,7 @@ def test_expedited_upload(self): # UNSIGNED8 without padded data part (see issue #5) self.data = [ - (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), # SDO upload 0x1400:02 + (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), # upload initiate 0x1400:02 (RX, b'\x4f\x00\x14\x02\xfe'), # expedited, size=1 ] trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw @@ -98,7 +98,7 @@ def test_expedited_upload(self): # Same with padding to a full SDO frame self.data = [ - (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), # SDO upload 0x1400:02 + (TX, b'\x40\x00\x14\x02\x00\x00\x00\x00'), # upload initiate 0x1400:02 (RX, b'\x42\x00\x14\x02\xfe\x00\x00\x00'), # expedited, no size indicated ] trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw @@ -142,10 +142,10 @@ def test_segmented_upload(self): def test_segmented_upload_too_much_data(self): # Server sends 5 bytes, but indicated size 4 self.data = [ - (TX, b'\x40\x08\x10\x00\x00\x00\x00\x00'), - (RX, b'\x41\x08\x10\x00\x04\x00\x00\x00'), - (TX, b'\x60\x00\x00\x00\x00\x00\x00\x00'), - (RX, b'\x05\x54\x69\x6E\x79\x20\x00\x00'), + (TX, b'\x40\x08\x10\x00\x00\x00\x00\x00'), # upload initiate, 0x1008:00 + (RX, b'\x41\x08\x10\x00\x04\x00\x00\x00'), # segmented, size indicated, 4 bytes + (TX, b'\x60\x00\x00\x00\x00\x00\x00\x00'), # upload segment + (RX, b'\x05\x54\x69\x6E\x79\x20\x00\x00'), # segment complete, 5 bytes ] device_name = self.network[2].sdo[0x1008].raw self.assertEqual(device_name, "Tiny")