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

Add support for loading DCF configuration to remote node #427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

meifonglow
Copy link
Contributor

As part of instrument commissioning process, I want to apply the configuration values stored in a DCF to a remote node, including PDO mapping and communications parameters. If a value was not specified, then the EDS default value is used instead.

This commit provides this capability.

Potentially addresses #274.

for obj in self.object_dictionary.values():
if isinstance(obj, ODRecord) or isinstance(obj, ODArray):
if "PDO" in obj.name:

Choose a reason for hiding this comment

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

Perhaps it is more safe to use the indices rather than the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean explicitly ignoring objects with indices within supported PDO ranges, i.e: 0x1400 <= obj.index < 0x1C00 ?

Choose a reason for hiding this comment

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

Yes exactly. I'm thinking using names might yield both false positives and false negatives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks that makes sense.
I've also removed excluding objects with "COB-ID" keyword. They are being applied OK by __load_configuration_helper() and would have been incorrectly ignored in previous commit.

@meifonglow meifonglow force-pushed the pr-load-dcf-config-to-remote-node branch 2 times, most recently from c406d3e to b9534aa Compare May 26, 2024 05:00
Copy link
Collaborator

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a couple of style nit-picks.

if isinstance(obj, ODRecord) or isinstance(obj, ODArray):
if 0x1400 <= obj.index < 0x1c00:
# Ignore PDO related objects
logger.debug(f"{obj.index:#06x}: {obj.name} already applied by pdo.save()")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger.debug(f"{obj.index:#06x}: {obj.name} already applied by pdo.save()")
logger.debug("0x%04X: %s already applied by pdo.save()", obj.index, obj.name)

See #443. Do we really need this much debug logging though?

Choose a reason for hiding this comment

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

Agree, it is something that would be logged every time so it may not add much value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Removing.

canopen/pdo/base.py Outdated Show resolved Hide resolved
if isinstance(obj, ODRecord) or isinstance(obj, ODArray):
if 0x1400 <= obj.index < 0x1c00:
# Ignore PDO related objects
logger.debug(f"{obj.index:#06x}: {obj.name} already applied by pdo.save()")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Removing.

canopen/node/remote.py Outdated Show resolved Hide resolved
canopen/node/remote.py Outdated Show resolved Hide resolved
@meifonglow meifonglow force-pushed the pr-load-dcf-config-to-remote-node branch from 8bc8bbc to a18acac Compare May 28, 2024 02:54
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.

None yet

3 participants