From 6a33115f4749fe45adcc90509e6e7ecaef60335a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 4 Jun 2024 15:10:37 +0200 Subject: [PATCH 1/5] Add test case for non-byte-aligned PDO mappings. Use some more exotic variations of mappings which occupy bytes only partially, even spanning several PDO bytes with unaligned start and end bits. Introduce test objects for UNSIGNED16 and UNSIGNED32 to use as additional dummies in the test. Half of the tested objects are signed to see any handling problems there. The test verifies the PDO byte representation after setting dummy values and the round-trip from decoding these bytes again. --- test/sample.eds | 14 ++++++++++++++ test/test_pdo.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/test/sample.eds b/test/sample.eds index 16d0c31a..da341483 100644 --- a/test/sample.eds +++ b/test/sample.eds @@ -838,6 +838,20 @@ DataType=0x0001 AccessType=rw PDOMapping=1 +[2007] +ParameterName=UNSIGNED16 value +ObjectType=0x7 +DataType=0x0006 +AccessType=rw +PDOMapping=1 + +[2008] +ParameterName=UNSIGNED32 value +ObjectType=0x7 +DataType=0x0007 +AccessType=rw +PDOMapping=1 + [2020] ParameterName=Complex data type ObjectType=0x7 diff --git a/test/test_pdo.py b/test/test_pdo.py index 6bba18fe..77f17b5e 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -55,6 +55,35 @@ def test_bit_mapping(self): self.assertEqual(node.tpdo[0x2002].raw, 0xf) self.assertEqual(node.pdo[0x1600][0x2002].raw, 0xf) + def test_bit_offsets(self): + node = canopen.Node(1, EDS_PATH) + pdo = node.pdo.tx[1] + pdo.add_variable('UNSIGNED8 value', length=4) # byte-aligned, partial byte length + pdo.add_variable('INTEGER8 value') # non-byte-aligned, one whole byte length + pdo.add_variable('UNSIGNED32 value', length=24) # non-aligned, partial last byte + pdo.add_variable('UNSIGNED16 value', length=12) # non-aligned, whole last byte + pdo.add_variable('INTEGER16 value', length=3) # byte-aligned, partial byte length + pdo.add_variable('INTEGER32 value', length=13) # non-aligned, whole last byte + + # Write some values + pdo['UNSIGNED8 value'].raw = 3 + pdo['INTEGER8 value'].raw = -2 + pdo['UNSIGNED32 value'].raw = 0x987654 + pdo['UNSIGNED16 value'].raw = 0x321 + pdo['INTEGER16 value'].raw = -1 + pdo['INTEGER32 value'].raw = -1071 + + # Check expected data + self.assertEqual(pdo.data, b'\xe3\x4f\x65\x87\x19\x32\x8f\xde') + + # Read values from data + self.assertEqual(pdo['UNSIGNED8 value'].raw, 3) + self.assertEqual(pdo['INTEGER8 value'].raw, -2) + self.assertEqual(pdo['UNSIGNED32 value'].raw, 0x987654) + self.assertEqual(pdo['UNSIGNED16 value'].raw, 0x321) + self.assertEqual(pdo['INTEGER16 value'].raw, -1) + self.assertEqual(pdo['INTEGER32 value'].raw, -1071) + if __name__ == "__main__": unittest.main() From 2cdc79de01d4f1d3bbedaff63e8e8916a2204f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Sun, 26 May 2024 14:24:04 +0200 Subject: [PATCH 2/5] Use configured length instead of OD value for PdoVariable. This was already respected for the non-byte-aligned mapping conditional. --- canopen/pdo/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 7312f660..a57f2a35 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -569,7 +569,7 @@ def get_data(self) -> bytes: data = data | (~((1 << self.length) - 1)) data = od_struct.pack(data) else: - data = self.pdo_parent.data[byte_offset:byte_offset + len(self.od) // 8] + data = self.pdo_parent.data[byte_offset:byte_offset + self.length // 8] return data From 918a4f2c5f074efc30b0c4bc6523bfa427550a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 4 Jun 2024 07:23:37 +0200 Subject: [PATCH 3/5] Simplify rounding up, without floating point or math library. --- canopen/pdo/base.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index a57f2a35..f3ff25fc 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -1,5 +1,4 @@ import threading -import math from typing import Callable, Dict, Iterable, List, Optional, Union from collections.abc import Mapping import logging @@ -254,7 +253,7 @@ def _fill_map(self, needed): self.map.append(var) def _update_data_size(self): - self.data = bytearray(int(math.ceil(self.length / 8.0))) + self.data = bytearray(-(-self.length // 8)) # rounds up @property def name(self) -> str: From d8b9269f599fb3cdc7dd6395c5f577c489f98ebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 4 Jun 2024 13:52:43 +0200 Subject: [PATCH 4/5] Rework unaligned data access in PdoVariable.get_data(). For an unaligned variable mapping, both the first and the last containing byte can be only partially occupied by the variable data. Thus unpacking these bytes using the variable's original Struct format can result in wrong values. And the current code would even disregard the partial bits in the final byte. Instead, calculate the last occupied byte position by rounding up the last bit index divided by 8. Then use a generic int variable for the bit shifts and masking operations. The OD variable Struct description is only needed for checking the signedness anymore. --- canopen/pdo/base.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index f3ff25fc..05817fcb 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -552,6 +552,7 @@ def get_data(self) -> bytes: """ byte_offset, bit_offset = divmod(self.offset, 8) + byte_last = -(-(self.offset + self.length) // 8) # rounds up if bit_offset or self.length % 8: # Need information of the current variable type (unsigned vs signed) data_type = self.od.data_type @@ -559,16 +560,21 @@ def get_data(self) -> bytes: # A boolean type needs to be treated as an U08 data_type = objectdictionary.UNSIGNED8 od_struct = self.od.STRUCT_TYPES[data_type] - data = od_struct.unpack_from(self.pdo_parent.data, byte_offset)[0] + # Mask of relevant bits for this mapping, starting at bit 0 + mask = (1 << self.length) - 1 + # Extract all needed bytes and convert to a number for bit operations + cur_msg_data = self.pdo_parent.data[byte_offset:byte_last] + cur_msg_bits = int.from_bytes(cur_msg_data, byteorder="little") # Shift and mask to get the correct values - data = (data >> bit_offset) & ((1 << self.length) - 1) + data_bits = (cur_msg_bits >> bit_offset) & mask + # Check if the variable is signed and if the data is negative prepend signedness - if od_struct.format.islower() and (1 << (self.length - 1)) < data: + if od_struct.format.islower() and (1 << (self.length - 1)) < data_bits: # fill up the rest of the bits to get the correct signedness - data = data | (~((1 << self.length) - 1)) - data = od_struct.pack(data) + data_bits |= ~mask + data = od_struct.pack(data_bits) else: - data = self.pdo_parent.data[byte_offset:byte_offset + self.length // 8] + data = self.pdo_parent.data[byte_offset:byte_last] return data From 8bf4d121e4329789353389cceff71a2b8316321f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 4 Jun 2024 14:19:57 +0200 Subject: [PATCH 5/5] Rework unaligned data access in PdoVariable.set_data(). For an unaligned variable mapping, both the first and the last containing byte can be only partially occupied by the variable data. Thus unpacking these bytes using the variable's original Struct format can result in wrong values. And the current code would even disregard the partial bits in the final byte, or simply fail on the wrong data size. Instead, calculate the last occupied byte position by rounding up the last bit index divided by 8. Then use generic int variables for the bit shifts and masking operations on both the surrounding, unaffected data bits, and the new variable data. The OD variable Struct description is not needed at all anymore. Also use the calculated final affected byte position in the byte-aligned case, instead truncating the passed in data if too long for the mapping. This would previously overwrite more bytes than actually mapped, when given a too long data buffer. --- canopen/pdo/base.py | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 05817fcb..2b3a8303 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -587,27 +587,24 @@ def set_data(self, data: bytes): logger.debug("Updating %s to %s in %s", self.name, binascii.hexlify(data), self.pdo_parent.name) + byte_last = -(-(self.offset + self.length) // 8) # rounds up if bit_offset or self.length % 8: - cur_msg_data = self.pdo_parent.data[byte_offset:byte_offset + len(self.od) // 8] - # Need information of the current variable type (unsigned vs signed) - data_type = self.od.data_type - if data_type == objectdictionary.BOOLEAN: - # A boolean type needs to be treated as an U08 - data_type = objectdictionary.UNSIGNED8 - od_struct = self.od.STRUCT_TYPES[data_type] - cur_msg_data = od_struct.unpack(cur_msg_data)[0] - # data has to have the same size as old_data - data = od_struct.unpack(data)[0] + # Mask of relevant bits for this mapping, starting at bit 0 + mask = (1 << self.length) - 1 + # Extract all needed bytes and convert to a number for bit operations + cur_msg_data = self.pdo_parent.data[byte_offset:byte_last] + cur_msg_bits = int.from_bytes(cur_msg_data, byteorder="little") # Mask out the old data value - # At the end we need to mask for correct variable length (bitwise operation failure) - shifted = (((1 << self.length) - 1) << bit_offset) & ((1 << len(self.od)) - 1) - bitwise_not = (~shifted) & ((1 << len(self.od)) - 1) - cur_msg_data = cur_msg_data & bitwise_not + cur_msg_bits &= ~(mask << bit_offset) + # Convert new value to a number for generic bit operations + data_bits = int.from_bytes(data, byteorder="little") & mask # Set the new data on the correct position - data = (data << bit_offset) | cur_msg_data - data = od_struct.pack_into(self.pdo_parent.data, byte_offset, data) + data_bits = (data_bits << bit_offset) | cur_msg_bits + + merged_data = data_bits.to_bytes(byte_last - byte_offset, byteorder="little") + self.pdo_parent.data[byte_offset:byte_last] = merged_data else: - self.pdo_parent.data[byte_offset:byte_offset + len(data)] = data + self.pdo_parent.data[byte_offset:byte_last] = data[0:byte_last] self.pdo_parent.update()