Skip to content

Add missing aborts and typehints for sdoclient #352

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

samsamfire
Copy link
Contributor

Hi,
I found some missing aborts whilst doing some tests:
in particular, an sdo abort should be sent when a timeout occurs (0x05040000)
an abort should also be sent if the expected command value is not correct (0x05040001).

I also added typehints were possible.

Also, have any of you considered to add a static type checker (mypy,...) and a linting tool ? Could be good in the future

samsamfire and others added 6 commits January 5, 2023 21:39
Also synchronous RPDOs should now be transmitted on sync reception
Need to add more tests for this last part
…#345)

* Fixed incorrect min (LowLImit) and max (HighLimit) values when using signed integer types.
* Fixed min/max for non-signed datatypes and added tests
* Removed type hints
-adding typehints
-adding some missing sdo aborts (timeout, unknown command)
@samsamfire
Copy link
Contributor Author

reverting the PDO change that has nothing to do with this PR

@acolomb
Copy link
Member

acolomb commented Apr 24, 2024

@samsamfire Sorry for not responding for such a long time. I've tried to grasp your changes here in order to see if there's merit in merging it for the next release. However, it is quite convoluted between actual changes and formatting. Would you mind separating those into several smaller PRs or at least split into independent commits?

@acolomb
Copy link
Member

acolomb commented Apr 27, 2025

Keeping this open because the functional change regarding SDO aborts could be worthwhile. But as is, this is too convoluted to act upon. Someone should pick up the gist of it and redo another, focused PR.

@acolomb acolomb added the sdo label Apr 27, 2025
@sveinse
Copy link
Collaborator

sveinse commented Apr 27, 2025

I think this makes sense. Once the formatting and type hints are shaved off, the changes aren't really that big. Getting them stripped down to pure functional changes should be done, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants