LocalNode improvements for TPDOs #524

grantweiss-RaymondCorp · 2024-07-29T20:47:57Z

Local Node improvements:

Implement sending of TPDOs on transition to NMT operational state. LocalNode not emitting TPDOs #328
Allow runtime (pre-op) configuration of TPDO COB-ID

grantweiss-RaymondCorp · 2024-07-29T21:17:24Z

I can also pull in grantweiss-RaymondCorp/canopen@2305314 if someone wants to confirm this is correct canopen behavior.

acolomb

Nice addition, but I think the implementation could be simplified by re-using more existing code. And some style nit-picks for consistency, see below.

canopen/node/local.py

acolomb · 2024-08-12T15:10:25Z

canopen/node/local.py

+    def _tpdo_configuration_write(self, index, subindex, od, data):
+        if 0x1800 <= index <= 0x19FF:
+            # Only allowed to edit pdo configuration in pre-op
+            if self.nmt.state != "PRE-OPERATIONAL":


That's not true in general, but we could make it a requirement for this library's implementation. Please indicate clearly if it should be the latter.

I almost had it, depending on what level we want to support (variable vs dynamic) it can be changed in pre-op or operational. https://www.can-cia.org/can-knowledge/pdo-protocol-1. I expanded it to allow while in operational. This still leaves out the PDO mapping procedure...

The only restriction I found in the standard is that subindex 03 "shall not be changed while the PDO exists (bit 31 of sub-index 01h is set to 0b)." So as long as the library's implementation does support dynamic reconfiguration (which I think it does?) we have no reason to restrict its usage IMHO.

The mentioned restriction as well as the PDO mapping procedure should be handled in the PDO code, but that's definitely not a blocker for this PR.

canopen/node/local.py

Simplify applying changes to PDO mapping by using pdo.read(from_od=True).

grantweiss-RaymondCorp · 2024-08-12T18:11:40Z

@acolomb Thank you for looking over everything, I think I addressed it all.

acolomb · 2024-08-12T19:22:58Z

canopen/node/local.py

+        if 0x1800 <= index <= 0x19FF:
+            # TPDO Communication parameter changed
+            tpdoNum = (index - 0x1800) + 1
+            self._tpdo_configuration_write(tpdoNum)
+        elif 0x1A00 <= index <= 0x1BFF:
+            # TPDO Mapping parameter changed
+            tpdoNum = (index - 0x1A00) + 1
+            self._tpdo_configuration_write(tpdoNum)


This could be shortened a bit because the ranges are adjacent. You only need one range check and one modulo to find the TPDO number. But feel free to ignore this if you think it's not an improvement.

acolomb · 2024-08-12T19:25:00Z

canopen/node/local.py

@@ -34,6 +34,8 @@ def __init__(
        self.add_write_callback(self.nmt.on_write)
        self.emcy = EmcyProducer(0x80 + self.id)

+        self.nmt.add_state_change_callback(self._nmt_state_changed)


I'm wondering whether this should be made opt-in. There is certainly existing code using LocalNode which already manages the TPDO transmissions externally, because the library didn't use to do it. With this change, these users could end up with duplicated transmissions, or at least things stepping on each others' toes.

What would be the preferred way to achieve this? A keyword argument in the LocalNode constructor?
Would the _tpdo_configuration_write call inside set_data also be opt-in?

Yes, I think an additional argument auto_tpdo: bool = False would be fine, with appropriate documentation (docstring) of course.

And while we're at it, I think the _nmt_state_changed name does not convey what the function does. It should rather be named _tpdo_auto_start_stop, then this line will clearly express what the connection is (NMT state --> TPDO start).

Would the _tpdo_configuration_write call inside set_data also be opt-in?

No, I think if someone writes to the local node's objects, it is quite expected to affect the PDO settings. Actually even for RPDOs. Of course our processing of these parameters is quite rudimentary now and not really standard compliant, but that can be improved step by step.

acolomb · 2024-08-12T19:26:58Z

canopen/node/local.py

+    def _nmt_state_changed(self, old_state, new_state):
+        if new_state == "OPERATIONAL":
+            for pdo in self.tpdo.map.values():
+                if pdo.enabled:


Maybe a matter of taste, but I would prefer an inverted check and a continue here. Makes for less indentation and doesn't stretch the else branch possibly out of sight.

And this will generate warnings for any TPDO not configured as periodic, right? Those should probably rather be skipped silently.

acolomb · 2024-08-12T19:28:58Z

canopen/node/local.py

+                    except Exception:
+                        logger.exception("Unknown error starting %s", pdo.name)
+                else:
+                    logger.info("%s not enabled", pdo.name)


Now this logging is probably too much. Worst case, it will be emitted for all 512 PDOs. That's too much even on DEBUG level. I'd just drop this log call.

acolomb · 2024-08-12T19:30:15Z

canopen/node/local.py

+        try:
+            pdo.read(from_od=True)
+        except:
+            pass


Why would we want to silently swallow any and all exceptions here?

acolomb · 2024-08-12T19:37:40Z

test/test_local.py

+        self.local_node.nmt.state = 'PRE-OPERATIONAL'
+
+        # Attempt to re-enable tpdo 1 via sdo writing
+        PDO_NOT_VALID = 1 << 31


Possible to re-use canopen.pdo.base.PDO_NOT_VALID?

acolomb · 2024-08-12T19:38:40Z

test/test_local.py

+        # Disable tpdo 1
+        self.local_node.tpdo[1].enabled = False
+        self.local_node.tpdo[1].cob_id = 0
+        self.local_node.tpdo[1].period = 0.5 # manually assign a period


I don't understand that comment. Just above, you do the same without a comment. Is this intended to test mix & match between direct assignment and SDO based changes?

Isn't it using the value from the inhibit time object anyway?

It doesn't use inhibit time currently. I have a different branch for this, I wasn't sure if I should try to pull it in with this stuff. Yes the intention here was to test to make sure the SDOs are correctly being propagated to the PDO configuration stack. grantweiss-RaymondCorp@0a9f50d

The approach of falling back to event_timer looks reasonable. But I agree we don't have to do it all in this PR, but can iterate with more improvements / features once this is merged.

acolomb · 2024-08-12T19:44:42Z

test/test_local.py

+
+        # Attempt to re-enable tpdo 1 via sdo writing
+        PDO_NOT_VALID = 1 << 31
+        odDefaultVal = self.local_node.object_dictionary["Transmit PDO 0 communication parameters.COB-ID use by TPDO 1"].default


I'd use the numeric index here, as it's less fragile and shorter. There's already a typo in that description, and fixing it would break the test 😉

acolomb · 2024-08-12T19:45:24Z

test/test_local.py

+
+        self.remote_node.sdo["Transmit PDO 0 communication parameters.COB-ID use by TPDO 1"].raw = enabledCobId
+
+        # Transition to operational


These two comments don't really help. The test code should be simple enough to not need an explanation.

codecov-commenter · 2024-08-12T19:53:05Z

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

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 72.01%. Comparing base (a196e1e) to head (43e32a7).
Report is 10 commits behind head on master.

Files	Patch %	Lines
canopen/node/local.py	72.41%	6 Missing and 2 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
+ Coverage   68.86%   72.01%   +3.15%     
==========================================
  Files          26       26              
  Lines        3112     3148      +36     
  Branches      526      536      +10     
==========================================
+ Hits         2143     2267     +124     
+ Misses        831      747      -84     
+ Partials      138      134       -4

Files	Coverage Δ
canopen/nmt.py	`94.32% <100.00%> (+1.09%)`	⬆️
canopen/node/local.py	`81.81% <72.41%> (-3.37%)`	⬇️

... and 9 files with indirect coverage changes

grantweiss-RaymondCorp added 5 commits July 29, 2024 13:22

Add nmt state change callbacks

e184bf5

Enable local node PDOs on transition to operational

853f7eb

Add unit test for local node pdo transmissions

290a358

Make new callback method private

b5e546f

Add sdo write callback to local node for tpdo configuration

822009c

acolomb requested changes Aug 12, 2024

View reviewed changes

Fix styling to match project.

43e32a7

Simplify applying changes to PDO mapping by using pdo.read(from_od=True).

acolomb requested changes Aug 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LocalNode improvements for TPDOs #524

LocalNode improvements for TPDOs #524

grantweiss-RaymondCorp commented Jul 29, 2024

grantweiss-RaymondCorp commented Jul 29, 2024 •

edited

Loading

acolomb left a comment

acolomb Aug 12, 2024

grantweiss-RaymondCorp Aug 12, 2024

acolomb Aug 12, 2024 •

edited

Loading

grantweiss-RaymondCorp commented Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

grantweiss-RaymondCorp Aug 12, 2024 •

edited

Loading

acolomb Aug 13, 2024

acolomb Aug 13, 2024

acolomb Aug 12, 2024

acolomb Aug 13, 2024 •

edited

Loading

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

grantweiss-RaymondCorp Aug 12, 2024 •

edited

Loading

acolomb Aug 13, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

codecov-commenter commented Aug 12, 2024


		self.remote_node.sdo["Transmit PDO 0 communication parameters.COB-ID use by TPDO 1"].raw = enabledCobId

		# Transition to operational

LocalNode improvements for TPDOs #524

Are you sure you want to change the base?

LocalNode improvements for TPDOs #524

Conversation

grantweiss-RaymondCorp commented Jul 29, 2024

grantweiss-RaymondCorp commented Jul 29, 2024 • edited Loading

acolomb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolomb Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

grantweiss-RaymondCorp commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantweiss-RaymondCorp Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolomb Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantweiss-RaymondCorp Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 12, 2024

Codecov Report

grantweiss-RaymondCorp commented Jul 29, 2024 •

edited

Loading

acolomb Aug 12, 2024 •

edited

Loading

grantweiss-RaymondCorp Aug 12, 2024 •

edited

Loading

acolomb Aug 13, 2024 •

edited

Loading

grantweiss-RaymondCorp Aug 12, 2024 •

edited

Loading