Fix and extend PDO selection by record IDs (fixes #607) (#613)

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.

Support lookup by mapping record and communication parameter record
index in TPDO and RPDO classes, by storing record index offsets for
each PdoMaps collection.  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 those if the regular index lookup fails.  Fix
docstrings to cover whole RPDO / TPDO parameter index ranges.

Support lookup by communication parameter record index in generic PDO
class, by storing 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.  This class now uses a proper PdoMaps proxy object as well,
instead of overwriting it with a custom dictionary.  To allow this
dummy usage, adjust its constructor to skip generating entries.  Add
some explanation why relative indices cannot be used here.

Author: acolomb

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
@@ -24,23 +26,36 @@ 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[0x1A00 + (key - 1)] = value
+            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[0x1600 + (key - 1)] = value
+            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.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)
 
 
 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.
     """
 
@@ -65,7 +80,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.
     """
 

canopen/pdo/base.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import binascii
+import contextlib
 import logging
 import math
 import threading
@@ -33,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):
@@ -45,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():
@@ -144,17 +144,22 @@ 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:
         :param pdo_node:
         :param 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(
@@ -167,7 +172,14 @@ def __init__(self, com_offset, map_offset, pdo_node: PdoBase, cob_base=None):
                 self.maps[map_no + 1] = new_map
 
     def __getitem__(self, key: int) -> PdoMap:
-        return self.maps[key]
+        try:
+            return self.maps[key]
+        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]:
         return iter(self.maps)

test/test_pdo.py

@@ -50,9 +50,12 @@ 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)
+        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'])
@@ -68,14 +71,28 @@ 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])
         self.assertRaises(KeyError, lambda: node.pdo['DOES NOT EXIST'])
         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))