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 5 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
39 changes: 39 additions & 0 deletions canopen/node/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ 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.

self.add_write_callback(self._tpdo_configuration_write)

def associate_network(self, network):
self.network = network
self.sdo.network = network
Expand Down Expand Up @@ -127,3 +130,39 @@ 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 i, pdo in self.tpdo.map.items():
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(f"Successfully started TPDO {i}")
acolomb marked this conversation as resolved.
Show resolved Hide resolved
except ValueError:
logger.warning(f"Failed to start TPDO {i} due to missing period")
except Exception as e:
logger.error(f"Unknown error starting TPDO {i}: {str(e)}")
acolomb marked this conversation as resolved.
Show resolved Hide resolved
else:
logger.info(f"TPDO {i} not enabled")
elif old_state == "OPERATIONAL":
acolomb marked this conversation as resolved.
Show resolved Hide resolved
self.tpdo.stop()

def _tpdo_configuration_write(self, index, subindex, od, data):
if 0x1800 <= index <= 0x19FF:
acolomb marked this conversation as resolved.
Show resolved Hide resolved
# Only allowed to edit pdo configuration in pre-op
if self.nmt.state != "PRE-OPERATIONAL":
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@acolomb acolomb Aug 12, 2024

Choose a reason for hiding this comment

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

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.

logger.warning("Tried to configure tpdo when not in pre-op")
return

if subindex == 0x01:
if len(data) == 4:
tpdoNum = (index - 0x1800) + 1
if tpdoNum in self.tpdo.map.keys():
PDO_NOT_VALID = 1 << 31
RTR_NOT_ALLOWED = 1 << 30

cob_id = int.from_bytes(data, 'little') & 0x7FF

self.tpdo.map[tpdoNum].cob_id = cob_id & 0x1FFFFFFF
self.tpdo.map[tpdoNum].enabled = cob_id & PDO_NOT_VALID == 0
self.tpdo.map[tpdoNum].rtr_allowed = cob_id & RTR_NOT_ALLOWED == 0
acolomb marked this conversation as resolved.
Show resolved Hide resolved
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()