From 7fcaf89fbec661c6539b1e85215b3e0b57516f10 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 1/9] 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 9eb6fb2f..255f580c 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 - by_mapping_record = node.pdo[0x1600] + 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'] @@ -68,7 +68,7 @@ def test_pdo_getitem(self): 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[0x1600][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]) From 890990d12345e339fb57c8c38861cb221077762f 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 2/9] 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 7721bde8df823b9526aa244db811a6a23c9769ec 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 3/9] 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 255f580c..7a8bbb4e 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 a91bc4f77c3dde824fc44184324239e267435bfb 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 4/9] 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 e36ed1e42932507fcbe384ff482fdf729d233a2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Thu, 4 Sep 2025 22:21:01 +0200 Subject: [PATCH 5/9] Re-raise original KeyError exception if mapping index lookup fails. --- canopen/pdo/base.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index b050206b..7071f8c7 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -173,9 +173,12 @@ 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: - with contextlib.suppress(KeyError): + try: return self.maps[key] - return self.maps[key + 1 - self.map_offset] + except KeyError: + with contextlib.suppress(KeyError): + return self.maps[key + 1 - self.map_offset] + raise def __iter__(self) -> Iterator[int]: return iter(self.maps) From 56f44b6fb90cca515750668e6cbe2d1d0353ebca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Thu, 4 Sep 2025 22:25:01 +0200 Subject: [PATCH 6/9] Fix docstrings to cover whole RPDO / TPDO parameter ranges. --- canopen/pdo/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/canopen/pdo/__init__.py b/canopen/pdo/__init__.py index 9729934d..b341cb6d 100644 --- a/canopen/pdo/__init__.py +++ b/canopen/pdo/__init__.py @@ -42,7 +42,7 @@ def __init__(self, node, rpdo: PdoBase, tpdo: PdoBase): class RPDO(PdoBase): """Receive PDO to transfer data from somewhere to the represented node. - Properties 0x1400 to 0x1403 | Mapping 0x1600 to 0x1603. + Properties 0x1400 to 0x15FF | Mapping 0x1600 to 0x17FF. :param object node: Parent node for this object. """ @@ -67,7 +67,7 @@ def stop(self): class TPDO(PdoBase): """Transmit PDO to broadcast data from the represented node to the network. - Properties 0x1800 to 0x1803 | Mapping 0x1A00 to 0x1A03. + Properties 0x1800 to 0x19FF | Mapping 0x1A00 to 0x1BFF. :param object node: Parent node for this object. """ From 6bd6f9e57b7d799110a34bc68610d085cf7bb6f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Thu, 4 Sep 2025 23:46:07 +0200 Subject: [PATCH 7/9] Support RPDO / TPDO lookup by communication parameter record index. This does not work for the generic, joined accessor class PDO yet. --- canopen/pdo/base.py | 5 +++-- test/test_pdo.py | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 7071f8c7..55781fd7 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -46,8 +46,7 @@ def __getitem__(self, key: Union[int, str]): 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) - or 0x1A00 <= key <= 0x1BFF # By TPDO ID (512) + or 0x1600 <= key <= 0x1BFF # By RPDO / TPDO mapping or communication record ): return self.map[key] for pdo_map in self.map.values(): @@ -178,6 +177,8 @@ def __getitem__(self, key: int) -> PdoMap: except KeyError: with contextlib.suppress(KeyError): return self.maps[key + 1 - self.map_offset] + with contextlib.suppress(KeyError): + return self.maps[key + 1 - self.com_offset] raise def __iter__(self) -> Iterator[int]: diff --git a/test/test_pdo.py b/test/test_pdo.py index 7a8bbb4e..f3d89750 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -54,6 +54,7 @@ def test_pdo_getitem(self): 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) + self.assertIs(node.tpdo[0x1800], 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 0846a18c068b3ae3181c6489d05dc2ada0d64733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Thu, 4 Sep 2025 23:48:24 +0200 Subject: [PATCH 8/9] Support PDO lookup by communication parameter record index. Store each PDO under two different index numbers in the dummy PdoMaps object's "maps" attribute. That would duplicate them in sequential access while iterating / counting though, so override the PdoBase dunder methods to explicitly chain just the two sub-sequences rx and tx. --- canopen/pdo/__init__.py | 10 ++++++++++ test/test_pdo.py | 1 + 2 files changed, 11 insertions(+) diff --git a/canopen/pdo/__init__.py b/canopen/pdo/__init__.py index b341cb6d..b297fe6b 100644 --- a/canopen/pdo/__init__.py +++ b/canopen/pdo/__init__.py @@ -1,4 +1,6 @@ +import itertools import logging +from collections.abc import Iterator from canopen import node from canopen.pdo.base import PdoBase, PdoMap, PdoMaps, PdoVariable @@ -35,8 +37,16 @@ def __init__(self, node, rpdo: PdoBase, tpdo: PdoBase): # The object 0x1A00 equals to key '1' so we remove 1 from the key for key, value in self.rx.items(): self.map.maps[self.rx.map_offset + (key - 1)] = value + self.map.maps[self.rx.com_offset + (key - 1)] = value for key, value in self.tx.items(): self.map.maps[self.tx.map_offset + (key - 1)] = value + self.map.maps[self.tx.com_offset + (key - 1)] = value + + def __iter__(self) -> Iterator[int]: + return itertools.chain(self.rx, self.tx) + + def __len__(self) -> int: + return len(self.rx) + len(self.tx) class RPDO(PdoBase): diff --git a/test/test_pdo.py b/test/test_pdo.py index f3d89750..d6cb0281 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -55,6 +55,7 @@ def test_pdo_getitem(self): self.assertEqual(by_mapping_record['INTEGER16 value'].raw, -3) self.assertIs(node.tpdo[0x1A00], by_mapping_record) self.assertIs(node.tpdo[0x1800], by_mapping_record) + self.assertIs(node.pdo[0x1800], 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 7e67d904577c02128e857d7aeda3857014842004 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Fri, 5 Sep 2025 01:04:07 +0200 Subject: [PATCH 9/9] Fix returned numbers in PDO.__iter__(). The base iterators of rx and tx return a 1-based sequence, while the wrapping PDO class allows only mapping or communication record index numbers to identify a PdoMap object. Return the mapping record's index from the iterator to avoid ambiguity. Add tests to verify the returned objects are actually the RPDO and TPDO members (in this order), with strictly increasing index numbers. --- canopen/pdo/__init__.py | 5 ++++- test/test_pdo.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/canopen/pdo/__init__.py b/canopen/pdo/__init__.py index b297fe6b..b749dfee 100644 --- a/canopen/pdo/__init__.py +++ b/canopen/pdo/__init__.py @@ -43,7 +43,10 @@ def __init__(self, node, rpdo: PdoBase, tpdo: PdoBase): self.map.maps[self.tx.com_offset + (key - 1)] = value def __iter__(self) -> Iterator[int]: - return itertools.chain(self.rx, self.tx) + return itertools.chain( + (self.rx.map_offset + i - 1 for i in self.rx), + (self.tx.map_offset + i - 1 for i in self.tx), + ) def __len__(self) -> int: return len(self.rx) + len(self.tx) diff --git a/test/test_pdo.py b/test/test_pdo.py index d6cb0281..50d218de 100644 --- a/test/test_pdo.py +++ b/test/test_pdo.py @@ -79,6 +79,20 @@ def test_pdo_getitem(self): self.assertRaises(KeyError, lambda: node.pdo[0x1BFF]) self.assertRaises(KeyError, lambda: node.tpdo[0x1BFF]) + def test_pdo_iterate(self): + node = self.node + pdo_iter = iter(node.pdo.items()) + prev = 0 # To check strictly increasing record index number + for rpdo, (index, pdo) in zip(node.rpdo.values(), pdo_iter): + self.assertIs(rpdo, pdo) + self.assertGreater(index, prev) + prev = index + # Continue consuming from pdo_iter + for tpdo, (index, pdo) in zip(node.tpdo.values(), pdo_iter): + self.assertIs(tpdo, pdo) + self.assertGreater(index, prev) + prev = index + def test_pdo_maps_iterate(self): node = self.node self.assertEqual(len(node.pdo), sum(1 for _ in node.pdo))