From 7e82e285a12cef5a70a6876525f1c0dbd735cf8f Mon Sep 17 00:00:00 2001 From: Svein Seldal Date: Sat, 26 Apr 2025 15:48:52 +0200 Subject: [PATCH 1/5] Fix multiple assosciations and removals of networks --- canopen/node/local.py | 6 ++ canopen/node/remote.py | 6 ++ test/test_node.py | 121 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+) create mode 100644 test/test_node.py diff --git a/canopen/node/local.py b/canopen/node/local.py index 8f2493d9..18f91a41 100644 --- a/canopen/node/local.py +++ b/canopen/node/local.py @@ -39,6 +39,9 @@ def __init__( self.emcy = EmcyProducer(0x80 + self.id) def associate_network(self, network: canopen.network.Network): + if self.network is not canopen.network._UNINITIALIZED_NETWORK: + # Unsubscribe from old network (to prevent double subscription) + self.remove_network() self.network = network self.sdo.network = network self.tpdo.network = network @@ -49,6 +52,9 @@ def associate_network(self, network: canopen.network.Network): network.subscribe(0, self.nmt.on_command) def remove_network(self) -> None: + # Make it safe to call this method multiple times + if self.network is canopen.network._UNINITIALIZED_NETWORK: + return self.network.unsubscribe(self.sdo.rx_cobid, self.sdo.on_request) self.network.unsubscribe(0, self.nmt.on_command) self.network = canopen.network._UNINITIALIZED_NETWORK diff --git a/canopen/node/remote.py b/canopen/node/remote.py index 226c0c0f..91bf3329 100644 --- a/canopen/node/remote.py +++ b/canopen/node/remote.py @@ -51,6 +51,9 @@ def __init__( self.load_configuration() def associate_network(self, network: canopen.network.Network): + if self.network is not canopen.network._UNINITIALIZED_NETWORK: + # Unsubscribe from old network (to prevent double subscriptions) + self.remove_network() self.network = network self.sdo.network = network self.pdo.network = network @@ -64,6 +67,9 @@ def associate_network(self, network: canopen.network.Network): network.subscribe(0, self.nmt.on_command) def remove_network(self) -> None: + # Make it safe to call this method multiple times + if self.network is canopen.network._UNINITIALIZED_NETWORK: + return for sdo in self.sdo_channels: self.network.unsubscribe(sdo.tx_cobid, sdo.on_response) self.network.unsubscribe(0x700 + self.id, self.nmt.on_heartbeat) diff --git a/test/test_node.py b/test/test_node.py new file mode 100644 index 00000000..6cd89ef6 --- /dev/null +++ b/test/test_node.py @@ -0,0 +1,121 @@ +"""Unit tests for the RemoteNode and LocalNode classes.""" +import unittest + +import canopen + +from .util import SAMPLE_EDS + + +def count_subscribers(network: canopen.Network) -> int: + """Count the number of subscribers in the network.""" + return sum( + len(n) for n in network.subscribers.values() + ) + + +class TestLocalNode(unittest.TestCase): + """ + Test local node. + """ + + @classmethod + def setUpClass(cls): + cls.network = canopen.Network() + cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0 + cls.network.connect("test", interface="virtual") + + cls.node = canopen.LocalNode(2, SAMPLE_EDS) + + @classmethod + def tearDownClass(cls): + cls.network.disconnect() + + def test_associate_network(self): + + # Need to store the number of subscribers before associating because + # the current network implementation automatically adds subscribers + # to the list + n_subscribers = count_subscribers(self.network) + + # Associating the network with the local node + self.node.associate_network(self.network) + self.assertIs(self.node.network, self.network) + self.assertIs(self.node.sdo.network, self.network) + self.assertIs(self.node.tpdo.network, self.network) + self.assertIs(self.node.rpdo.network, self.network) + self.assertIs(self.node.nmt.network, self.network) + self.assertIs(self.node.emcy.network, self.network) + + # Test that its possible to associate the network multiple times + # by checking that the number of subscribers remains the same + count = count_subscribers(self.network) + self.node.associate_network(self.network) + self.assertEqual(count_subscribers(self.network), count) + + # Test removal of the network. The count of subscribers should + # be the same as before the association + self.node.remove_network() + uninitalized = canopen.network._UNINITIALIZED_NETWORK + self.assertIs(self.node.network, uninitalized) + self.assertIs(self.node.sdo.network, uninitalized) + self.assertIs(self.node.tpdo.network, uninitalized) + self.assertIs(self.node.rpdo.network, uninitalized) + self.assertIs(self.node.nmt.network, uninitalized) + self.assertIs(self.node.emcy.network, uninitalized) + self.assertEqual(count_subscribers(self.network), n_subscribers) + + # Test that its possible to deassociate the network multiple times + self.node.remove_network() + + +class TestRemoteNode(unittest.TestCase): + """ + Test remote node. + """ + + @classmethod + def setUpClass(cls): + cls.network = canopen.Network() + cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0 + cls.network.connect("test", interface="virtual") + + cls.node = canopen.RemoteNode(2, SAMPLE_EDS) + + @classmethod + def tearDownClass(cls): + cls.network.disconnect() + + def test_associate_network(self): + + # Need to store the number of subscribers before associating because + # the current network implementation automatically adds subscribers + # to the list + n_subscribers = count_subscribers(self.network) + + # Associating the network with the local node + self.node.associate_network(self.network) + self.assertIs(self.node.network, self.network) + self.assertIs(self.node.sdo.network, self.network) + self.assertIs(self.node.tpdo.network, self.network) + self.assertIs(self.node.rpdo.network, self.network) + self.assertIs(self.node.nmt.network, self.network) + + # Test that its possible to associate the network multiple times + # by checking that the number of subscribers remains the same + count = count_subscribers(self.network) + self.node.associate_network(self.network) + self.assertEqual(count_subscribers(self.network), count) + + # Test removal of the network. The count of subscribers should + # be the same as before the association + self.node.remove_network() + uninitalized = canopen.network._UNINITIALIZED_NETWORK + self.assertIs(self.node.network, uninitalized) + self.assertIs(self.node.sdo.network, uninitalized) + self.assertIs(self.node.tpdo.network, uninitalized) + self.assertIs(self.node.rpdo.network, uninitalized) + self.assertIs(self.node.nmt.network, uninitalized) + self.assertEqual(count_subscribers(self.network), n_subscribers) + + # Test that its possible to deassociate the network multiple times + self.node.remove_network() From d02d5ee6e5977ba3ce9a6d7dd2508913a06db975 Mon Sep 17 00:00:00 2001 From: Svein Seldal Date: Mon, 28 Apr 2025 17:55:05 +0200 Subject: [PATCH 2/5] Updated from review --- canopen/network.py | 3 +++ canopen/node/local.py | 7 +++---- canopen/node/remote.py | 7 +++---- test/test_node.py | 31 +++++++++++++------------------ 4 files changed, 22 insertions(+), 26 deletions(-) diff --git a/canopen/network.py b/canopen/network.py index 02bec899..9610f027 100644 --- a/canopen/network.py +++ b/canopen/network.py @@ -79,6 +79,9 @@ def unsubscribe(self, can_id, callback=None) -> None: del self.subscribers[can_id] else: self.subscribers[can_id].remove(callback) + # Remove empty list entry + if not self.subscribers[can_id]: + del self.subscribers[can_id] def connect(self, *args, **kwargs) -> Network: """Connect to CAN bus using python-can. diff --git a/canopen/node/local.py b/canopen/node/local.py index 18f91a41..af6fd660 100644 --- a/canopen/node/local.py +++ b/canopen/node/local.py @@ -39,9 +39,8 @@ def __init__( self.emcy = EmcyProducer(0x80 + self.id) def associate_network(self, network: canopen.network.Network): - if self.network is not canopen.network._UNINITIALIZED_NETWORK: - # Unsubscribe from old network (to prevent double subscription) - self.remove_network() + if self.has_network(): + raise RuntimeError("Node is already associated with a network") self.network = network self.sdo.network = network self.tpdo.network = network @@ -53,7 +52,7 @@ def associate_network(self, network: canopen.network.Network): def remove_network(self) -> None: # Make it safe to call this method multiple times - if self.network is canopen.network._UNINITIALIZED_NETWORK: + if not self.has_network(): return self.network.unsubscribe(self.sdo.rx_cobid, self.sdo.on_request) self.network.unsubscribe(0, self.nmt.on_command) diff --git a/canopen/node/remote.py b/canopen/node/remote.py index 91bf3329..5fd027c7 100644 --- a/canopen/node/remote.py +++ b/canopen/node/remote.py @@ -51,9 +51,8 @@ def __init__( self.load_configuration() def associate_network(self, network: canopen.network.Network): - if self.network is not canopen.network._UNINITIALIZED_NETWORK: - # Unsubscribe from old network (to prevent double subscriptions) - self.remove_network() + if self.has_network(): + raise RuntimeError("Node is already associated with a network") self.network = network self.sdo.network = network self.pdo.network = network @@ -68,7 +67,7 @@ def associate_network(self, network: canopen.network.Network): def remove_network(self) -> None: # Make it safe to call this method multiple times - if self.network is canopen.network._UNINITIALIZED_NETWORK: + if not self.has_network(): return for sdo in self.sdo_channels: self.network.unsubscribe(sdo.tx_cobid, sdo.on_response) diff --git a/test/test_node.py b/test/test_node.py index 6cd89ef6..8ebc8122 100644 --- a/test/test_node.py +++ b/test/test_node.py @@ -2,6 +2,7 @@ import unittest import canopen +import canopen.network from .util import SAMPLE_EDS @@ -14,15 +15,13 @@ def count_subscribers(network: canopen.Network) -> int: class TestLocalNode(unittest.TestCase): - """ - Test local node. - """ + """Unit tests for the LocalNode class.""" @classmethod def setUpClass(cls): cls.network = canopen.Network() cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0 - cls.network.connect("test", interface="virtual") + cls.network.connect(interface="virtual") cls.node = canopen.LocalNode(2, SAMPLE_EDS) @@ -46,11 +45,10 @@ def test_associate_network(self): self.assertIs(self.node.nmt.network, self.network) self.assertIs(self.node.emcy.network, self.network) - # Test that its possible to associate the network multiple times - # by checking that the number of subscribers remains the same - count = count_subscribers(self.network) - self.node.associate_network(self.network) - self.assertEqual(count_subscribers(self.network), count) + # Test that its not possible to associate the network multiple times + with self.assertRaises(RuntimeError) as cm: + self.node.associate_network(self.network) + self.assertIn("already associated with a network", str(cm.exception)) # Test removal of the network. The count of subscribers should # be the same as before the association @@ -69,15 +67,13 @@ def test_associate_network(self): class TestRemoteNode(unittest.TestCase): - """ - Test remote node. - """ + """Unittests for the RemoteNode class.""" @classmethod def setUpClass(cls): cls.network = canopen.Network() cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0 - cls.network.connect("test", interface="virtual") + cls.network.connect(interface="virtual") cls.node = canopen.RemoteNode(2, SAMPLE_EDS) @@ -100,11 +96,10 @@ def test_associate_network(self): self.assertIs(self.node.rpdo.network, self.network) self.assertIs(self.node.nmt.network, self.network) - # Test that its possible to associate the network multiple times - # by checking that the number of subscribers remains the same - count = count_subscribers(self.network) - self.node.associate_network(self.network) - self.assertEqual(count_subscribers(self.network), count) + # Test that its not possible to associate the network multiple times + with self.assertRaises(RuntimeError) as cm: + self.node.associate_network(self.network) + self.assertIn("already associated with a network", str(cm.exception)) # Test removal of the network. The count of subscribers should # be the same as before the association From f9d477a39992d00e8fed193a64aa446f30cca4df Mon Sep 17 00:00:00 2001 From: Svein Seldal Date: Mon, 28 Apr 2025 18:01:00 +0200 Subject: [PATCH 3/5] Part 2 of updates --- canopen/node/local.py | 1 - canopen/node/remote.py | 1 - test/test_node.py | 17 +++++++---------- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/canopen/node/local.py b/canopen/node/local.py index af6fd660..34bba899 100644 --- a/canopen/node/local.py +++ b/canopen/node/local.py @@ -51,7 +51,6 @@ def associate_network(self, network: canopen.network.Network): network.subscribe(0, self.nmt.on_command) def remove_network(self) -> None: - # Make it safe to call this method multiple times if not self.has_network(): return self.network.unsubscribe(self.sdo.rx_cobid, self.sdo.on_request) diff --git a/canopen/node/remote.py b/canopen/node/remote.py index 5fd027c7..371f784c 100644 --- a/canopen/node/remote.py +++ b/canopen/node/remote.py @@ -66,7 +66,6 @@ def associate_network(self, network: canopen.network.Network): network.subscribe(0, self.nmt.on_command) def remove_network(self) -> None: - # Make it safe to call this method multiple times if not self.has_network(): return for sdo in self.sdo_channels: diff --git a/test/test_node.py b/test/test_node.py index 8ebc8122..2c510318 100644 --- a/test/test_node.py +++ b/test/test_node.py @@ -3,8 +3,7 @@ import canopen import canopen.network - -from .util import SAMPLE_EDS +import canopen.objectdictionary def count_subscribers(network: canopen.Network) -> int: @@ -23,7 +22,7 @@ def setUpClass(cls): cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0 cls.network.connect(interface="virtual") - cls.node = canopen.LocalNode(2, SAMPLE_EDS) + cls.node = canopen.LocalNode(2, canopen.objectdictionary.ObjectDictionary()) @classmethod def tearDownClass(cls): @@ -31,9 +30,8 @@ def tearDownClass(cls): def test_associate_network(self): - # Need to store the number of subscribers before associating because - # the current network implementation automatically adds subscribers - # to the list + # Need to store the number of subscribers before associating because the + # network implementation automatically adds subscribers to the list n_subscribers = count_subscribers(self.network) # Associating the network with the local node @@ -75,7 +73,7 @@ def setUpClass(cls): cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0 cls.network.connect(interface="virtual") - cls.node = canopen.RemoteNode(2, SAMPLE_EDS) + cls.node = canopen.RemoteNode(2, canopen.objectdictionary.ObjectDictionary()) @classmethod def tearDownClass(cls): @@ -83,9 +81,8 @@ def tearDownClass(cls): def test_associate_network(self): - # Need to store the number of subscribers before associating because - # the current network implementation automatically adds subscribers - # to the list + # Need to store the number of subscribers before associating because the + # network implementation automatically adds subscribers to the list n_subscribers = count_subscribers(self.network) # Associating the network with the local node From 38d3836e0f97933dd621dec19e1d36e1497a7184 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Mon, 28 Apr 2025 21:35:06 +0200 Subject: [PATCH 4/5] Apply suggestions from code review --- test/test_node.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/test/test_node.py b/test/test_node.py index 2c510318..43373a2a 100644 --- a/test/test_node.py +++ b/test/test_node.py @@ -1,20 +1,14 @@ -"""Unit tests for the RemoteNode and LocalNode classes.""" import unittest import canopen -import canopen.network -import canopen.objectdictionary def count_subscribers(network: canopen.Network) -> int: """Count the number of subscribers in the network.""" - return sum( - len(n) for n in network.subscribers.values() - ) + return sum(len(n) for n in network.subscribers.values()) class TestLocalNode(unittest.TestCase): - """Unit tests for the LocalNode class.""" @classmethod def setUpClass(cls): @@ -29,7 +23,6 @@ def tearDownClass(cls): cls.network.disconnect() def test_associate_network(self): - # Need to store the number of subscribers before associating because the # network implementation automatically adds subscribers to the list n_subscribers = count_subscribers(self.network) @@ -65,7 +58,6 @@ def test_associate_network(self): class TestRemoteNode(unittest.TestCase): - """Unittests for the RemoteNode class.""" @classmethod def setUpClass(cls): @@ -80,7 +72,6 @@ def tearDownClass(cls): cls.network.disconnect() def test_associate_network(self): - # Need to store the number of subscribers before associating because the # network implementation automatically adds subscribers to the list n_subscribers = count_subscribers(self.network) From e4ac0ce30986d361860f452fee3b188c6eb3ffa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Mon, 28 Apr 2025 21:35:56 +0200 Subject: [PATCH 5/5] Simplify cleanup logic in Network.unsubscribe(). --- canopen/network.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/canopen/network.py b/canopen/network.py index 9610f027..6f2777c8 100644 --- a/canopen/network.py +++ b/canopen/network.py @@ -75,13 +75,10 @@ def unsubscribe(self, can_id, callback=None) -> None: If given, remove only this callback. Otherwise all callbacks for the CAN ID. """ - if callback is None: - del self.subscribers[can_id] - else: + if callback is not None: self.subscribers[can_id].remove(callback) - # Remove empty list entry - if not self.subscribers[can_id]: - del self.subscribers[can_id] + if not self.subscribers[can_id] or callback is None: + del self.subscribers[can_id] def connect(self, *args, **kwargs) -> Network: """Connect to CAN bus using python-can.