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

Implement reading PDO from OD defaults (#273) #370

Merged

Conversation

sveinse
Copy link
Contributor

@sveinse sveinse commented Apr 8, 2023

A small improvement based on discussion in #273 that allows reading the PDO configuration from the object dictionary defaults rather than requesting them from the remote node.

@samsamfire
Copy link
Contributor

Hi, I did something similar on my side for two main reasons (just putting this for explanation):

  • When using a large OD, reading via SDO can be quite long : ~5 seconds. When you are testing and just running scripts, this can be really annoying.
  • Sometimes you would like to use PDOs but don't want to use SDO as this can break communication if there is already a master on the bus talking to the node.

Anyway, cool improvement !

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.

Nice addition. Only one nit-pick comment.

"""Read PDO configuration for this map using SDO."""
cob_id = self.com_record[1].raw

def _raw_from(param):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need an underscore name here? The function is only defined within this scope, so there is nothing to be gained by trying to hide it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not really. Only a convention. My habit is to name functions that are defined within a fn or meth using a underscore prefix, but it is not necessity.

@acolomb
Copy link
Collaborator

acolomb commented May 2, 2023

And I think this needs documentation?

@christiansandberg christiansandberg merged commit 5418054 into christiansandberg:master Sep 1, 2023
1 check passed
@sveinse sveinse deleted the feature-read-pdo-from-od branch April 26, 2024 01:04
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

4 participants