Make sure any BaseNode always has a valid Node ID (#657)

The BaseNode.id attribute can be initialized either from the given
node_id parameter, or from the Object Dictionary if zero was passed.
The None value as mentioned in the docstring actually violates the
type hint, so drop that suggestion.  Make sure the inferred type of
the self.id attribute does not allow None, but is always an integer.

Furthermore, check the resulting Node ID against the allowed value
range and raise a ValueError instead of constructing a node object
with an invalid Node ID, which will cause problems down the road.
This is technically an API change, but it just causes things to fail
early on instead of raising exceptions later on.

Copy the docstring for LocalNode as well, to document the expected
node_id "invalid" value.  And because there was none at all before,
include a bit of functional description to set the expectations
straight.

Author: acolomb

canopen/node/base.py

@@ -8,7 +8,7 @@ class BaseNode:
     """A CANopen node.
 
     :param node_id:
-        Node ID (set to None or 0 if specified by object dictionary)
+        Node ID (set to 0 if specified by object dictionary)
     :param object_dictionary:
         Object dictionary as either a path to a file, an ``ObjectDictionary``
         or a file like object.
@@ -25,7 +25,9 @@ def __init__(
             object_dictionary = import_od(object_dictionary, node_id)
         self.object_dictionary = object_dictionary
 
-        self.id = node_id or self.object_dictionary.node_id
+        self.id = node_id or object_dictionary.node_id or 0
+        if not 1 <= self.id <= 127:
+            raise ValueError(f"No valid Node ID provided, {self.id} not in range 1..127")
 
     def has_network(self) -> bool:
         """Check whether the node has been associated to a network."""

canopen/node/local.py

@@ -18,6 +18,21 @@
 
 
 class LocalNode(BaseNode):
+    """Local CANopen node implementing essential communication services.
+
+    This does not provide a full-fledged communication logic stack, but needs
+    additional application logic to wire up the various services, such as
+    triggering PDO transmissions according to their communication parameters.
+
+    Notable exceptions are a local data store for SDO server access, and using
+    the Heartbeat Producer Time parameter to control Heartbeat transmission.
+
+    :param node_id:
+        Node ID (set to 0 if specified by object dictionary)
+    :param object_dictionary:
+        Object dictionary as either a path to a file, an ``ObjectDictionary``
+        or a file like object.
+    """
 
     def __init__(
         self,

canopen/node/remote.py

@@ -19,7 +19,7 @@ class RemoteNode(BaseNode):
     """A CANopen remote node.
 
     :param node_id:
-        Node ID (set to None or 0 if specified by object dictionary)
+        Node ID (set to 0 if specified by object dictionary)
     :param object_dictionary:
         Object dictionary as either a path to a file, an ``ObjectDictionary``
         or a file like object.

test/test_node.py

@@ -8,6 +8,25 @@ def count_subscribers(network: canopen.Network) -> int:
     return sum(len(n) for n in network.subscribers.values())
 
 
+class TestBaseNode(unittest.TestCase):
+
+    def test_valid_node_id(self):
+        node = canopen.node.base.BaseNode(1, canopen.ObjectDictionary())
+        self.assertEqual(node.id, 1)
+
+    def test_valid_node_id_from_od(self):
+        od = canopen.ObjectDictionary()
+        od.node_id = 2
+        node = canopen.node.base.BaseNode(0, od)
+        self.assertEqual(node.id, 2)
+
+    def test_invalid_node_id(self):
+        with self.assertRaises(ValueError):
+            _ = canopen.node.base.BaseNode(0, canopen.ObjectDictionary())
+        with self.assertRaises(ValueError):
+            _ = canopen.node.base.BaseNode(128, canopen.ObjectDictionary())
+
+
 class TestLocalNode(unittest.TestCase):
 
     @classmethod