Save the passed node ID to the resulting OD in import_od()

[erlend-aasland]

Fixes #478

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.84%. Comparing base (cc37aee) to head (ac79a35).
Report is 1 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage   67.82%   67.84%   +0.02%     
==========================================
  Files          26       26              
  Lines        3105     3107       +2     
  Branches      522      524       +2     
==========================================
+ Hits         2106     2108       +2     
  Misses        859      859              
  Partials      140      140              

Files	Coverage Δ
canopen/objectdictionary/eds.py	83.90% <100.00%> (+0.09%)	⬆️

[erlend-aasland]

Note: I deliberately changed the value of NodeID in sample.eds, since base=0 is used when converting from str to int, just to make sure we test that aspect as well.

[acolomb]

Wouldn't it make sense to stick with the node_id parameter in places where it's used? Just in addition, override the od.node_id attribute if the argument was given. I think it's safer not having to remember the od. prefix in any later usage within the function.

[erlend-aasland]

Wouldn't it make sense to stick with the node_id parameter in places where it's used? Just in addition, override the od.node_id attribute if the argument was given. I think it's safer not having to remember the od. prefix in any later usage within the function.

That would absolutely reduce the diff, but I generally don't like shadowing parameters/variables, which is why I went for the od member. There is of course the risk that new code would be written using node_id, and not the correct od.node_id, but such code would probably be copy-and-pasted from the existing snippets that correctly use od member anyway.

I don't have a strong opinion about this; I'll change it as you wish.

[acolomb]

Yea I prefer overriding the node_id parameter, as it was. It's like the basic pattern for optional parameters, where None gets replaced by a default value (from the file in this case). And for the actual change, we don't "shadow" the parameter, but rather just need to make sure it is respected when copying to the OD instance. And it's less cognitive load when reading without the od. prefix.

[erlend-aasland]

Addressed in 8a22e7e. But, there's a case we're not testing: if the passed node_id argument is None and there is not Device Comissioning.Node ID entry, we'll end up with a node ID of zero, which is mean for broadcast only AFAIK. Do we want to emit a warning, or perhaps raise an exception in that case? Or perhaps another default?

[acolomb]

An ObjectDictionary instance does not need a node_id at all, so keeping it at None is fine IMHO. Think of it as a blueprint for a Node, which is where the node ID really gets relevant. It's absolutely expected to share the same OD instance between several nodes, so the OD's node ID is ambiguous anyway. The whole saving within the OD instance serves only a single purpose: Loading DCF files (for a single, specific, pre-parameterized node) with the same code as for EDS. That's really the only case when ObjectDictionary.node_id makes sense.

[erlend-aasland]

For the case described in #484 (comment), of course the node ID ends up being None, not 0.

[erlend-aasland]

Thanks for the insight!

[erlend-aasland]

I still would consider adding this patch to the PR before merging:

diff --git a/test/test_eds.py b/test/test_eds.py
index f46cc0e..00d5013 100644
--- a/test/test_eds.py
+++ b/test/test_eds.py
@@ -4,7 +4,8 @@ import canopen
 from canopen.objectdictionary.eds import _signed_int_from_hex
 from canopen.utils import pretty_index
 
-EDS_PATH = os.path.join(os.path.dirname(__file__), 'sample.eds')
+SAMPLE_EDS = os.path.join(os.path.dirname(__file__), 'sample.eds')
+DATATYPES_EDS = os.path.join(os.path.dirname(__file__), 'datatypes.eds')
 
 
 class TestEDS(unittest.TestCase):
@@ -48,7 +49,7 @@ class TestEDS(unittest.TestCase):
     }
 
     def setUp(self):
-        self.od = canopen.import_od(EDS_PATH, 2)
+        self.od = canopen.import_od(SAMPLE_EDS, 2)
 
     def test_load_nonexisting_file(self):
         with self.assertRaises(IOError):
@@ -59,17 +60,22 @@ class TestEDS(unittest.TestCase):
             canopen.import_od(__file__)
 
     def test_load_file_object(self):
-        with open(EDS_PATH) as fp:
+        with open(SAMPLE_EDS) as fp:
             od = canopen.import_od(fp)
         self.assertTrue(len(od) > 0)
 
     def test_load_implicit_nodeid(self):
         # sample.eds has a DeviceComissioning section with NodeID set to 0x10.
-        od = canopen.import_od(EDS_PATH)
+        od = canopen.import_od(SAMPLE_EDS)
         self.assertEqual(od.node_id, 16)
 
+    def test_load_implicit_nodeid_no_devicecomissioning_section(self):
+        # datatypes.eds has no DeviceComissioning section.
+        od = canopen.import_od(DATATYPES_EDS)
+        self.assertIsNone(od.node_id)
+
     def test_load_explicit_nodeid(self):
-        od = canopen.import_od(EDS_PATH, node_id=3)
+        od = canopen.import_od(SAMPLE_EDS, node_id=3)
         self.assertEqual(od.node_id, 3)
 
     def test_variable(self):

[acolomb]

I still would consider adding this patch to the PR before merging:

Sure, go ahead. The separate sample EDS files were introduced recently for the PDO stuff IIRC.

[erlend-aasland]

Thanks for the review!