Skip to content

Conversation

@acolomb
Copy link
Member

@acolomb acolomb commented Aug 2, 2021

The BaseNode402.op_mode setter carries out some precautionary steps as a side-effect. Some of these were already questioned in the code comments.

These changes push the responsibility for these measures toward the application, where they are just as easy to implement, but it's hard to avoid them when not needed or harmful to the application logic. The greatest possible benefit is less SDO bus traffic and delays when using the library functions, which sums up quickly with multiple drive controllers involved.

As the comment already stated, clearing the target values should
possibly happen before switching to the OPERATION ENABLED state, to
avoid unexpected movements.  So doing that when actually leaving that
state is mostly useless.

Some legitimate use cases even require switching the operation mode
while in OPERATION ENABLED, e.g. switching between Profile Position
and Profile Velocity.  This change does not allow that, but at the
very least will avoid the need to reset target values again.
Some legitimate use cases require switching the operation mode while
in OPERATION ENABLED, e.g. switching between Profile Position and
Profile Velocity.

If an application or specific controller needs the transition from
OPERATION ENABLED to SWITCHED ON during operation mode changes, that
should be handled outside this library, and is easy enough to do.  On
the other hand, having it inside the op_mode setter prevents the above
mentioned use-case.
Do not generate a log message about the changed operation mode when it
actually failed.  Use a consistent style for the TypeError message
formatting.
@acolomb acolomb force-pushed the op-mode-switch-efficient branch from 2389661 to 47eeba3 Compare August 12, 2021 20:01
@af-silva af-silva merged commit 8523a68 into canopen-python:master Aug 16, 2021
@acolomb acolomb deleted the op-mode-switch-efficient branch August 17, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants