Gracefully handle readonly entries in PdoMap.save() (fixes #541) (#593)

* Skip saving and log read-only PDO parameters.

For PDO communication or mapping parameters which are read-only on a
node, the PdoMap.save() method currently aborts with an SDO exception.
However, individual parameters may simply not support configuration
changes.  Rely on the Object Dictionary to tell which ones are
expected to fail and skip them.  For communication record entries, log
what is not being saved.

* Switch to a LocalNode in TestPDO to not require a network.

* Test PdoMap.save() skipping readonly entries.

Co-authored-by: Erlend E. Aasland <[email protected]>

Author: acolomb

canopen/pdo/base.py

@@ -403,19 +403,27 @@ def save(self) -> None:
             logger.info("Skip saving %s: COB-ID was never set", self.com_record.od.name)
             return
         logger.info("Setting COB-ID 0x%X and temporarily disabling PDO", self.cob_id)
-        self.com_record[1].raw = self.cob_id | PDO_NOT_VALID | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
-        if self.trans_type is not None:
-            logger.info("Setting transmission type to %d", self.trans_type)
-            self.com_record[2].raw = self.trans_type
-        if self.inhibit_time is not None:
-            logger.info("Setting inhibit time to %d us", (self.inhibit_time * 100))
-            self.com_record[3].raw = self.inhibit_time
-        if self.event_timer is not None:
-            logger.info("Setting event timer to %d ms", self.event_timer)
-            self.com_record[5].raw = self.event_timer
-        if self.sync_start_value is not None:
-            logger.info("Setting SYNC start value to %d", self.sync_start_value)
-            self.com_record[6].raw = self.sync_start_value
+        self.com_record[1].raw = (
+            self.cob_id
+            | PDO_NOT_VALID
+            | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0)
+        )
+
+        def _set_com_record(
+            subindex: int, value: Optional[int], log_fmt: str, log_factor: int = 1
+        ):
+            if value is None:
+                return
+            if self.com_record[subindex].writable:
+                logger.info(f"Setting {log_fmt}", value * log_factor)
+                self.com_record[subindex].raw = value
+            else:
+                logger.info(f"Cannot set {log_fmt}, not writable", value * log_factor)
+
+        _set_com_record(2, self.trans_type, "transmission type to %d")
+        _set_com_record(3, self.inhibit_time, "inhibit time to %d us", 100)
+        _set_com_record(5, self.event_timer, "event timer to %d ms")
+        _set_com_record(6, self.sync_start_value, "SYNC start value to %d")
 
         try:
             self.map_array[0].raw = 0
@@ -425,20 +433,21 @@ def save(self) -> None:
             # mappings for an invalid object 0x0000:00 to overwrite any
             # excess entries with all-zeros.
             self._fill_map(self.map_array[0].raw)
-        subindex = 1
-        for var in self.map:
-            logger.info("Writing %s (0x%04X:%02X, %d bits) to PDO map",
-                        var.name, var.index, var.subindex, var.length)
+        for var, entry in zip(self.map, self.map_array.values()):
+            if not entry.od.writable:
+                continue
+            logger.info(
+                "Writing %s (0x%04X:%02X, %d bits) to PDO map",
+                var.name,
+                var.index,
+                var.subindex,
+                var.length,
+            )
             if getattr(self.pdo_node.node, "curtis_hack", False):
                 # Curtis HACK: mixed up field order
-                self.map_array[subindex].raw = (var.index |
-                                                var.subindex << 16 |
-                                                var.length << 24)
+                entry.raw = var.index | var.subindex << 16 | var.length << 24
             else:
-                self.map_array[subindex].raw = (var.index << 16 |
-                                                var.subindex << 8 |
-                                                var.length)
-            subindex += 1
+                entry.raw = var.index << 16 | var.subindex << 8 | var.length
         try:
             self.map_array[0].raw = len(self.map)
         except SdoAbortedError as e:

test/test_pdo.py

@@ -7,7 +7,7 @@
 
 class TestPDO(unittest.TestCase):
     def setUp(self):
-        node = canopen.Node(1, SAMPLE_EDS)
+        node = canopen.LocalNode(1, SAMPLE_EDS)
         pdo = node.pdo.tx[1]
         pdo.add_variable('INTEGER16 value')  # 0x2001
         pdo.add_variable('UNSIGNED8 value', length=4)  # 0x2002
@@ -64,6 +64,19 @@ def test_pdo_save(self):
         self.node.tpdo.save()
         self.node.rpdo.save()
 
+    def test_pdo_save_skip_readonly(self):
+        """Expect no exception when a record entry is not writable."""
+        # Saving only happens with a defined COB ID and for specified parameters
+        self.node.tpdo[1].cob_id = self.node.tpdo[1].predefined_cob_id
+        self.node.tpdo[1].trans_type = 1
+        self.node.tpdo[1].map_array[1].od.access_type = "r"
+        self.node.tpdo[1].save()
+
+        self.node.tpdo[2].cob_id = self.node.tpdo[2].predefined_cob_id
+        self.node.tpdo[2].trans_type = 1
+        self.node.tpdo[2].com_record[2].od.access_type = "r"
+        self.node.tpdo[2].save()
+
     def test_pdo_export(self):
         try:
             import canmatrix