Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LocalNode improvements for TPDOs #524

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions canopen/nmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ def __init__(self, node_id: int):
self.id = node_id
self.network = None
self._state = 0
self._state_change_callbacks = []

def add_state_change_callback(self, callback: Callable[[str, str], None]):
"""Add function to be called on nmt state change.

:param callback:
Function that should accept an old NMT state and new NMT state as arguments.
"""
self._state_change_callbacks.append(callback)

def on_command(self, can_id, data, timestamp):
cmd, node_id = struct.unpack_from("BB", data)
Expand All @@ -57,6 +66,8 @@ def on_command(self, can_id, data, timestamp):
if new_state != self._state:
logger.info("New NMT state %s, old state %s",
NMT_STATES[new_state], NMT_STATES[self._state])
for callback in self._state_change_callbacks:
callback(old_state = NMT_STATES[self._state], new_state = NMT_STATES[new_state])
self._state = new_state

def send_command(self, code: int):
Expand All @@ -69,6 +80,9 @@ def send_command(self, code: int):
new_state = COMMAND_TO_STATE[code]
logger.info("Changing NMT state on node %d from %s to %s",
self.id, NMT_STATES[self._state], NMT_STATES[new_state])
if new_state != self._state:
for callback in self._state_change_callbacks:
callback(old_state = NMT_STATES[self._state], new_state = NMT_STATES[new_state])
self._state = new_state

@property
Expand Down
40 changes: 40 additions & 0 deletions canopen/node/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def associate_network(self, network):
self.network = network
self.sdo.network = network
Expand Down Expand Up @@ -115,6 +117,15 @@ def set_data(
self.data_store.setdefault(index, {})
self.data_store[index][subindex] = bytes(data)

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)
Comment on lines +120 to +127
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def _find_object(self, index, subindex):
if index not in self.object_dictionary:
# Index does not exist
Expand All @@ -127,3 +138,32 @@ def _find_object(self, index, subindex):
raise SdoAbortedError(0x06090011)
obj = obj[subindex]
return obj

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@acolomb acolomb Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try:
pdo.start()
logger.info("Successfully started %s", pdo.name)
except ValueError:
logger.warning("Failed to start %s due to missing period", pdo.name)
except Exception:
logger.exception("Unknown error starting %s", pdo.name)
else:
logger.info("%s not enabled", pdo.name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
self.tpdo.stop()

def _tpdo_configuration_write(self, tpdoNum):
pdo = self.tpdo.map[tpdoNum]

# Only allowed to edit pdo configuration in pre-op or operational
if self.nmt.state not in ("PRE-OPERATIONAL", "OPERATIONAL"):
logger.warning("Tried to configure %s when not in pre-op or operational", pdo.name)
return

try:
pdo.read(from_od=True)
except:
pass
Comment on lines +166 to +169
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

31 changes: 31 additions & 0 deletions test/test_local.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,37 @@ def test_save(self):
self.remote_node.pdo.save()
self.local_node.pdo.save()

def test_send_pdo_on_operational(self):
self.local_node.tpdo[1].period = 0.5

self.local_node.nmt.state = 'INITIALISING'
self.local_node.nmt.state = 'PRE-OPERATIONAL'
self.local_node.nmt.state = 'OPERATIONAL'

self.assertNotEqual(self.local_node.tpdo[1]._task, None)

def test_config_pdo(self):
# 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


self.local_node.nmt.state = 'INITIALISING'
self.local_node.nmt.state = 'PRE-OPERATIONAL'

# Attempt to re-enable tpdo 1 via sdo writing
PDO_NOT_VALID = 1 << 31
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

odDefaultVal = self.local_node.object_dictionary["Transmit PDO 0 communication parameters.COB-ID use by TPDO 1"].default
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

enabledCobId = odDefaultVal & ~PDO_NOT_VALID # Ensure invalid bit is not set

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

# Transition to operational
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.local_node.nmt.state = 'OPERATIONAL'

# Ensure tpdo automatically started with transition
self.assertNotEqual(self.local_node.tpdo[1]._task, None)


if __name__ == "__main__":
unittest.main()