From b2fa0bb83eb7833934786b8331f9d30a37aa9736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Mon, 27 May 2024 11:12:14 +0200 Subject: [PATCH 1/2] Bugfix for https://github.com/christiansandberg/canopen/issues/445 - changed save() Method of PdoBase to be more robust nad without an always true condition - use getattr directly in curtis hack - removed unused variables - added Test for PDO saving --- canopen/pdo/base.py | 78 ++++++++++++++++++++++----------------------- test/test_pdo.py | 5 +++ 2 files changed, 44 insertions(+), 39 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index 7312f660..b4fb45ca 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -215,7 +215,6 @@ def __getitem_by_name(self, value): value, ', '.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,7 @@ 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 +370,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.debug("Skipping %s: no cob-id 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,39 +388,38 @@ 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%X:%d, %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%X:%d, %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) @@ -521,7 +521,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 +600,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 diff --git a/test/test_pdo.py b/test/test_pdo.py index 6bba18fe..32963cf3 100644 --- a/test/test_pdo.py +++ b/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() From b470a0a0d7ea7e43b4f5e6b7178d117b036131ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Tue, 4 Jun 2024 17:07:55 +0200 Subject: [PATCH 2/2] Update canopen/pdo/base.py: - additional logging on enabling pdo - minor formatting options --- canopen/pdo/base.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/canopen/pdo/base.py b/canopen/pdo/base.py index b4fb45ca..a1f1d8e0 100644 --- a/canopen/pdo/base.py +++ b/canopen/pdo/base.py @@ -359,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 getattr(self.pdo_node.node, "curtis_hack", False): # 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,7 +372,7 @@ def _raw_from(param): def save(self) -> None: """Save PDO configuration for this map using SDO.""" if self.cob_id is None: - logger.debug("Skipping %s: no cob-id set", self.com_record.od.name) + 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) @@ -422,7 +423,9 @@ def save(self) -> None: 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: