Skip to content

Gracefully handle readonly entries in PdoMap.save() (fixes #541) #593

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

Merged

Conversation

acolomb
Copy link
Member

@acolomb acolomb commented Jun 17, 2025

For PDO communication or mapping parameters which are read-only on a
node, the PdoMap.save() method currently aborts with an SDO exception.
However, individual parameters may simply not support configuration
changes. Rely on the Object Dictionary to tell which ones are
expected to fail and skip them. For communication record entries, log
what is not being saved.

erlend-aasland and others added 2 commits June 17, 2025 19:56
For PDO communication or mapping parameters which are read-only on a
node, the PdoMap.save() method currently aborts with an SDO exception.
However, individual parameters may simply not support configuration
changes.  Rely on the Object Dictionary to tell which ones are
expected to fail and skip them.  For communication record entries, log
what is not being saved.
@acolomb
Copy link
Member Author

acolomb commented Jun 17, 2025

This is an alternative approach to #542, with improvements based on the discussion there.

Copy link

codecov bot commented Jun 17, 2025

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
canopen/pdo/base.py 94.44% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@acolomb
Copy link
Member Author

acolomb commented Jun 17, 2025

The one missing line reported by Codecov is related to the hack for supporting Curtis' mixed up mapping entries. I don't feel like putting effort into that part.

@acolomb acolomb added this to the v2.4.0 milestone Jun 18, 2025
@sveinse
Copy link
Collaborator

sveinse commented Jun 22, 2025

I'd like to add something to consider.

I know that this PR is not considering the upcoming asyncio PR yet, but I'd like to inform that the PdoMap.save() and PdoMap.read() will need to be refactored for async. Today the logic and the IO is done together, which doesn't play nice with asyncio. To prevent repeating this logic twice, the asyncio PR refeactors the code following the design principles of sansio, where the IO is done outside of the logic function.

I would have love if we were able to take this into consideration when refactoring PdoMap.save() in thie PR. It would gain us all to think ahead.

@acolomb
Copy link
Member Author

acolomb commented Jun 22, 2025

I'm not at all opposed to refactoring this as needed. However, to move forward quickly, I'd rather limit this PR's scope to fixing the actual unwanted / faulty behavior.

@sveinse
Copy link
Collaborator

sveinse commented Jun 22, 2025

No problem. As a consequence I need to update the asyncio PR to reflect the result of this PR.

Copy link
Collaborator

@sveinse sveinse left a comment

Choose a reason for hiding this comment

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

Are you formatting with black on the new stuff? Because its going a bit back an forth between splitting on the lines and joining them. E.g. base.py L439 is split up, while L450 is collected together.

It's cosmetics. The code changes looks good.

@acolomb
Copy link
Member Author

acolomb commented Jun 22, 2025

Yes, I take black as a reference for how to format new / changed code. In this case, it's simply based on the line length limit of 96 I have configured. The old style was non-conformant in both cases, and the new loop variable allowed some shortening.

@acolomb acolomb merged commit 09f6e9e into canopen-python:master Jun 22, 2025
4 of 5 checks passed
@acolomb acolomb deleted the gracefully-handle-pdo-save-readonly branch June 22, 2025 20:17
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.

3 participants