Skip to content

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.

# Now apply all other records in object dictionary
for obj in self.object_dictionary.values():
if isinstance(obj, ODRecord) or isinstance(obj, ODArray):
if "PDO" in obj.name:
Copy link
Member

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 ?

Copy link
Member

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
Member

@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
Member

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?

Copy link
Member

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.

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.

@meifonglow meifonglow force-pushed the pr-load-dcf-config-to-remote-node branch from 8bc8bbc to a18acac Compare May 28, 2024 02:54
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.74%. Comparing base (02ebc0c) to head (8cd3914).

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #427      +/-   ##
==========================================
- Coverage   67.85%   67.74%   -0.11%     
==========================================
  Files          26       26              
  Lines        3108     3113       +5     
  Branches      525      527       +2     
==========================================
  Hits         2109     2109              
- Misses        859      864       +5     
  Partials      140      140              
Files Coverage Δ
canopen/pdo/base.py 63.34% <0.00%> (-0.35%) ⬇️
canopen/node/remote.py 53.08% <0.00%> (-2.05%) ⬇️

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.

4 participants