From 26d394ac4708580e298eba3a305596e0393449cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 15 Jul 2025 10:15:31 +0200 Subject: [PATCH 01/11] Refactor. --- canopen/pdo/base.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 7bef6e62..58d5d3b7 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -40,17 +40,19 @@ def __iter__(self): return iter(self.map) def __getitem__(self, key): - if isinstance(key, int) and (0x1A00 <= key <= 0x1BFF or # By TPDO ID (512) - 0x1600 <= key <= 0x17FF or # By RPDO ID (512) - 0 < key <= 512): # By PDO Index - return self.map[key] - else: - for pdo_map in self.map.values(): - try: - return pdo_map[key] - except KeyError: - # ignore if one specific PDO does not have the key and try the next one - continue + if isinstance(key, int): + if ( + 0 < key <= 512 # By PDO Index + or 0x1600 <= key <= 0x17FF # By RPDO ID (512) + or 0x1A00 <= key <= 0x1BFF # By TPDO ID (512) + ): + return self.map[key] + for pdo_map in self.map.values(): + try: + return pdo_map[key] + except KeyError: + # ignore if one specific PDO does not have the key and try the next one + continue raise KeyError(f"PDO: {key} was not found in any map") def __len__(self): From f2263daf19f60de78872dceb7c9f3b87661b0420 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 15 Jul 2025 10:15:38 +0200 Subject: [PATCH 02/11] Raise KeyError for zero index. --- canopen/pdo/base.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 58d5d3b7..79ff2cf8 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -41,6 +41,8 @@ def __iter__(self): def __getitem__(self, key): if isinstance(key, int): + if key == 0: + raise KeyError("PDO index zero requested for 1-based sequence") if ( 0 < key <= 512 # By PDO Index or 0x1600 <= key <= 0x17FF # By RPDO ID (512) From 0c7606f1656749aec6fec521b2e9c1e0671b43ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 15 Jul 2025 10:16:02 +0200 Subject: [PATCH 03/11] Add annotation. --- 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 79ff2cf8..c4104143 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -39,7 +39,7 @@ def __init__(self, node: Union[LocalNode, RemoteNode]): def __iter__(self): return iter(self.map) - def __getitem__(self, key): + def __getitem__(self, key: Union[int, str]): if isinstance(key, int): if key == 0: raise KeyError("PDO index zero requested for 1-based sequence") From 9ce5aa4b9b6d71d1c9b65443e3753380e7ad805f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 11:47:13 +0200 Subject: [PATCH 04/11] Fix mixed up mapping parameter record index numbers in class PDO. The access by this index number mixed up the two number ranges. The CiA 301 standard defines: * Object 1600h to 17FFh: RPDO mapping parameter * Object 1A00h to 1BFFh: TPDO mapping parameter The test was also wrong, because it added the two tested variables to the TPDO1, while testing the __getitem__ lookup with index 0x1600. --- canopen/pdo/__init__.py | 4 ++-- test/test_pdo.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/canopen/pdo/__init__.py b/canopen/pdo/__init__.py index 533309f8..368a39c4 100644 --- a/canopen/pdo/__init__.py +++ b/canopen/pdo/__init__.py @@ -32,9 +32,9 @@ def __init__(self, node, rpdo, tpdo): self.map = {} # the object 0x1A00 equals to key '1' so we remove 1 from the key for key, value in self.rx.items(): - self.map[0x1A00 + (key - 1)] = value - for key, value in self.tx.items(): self.map[0x1600 + (key - 1)] = value + for key, value in self.tx.items(): + self.map[0x1A00 + (key - 1)] = value class RPDO(PdoBase): diff --git a/test/test_pdo.py b/test/test_pdo.py index b8bb0599..b56c9a4a 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -50,7 +50,7 @@ def test_pdo_getitem(self): self.assertEqual(node.tpdo[1]['BOOLEAN value 2'].raw, True) # Test different types of access - self.assertEqual(node.pdo[0x1600]['INTEGER16 value'].raw, -3) + self.assertEqual(node.pdo[0x1A00]['INTEGER16 value'].raw, -3) self.assertEqual(node.pdo['INTEGER16 value'].raw, -3) self.assertEqual(node.pdo.tx[1]['INTEGER16 value'].raw, -3) self.assertEqual(node.pdo[0x2001].raw, -3) @@ -58,7 +58,7 @@ def test_pdo_getitem(self): self.assertEqual(node.pdo[0x2002].raw, 0xf) self.assertEqual(node.pdo['0x2002'].raw, 0xf) self.assertEqual(node.tpdo[0x2002].raw, 0xf) - self.assertEqual(node.pdo[0x1600][0x2002].raw, 0xf) + self.assertEqual(node.pdo[0x1A00][0x2002].raw, 0xf) def test_pdo_save(self): self.node.tpdo.save() From ce0013bd3a2839ba189be865368aba83a60ce7b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 11:56:02 +0200 Subject: [PATCH 05/11] Properly test different access type return values. Augment test_pdo_getitem() to not only check the mapped object values. What's more important is the type of object returned, and whether it is the correct object (identical to other access method results). --- test/test_pdo.py | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/test/test_pdo.py b/test/test_pdo.py index b56c9a4a..124af2a3 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -50,15 +50,25 @@ def test_pdo_getitem(self): self.assertEqual(node.tpdo[1]['BOOLEAN value 2'].raw, True) # Test different types of access - self.assertEqual(node.pdo[0x1A00]['INTEGER16 value'].raw, -3) - self.assertEqual(node.pdo['INTEGER16 value'].raw, -3) - self.assertEqual(node.pdo.tx[1]['INTEGER16 value'].raw, -3) - self.assertEqual(node.pdo[0x2001].raw, -3) - self.assertEqual(node.tpdo[0x2001].raw, -3) - self.assertEqual(node.pdo[0x2002].raw, 0xf) - self.assertEqual(node.pdo['0x2002'].raw, 0xf) - self.assertEqual(node.tpdo[0x2002].raw, 0xf) - self.assertEqual(node.pdo[0x1A00][0x2002].raw, 0xf) + by_mapping_record = node.pdo[0x1A00] + self.assertIsInstance(by_mapping_record, canopen.pdo.PdoMap) + self.assertEqual(by_mapping_record['INTEGER16 value'].raw, -3) + by_object_name = node.pdo['INTEGER16 value'] + self.assertIsInstance(by_object_name, canopen.pdo.PdoVariable) + self.assertIs(by_object_name.od, node.object_dictionary['INTEGER16 value']) + self.assertEqual(by_object_name.raw, -3) + by_pdo_index = node.pdo.tx[1] + self.assertIs(by_pdo_index, by_mapping_record) + by_object_index = node.pdo[0x2001] + self.assertIsInstance(by_object_index, canopen.pdo.PdoVariable) + self.assertIs(by_object_index, by_object_name) + by_object_index_tpdo = node.tpdo[0x2001] + self.assertIs(by_object_index_tpdo, by_object_name) + by_object_index = node.pdo[0x2002] + self.assertEqual(by_object_index.raw, 0xf) + self.assertIs(node.pdo['0x2002'], by_object_index) + self.assertIs(node.tpdo[0x2002], by_object_index) + self.assertIs(node.pdo[0x1A00][0x2002], by_object_index) def test_pdo_save(self): self.node.tpdo.save() From ed4a4afc9a70c9d31d7a019b03eacad6c6f4e158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 12:28:40 +0200 Subject: [PATCH 06/11] Store parameter record index offsets for each PdoMaps collection. --- canopen/pdo/base.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 216fc550..d6c03921 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -144,10 +144,10 @@ def stop(self): pdo_map.stop() -class PdoMaps(Mapping): +class PdoMaps(Mapping[int, 'PdoMap']): """A collection of transmit or receive maps.""" - def __init__(self, com_offset, map_offset, pdo_node: PdoBase, cob_base=None): + def __init__(self, com_offset: int, map_offset: int, pdo_node: PdoBase, cob_base=None): """ :param com_offset: :param map_offset: @@ -155,6 +155,8 @@ def __init__(self, com_offset, map_offset, pdo_node: PdoBase, cob_base=None): :param cob_base: """ self.maps: dict[int, PdoMap] = {} + self.com_offset = com_offset + self.map_offset = map_offset for map_no in range(512): if com_offset + map_no in pdo_node.node.object_dictionary: new_map = PdoMap( From d4d7fe251b745dca9700a746b08d0acee6675842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 12:30:42 +0200 Subject: [PATCH 07/11] Support lookup by mapping record index in TPDO and RPDO classes. These two PdoBase-derived classes only supported numeric access based on the sequential index, not with the mapping parameter record index like the PDO class. Implement a fallback to check that if the regular index lookup fails. --- canopen/pdo/base.py | 5 ++++- test/test_pdo.py | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index d6c03921..543bc670 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -1,6 +1,7 @@ from __future__ import annotations import binascii +import contextlib import logging import math import threading @@ -169,7 +170,9 @@ def __init__(self, com_offset: int, map_offset: int, pdo_node: PdoBase, cob_base self.maps[map_no + 1] = new_map def __getitem__(self, key: int) -> PdoMap: - return self.maps[key] + with contextlib.suppress(KeyError): + return self.maps[key] + return self.maps[key + 1 - self.map_offset] def __iter__(self) -> Iterator[int]: return iter(self.maps) diff --git a/test/test_pdo.py b/test/test_pdo.py index 124af2a3..c9c6d97e 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -53,6 +53,7 @@ def test_pdo_getitem(self): by_mapping_record = node.pdo[0x1A00] self.assertIsInstance(by_mapping_record, canopen.pdo.PdoMap) self.assertEqual(by_mapping_record['INTEGER16 value'].raw, -3) + self.assertIs(node.tpdo[0x1A00], by_mapping_record) by_object_name = node.pdo['INTEGER16 value'] self.assertIsInstance(by_object_name, canopen.pdo.PdoVariable) self.assertIs(by_object_name.od, node.object_dictionary['INTEGER16 value']) From 190984672b2de07a2fd9df0ba4bbef74e7c5587e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 12:40:24 +0200 Subject: [PATCH 08/11] Replace mapping in class PDO with proper PdoMaps object. The mechanisms to lookup objects, implemented in class PdoMaps, did not apply to the PDO class itself, because it simply used a dictionary instead of a PdoMaps object. That also violates the static typing rules. Make the PdoBase.maps attribute mandatory and accept only type PdoMaps. To allow a basically "empty" PdoMaps object, adjust its constructor to skip adding entries when neither offset parameter is given as non-zero. Instead, access the PdoMaps.maps attribute directly to inject the offset-based TX and RX PdoMap objects in the PDO class constructor. Add some explanation why relative indices cannot be used here. --- canopen/pdo/__init__.py | 12 +++++++----- canopen/pdo/base.py | 5 ++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/canopen/pdo/__init__.py b/canopen/pdo/__init__.py index 368a39c4..9729934d 100644 --- a/canopen/pdo/__init__.py +++ b/canopen/pdo/__init__.py @@ -24,17 +24,19 @@ class PDO(PdoBase): :param tpdo: TPDO object holding the Transmit PDO mappings """ - def __init__(self, node, rpdo, tpdo): + def __init__(self, node, rpdo: PdoBase, tpdo: PdoBase): super(PDO, self).__init__(node) self.rx = rpdo.map self.tx = tpdo.map - self.map = {} - # the object 0x1A00 equals to key '1' so we remove 1 from the key + self.map = PdoMaps(0, 0, self) + # Combine RX and TX entries, but only via mapping parameter index. Relative index + # numbers would be ambiguous. + # The object 0x1A00 equals to key '1' so we remove 1 from the key for key, value in self.rx.items(): - self.map[0x1600 + (key - 1)] = value + self.map.maps[self.rx.map_offset + (key - 1)] = value for key, value in self.tx.items(): - self.map[0x1A00 + (key - 1)] = value + self.map.maps[self.tx.map_offset + (key - 1)] = value class RPDO(PdoBase): diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 543bc670..b050206b 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -34,7 +34,7 @@ class PdoBase(Mapping): def __init__(self, node: Union[LocalNode, RemoteNode]): self.network: canopen.network.Network = canopen.network._UNINITIALIZED_NETWORK - self.map: Optional[PdoMaps] = None + self.map: PdoMaps # must initialize in derived classes self.node: Union[LocalNode, RemoteNode] = node def __iter__(self): @@ -158,6 +158,9 @@ def __init__(self, com_offset: int, map_offset: int, pdo_node: PdoBase, cob_base self.maps: dict[int, PdoMap] = {} self.com_offset = com_offset self.map_offset = map_offset + if not com_offset and not map_offset: + # Skip generating entries without parameter index offsets + return for map_no in range(512): if com_offset + map_no in pdo_node.node.object_dictionary: new_map = PdoMap( From c5fc174383c3177552e17fa9aaa90c09e64eb13b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 12:58:57 +0200 Subject: [PATCH 09/11] Extend tests to cover KeyError exceptions. --- test/test_pdo.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test_pdo.py b/test/test_pdo.py index c9c6d97e..3d45de7c 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -71,6 +71,10 @@ def test_pdo_getitem(self): self.assertIs(node.tpdo[0x2002], by_object_index) self.assertIs(node.pdo[0x1A00][0x2002], by_object_index) + self.assertRaises(KeyError, lambda: node.pdo[0]) + self.assertRaises(KeyError, lambda: node.tpdo[0]) + self.assertRaises(KeyError, lambda: node.pdo['DOES NOT EXIST']) + def test_pdo_save(self): self.node.tpdo.save() self.node.rpdo.save() From 3c1de2c06e2c6b518b2dc7721c13f1950ea00253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 13:03:58 +0200 Subject: [PATCH 10/11] Extend tests to cover iteration over PdoMaps. --- test/test_pdo.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/test_pdo.py b/test/test_pdo.py index 3d45de7c..b610afdf 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -75,6 +75,12 @@ def test_pdo_getitem(self): self.assertRaises(KeyError, lambda: node.tpdo[0]) self.assertRaises(KeyError, lambda: node.pdo['DOES NOT EXIST']) + def test_pdo_maps_iterate(self): + node = self.node + self.assertEqual(len(node.pdo), sum(1 for _ in node.pdo)) + self.assertEqual(len(node.tpdo), sum(1 for _ in node.tpdo)) + self.assertEqual(len(node.rpdo), sum(1 for _ in node.rpdo)) + def test_pdo_save(self): self.node.tpdo.save() self.node.rpdo.save() From 5a31541ca8b5ba99b4f3396c9b332435e8d2d40e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 19 Aug 2025 13:07:11 +0200 Subject: [PATCH 11/11] Extend tests to cover iteration over PdoMap. --- test/test_pdo.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/test_pdo.py b/test/test_pdo.py index b610afdf..f07406a4 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -81,6 +81,9 @@ def test_pdo_maps_iterate(self): self.assertEqual(len(node.tpdo), sum(1 for _ in node.tpdo)) self.assertEqual(len(node.rpdo), sum(1 for _ in node.rpdo)) + pdo = node.tpdo[1] + self.assertEqual(len(pdo), sum(1 for _ in pdo)) + def test_pdo_save(self): self.node.tpdo.save() self.node.rpdo.save()