Repository: christiansandberg/canopen

Issue:
Behavior and failure for uninitialized network

https://github.com/christiansandberg/canopen/blob/5382f245d8ede61b113c6a648cb99b199667f0cd/canopen/node/local.py#L51-L59

Calling `LocalNode.remove_network()` or `RemoteNode.remove_network()` on an already closed network will fail, due to `self.network` no longer have the `.unsubscribe()` method (which is expected). Depending on what behavior is wanted, there lacks some functionality with regards to the logic of uninitialized `LocalNode`. Related to #525

### Option 1 - multiple calls to `.remove_network()` should be ok
If it should be acceptable to call `.remove_network()` multiple times without failing -> then these methods should support that the network is uninitialized

```py
    def remove_network(self) -> None:
        if self.network is canopen.network._UNINITIALIZED_NETWORK:
            return
        # Rest of the function....
```

### Option 2 - multiple calls to `.remove_network()` should fail
If a repeated call to `.remove_network()` should fail or fail on uninitialized network, I think we're missing a function to test if the `LocalNode` object is uninitialized. 

`LocalNode` and friends it sets the network uninitialized on its `__init__()` and then later initialize it by `.associate_network()`. This means that there are code paths through the usage of `LocalNode` where a cleanup code might need to call `.remove_network()` conditionally depending on the state of its association with a network.

There is no current way to test if a `LocalNode` object is associated without using the private `canopen.network._UNINITIALIZED_NETWORK` object. Moreso, `_UNINITIALIZED_NETWORK` is inherited from `Network` so a negative `isinstance(node.network, Network)` does not help to determine if its initialized.

To prevent access to the private object, I suggest that the best approach would be to add a new `.has_network()` method to the `LocalNode` and `RemoteNode` implementation.

```py
    def has_network(self):
       """Return True if the node has an associated network."""
       return self.network is not canopen.network._UNINITIALIZED_NETWORK
```

Discussion:
@acolomb: Thanks for pointing this out.  I prefer the first option, it does not make much sense to fail on that call.@sveinse: While working on this, it was uncovered that calling `associate_network()` multiple times is not safe either. The `Network.subscribe()` method happily appends multiple callbacks to its lists. This is addressed in the PR.

@sveinse: FYI I just discovered that we already have the a method for testing that a node is associated with a network or not: https://github.com/canopen-python/canopen/blob/48cca7c655f8c932b1f761a8edc11d0702397fab/canopen/node/base.py#L30-L32



Solution patch:
diff --git a/canopen/network.py b/canopen/network.py
index 02bec899..6f2777c8 100644
--- a/canopen/network.py
+++ b/canopen/network.py
@@ -75,10 +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)
+        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.
diff --git a/canopen/node/local.py b/canopen/node/local.py
index 8f2493d9..34bba899 100644
--- a/canopen/node/local.py
+++ b/canopen/node/local.py
@@ -39,6 +39,8 @@ def __init__(
         self.emcy = EmcyProducer(0x80 + self.id)
 
     def associate_network(self, network: canopen.network.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
@@ -49,6 +51,8 @@ def associate_network(self, network: canopen.network.Network):
         network.subscribe(0, self.nmt.on_command)
 
     def remove_network(self) -> None:
+        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)
         self.network = canopen.network._UNINITIALIZED_NETWORK
diff --git a/canopen/node/remote.py b/canopen/node/remote.py
index 226c0c0f..371f784c 100644
--- a/canopen/node/remote.py
+++ b/canopen/node/remote.py
@@ -51,6 +51,8 @@ def __init__(
             self.load_configuration()
 
     def associate_network(self, network: canopen.network.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
@@ -64,6 +66,8 @@ def associate_network(self, network: canopen.network.Network):
         network.subscribe(0, self.nmt.on_command)
 
     def remove_network(self) -> None:
+        if not self.has_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)


Test patch:
diff --git a/test/test_node.py b/test/test_node.py
new file mode 100644
index 00000000..43373a2a
--- /dev/null
+++ b/test/test_node.py
@@ -0,0 +1,104 @@
+import unittest
+
+import canopen
+
+
+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):
+
+    @classmethod
+    def setUpClass(cls):
+        cls.network = canopen.Network()
+        cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0
+        cls.network.connect(interface="virtual")
+
+        cls.node = canopen.LocalNode(2, canopen.objectdictionary.ObjectDictionary())
+
+    @classmethod
+    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)
+
+        # 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 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
+        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):
+
+    @classmethod
+    def setUpClass(cls):
+        cls.network = canopen.Network()
+        cls.network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0
+        cls.network.connect(interface="virtual")
+
+        cls.node = canopen.RemoteNode(2, canopen.objectdictionary.ObjectDictionary())
+
+    @classmethod
+    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)
+
+        # 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 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
+        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()