pdo: Skip saving if no COB-ID was set (fixes #445) (#446)

The PdoBase.save() method would fail with a cryptic TypeError
exception when the COB-ID was not previously set (still at None).
Instead just skip the corresponding PDO and log an INFO level message
about it.  Add logging when re-enabling the PDO as well.

Also fix some minor issues in surrounding code:
* Added a test for PDO saving from EDS without further settings.
* Skip the always true condition "self.map is not None".  The
  attribute is initialized as an empty list.
* Simplify check for curtis_hack using getattr().
* Remove unnecessary initialization in PdoMap.__getitem__().
* Removed unused variable "data" in PdoVariable.set_data()
* Wrong type for empty argument to send_message().

Co-authored-by: Frieder Schüler <[email protected]>

canopen/pdo/base.py

@@ -215,7 +215,6 @@ def __getitem_by_name(self, value):
                        f"{', '.join(valid_values)}")
 
     def __getitem__(self, key: Union[int, str]) -> "PdoVariable":
-        var = None
         if isinstance(key, int):
             # there is a maximum available of 8 slots per PDO map
             if key in range(0, 8):
@@ -360,7 +359,8 @@ def _raw_from(param):
             subindex = (value >> 8) & 0xFF
             # Ignore the highest bit, it is never valid for <= 64 PDO length
             size = value & 0x7F
-            if hasattr(self.pdo_node.node, "curtis_hack") and self.pdo_node.node.curtis_hack:  # Curtis HACK: mixed up field order
+            if getattr(self.pdo_node.node, "curtis_hack", False):
+                # Curtis HACK: mixed up field order
                 index = value & 0xFFFF
                 subindex = (value >> 16) & 0xFF
                 size = (value >> 24) & 0x7F
@@ -371,8 +371,10 @@ def _raw_from(param):
 
     def save(self) -> None:
         """Save PDO configuration for this map using SDO."""
-        logger.info("Setting COB-ID 0x%X and temporarily disabling PDO",
-                    self.cob_id)
+        if self.cob_id is 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)
@@ -387,42 +389,44 @@ def save(self) -> None:
             logger.info("Setting SYNC start value to %d", self.sync_start_value)
             self.com_record[6].raw = self.sync_start_value
 
-        if self.map is not None:
-            try:
-                self.map_array[0].raw = 0
-            except SdoAbortedError:
-                # WORKAROUND for broken implementations: If the array has a
-                # fixed number of entries (count not writable), generate dummy
-                # 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)
-                if hasattr(self.pdo_node.node, "curtis_hack") and self.pdo_node.node.curtis_hack:  # Curtis HACK: mixed up field order
-                    self.map_array[subindex].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
-            try:
-                self.map_array[0].raw = len(self.map)
-            except SdoAbortedError as e:
-                # WORKAROUND for broken implementations: If the array
-                # number-of-entries parameter is not writable, we have already
-                # generated the required number of mappings above.
-                if e.code != 0x06010002:
-                    # Abort codes other than "Attempt to write a read-only
-                    # object" should still be reported.
-                    raise
-            self._update_data_size()
+        try:
+            self.map_array[0].raw = 0
+        except SdoAbortedError:
+            # WORKAROUND for broken implementations: If the array has a
+            # fixed number of entries (count not writable), generate dummy
+            # 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)
+            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)
+            else:
+                self.map_array[subindex].raw = (var.index << 16 |
+                                                var.subindex << 8 |
+                                                var.length)
+            subindex += 1
+        try:
+            self.map_array[0].raw = len(self.map)
+        except SdoAbortedError as e:
+            # WORKAROUND for broken implementations: If the array
+            # number-of-entries parameter is not writable, we have already
+            # generated the required number of mappings above.
+            if e.code != 0x06010002:
+                # Abort codes other than "Attempt to write a read-only
+                # object" should still be reported.
+                raise
+        self._update_data_size()
 
         if self.enabled:
-            self.com_record[1].raw = self.cob_id | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
+            cob_id = self.cob_id | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
+            logger.info("Setting COB-ID 0x%X and re-enabling PDO", cob_id)
+            self.com_record[1].raw = cob_id
             self.subscribe()
 
     def subscribe(self) -> None:
@@ -521,7 +525,7 @@ def remote_request(self) -> None:
         Silently ignore if not allowed.
         """
         if self.enabled and self.rtr_allowed:
-            self.pdo_node.network.send_message(self.cob_id, None, remote=True)
+            self.pdo_node.network.send_message(self.cob_id, bytes(), remote=True)
 
     def wait_for_reception(self, timeout: float = 10) -> float:
         """Wait for the next transmit PDO.
@@ -600,7 +604,7 @@ def set_data(self, data: bytes):
             cur_msg_data = cur_msg_data & bitwise_not
             # 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)
+            od_struct.pack_into(self.pdo_parent.data, byte_offset, data)
         else:
             self.pdo_parent.data[byte_offset:byte_offset + len(data)] = data
 

test/test_pdo.py

@@ -55,6 +55,11 @@ def test_bit_mapping(self):
         self.assertEqual(node.tpdo[0x2002].raw, 0xf)
         self.assertEqual(node.pdo[0x1600][0x2002].raw, 0xf)
 
+    def test_save_pdo(self):
+        node = canopen.Node(1, EDS_PATH)
+        node.tpdo.save()
+        node.rpdo.save()
+
 
 if __name__ == "__main__":
     unittest.main()