Repository: christiansandberg/canopen

Issue:
Saving dynamic/empty PDOs: TypeError: unsupported operand type(s) for |: 'NoneType' and 'int'
Currently, if you have dynamic PDOs, which are only defined but not configured in the EDS-File, you can not save them, because they are initialized with cob_id = None, which can not be compared in this loc in pdo/base.py:376

`self.com_record[1].raw = self.cob_id | PDO_NOT_VALID | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)`
     
Also, map is initilized with [], but the save-check checks for None, which is never the case, so all pdo will be saved, even if they are empty.

Test to reproduce:

```
    def test_save_pdo(self):
        node = canopen.Node(1, EDS_PATH)
        node.tpdo.save()
        node.rpdo.save()
```

Discussion:
friederschueler: Maybe the log output is overkill, also if you don't like the other minor changes in the file I can exclude them.
friederschueler: Ok, I hope reduced my PR to a minimal but usable state.
acolomb: > I also don't like silent fails, but I even more dislike broken default code. My Use-Case is the following: I have a lot of PDOs on devices, I have real device (aka master) on the bus, but for testing I fake some bus communication with my python canopen. So all my PDOs are already setup and configured by the real master. I want to use those PDOs without reading them all over the real bus, this triggers errors because the bus load is pretty high. Now I want to change some PDOs or verify (read) some PDOs without iterating over all devices and all PDOs to set the cob-id to 0x0.

That use case is perfectly valid and I can add another example I've encountered:  The master is implemented in python-canopen and configures the other nodes' communication.  But when it restarts for whatever reason (especially during development), there is no need to switch all other nodes back to PRE-OP, exchange lots of SDOs and go back to OPERATIONAL, just to reach the same state again.  The nodes may even have their configuration saved in non-volatile memory and thus never need PDO configuration.  For a fast startup time, the master can then be coded to simply assume a fixed PDO layout, without reading via SDO first.

I don't care that much about what the original intention was to allow `None` as COB ID value.  Just that we don't take away possible uses of our API for no good reason.  I wanted to confirm that disabling PDOs using `save()` is still possible with this change.  Which I think we both agree on.

The question remains: Is a log warning appropriate to get the API user's attention and let them initialize cob_id to zero for unwanted PDOs?  Or is that overkill and there are many valid cases where PDO descriptions are not initialized properly before saving, so that a warning will be too noisy?
friederschueler: > I don't care that much about what the original intention was to allow `None` as COB ID value. Just that we don't take away possible uses of our API for no good reason. I wanted to confirm that disabling PDOs using `save()` is still possible with this change. Which I think we both agree on.

Yes we do, that would be a bad bugfix if we remove usage options.
 
> The question remains: Is a log warning appropriate to get the API user's attention and let them initialize cob_id to zero for unwanted PDOs? Or is that overkill and there are many valid cases where PDO descriptions are not initialized properly before saving, so that a warning will be too noisy?

The more I think about it, I think we should just give them a "warning" with logger.info() as all the other messages in this method also use this level. In my use-case it is encouraged to only save the needed pdos, not everything. I also think we should add another line if we enable the PDO, because we also log that we temporarily disable the PDO. I know there is also a logger line when subscribing, but you can do that without saving first:


        if self.enabled:
            cob_id = self.cob_id | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
            self.com_record[1].raw = cob_id
            logger.info("Setting COB-ID 0x%X and re-enabling PDO", cob_id)
            self.subscribe()

acolomb: I guess Info level works fine.  We can still fine-tune that in a later release if people have issues with that.

The extra log message seems good, although I would prefer it before the actual SDO access. 
friederschueler: done

Patch:
diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py
index 1f4bf3e..1784760 100644
--- a/canopen/pdo/base.py
+++ b/canopen/pdo/base.py
@@ -215,7 +215,6 @@ class PdoMap:
                        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 @@ class PdoMap:
             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 @@ class PdoMap:
 
     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 @@ class PdoMap:
             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 @@ class PdoMap:
         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 @@ class PdoVariable(variable.Variable):
             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 patch:
diff --git a/test/test_pdo.py b/test/test_pdo.py
index 6bba18f..32963cf 100644
--- a/test/test_pdo.py
+++ b/test/test_pdo.py
@@ -55,6 +55,11 @@ class TestPDO(unittest.TestCase):
         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()