8000 Fix canmatrix export by walmis · Pull Request #287 · canopen-python/canopen · GitHub
[go: up one dir, main page]
Skip to content

Fix canmatrix export #287

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.

Sign up for GitHub

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

Jump to bottom
Merged
acolomb merged 2 commits into from
Dec 21, 2021
Merged

Fix canmatrix export #287

acolomb merged 2 commits into from
Dec 21, 2021

Conversation

@walmis
Copy link
Contributor
@walmis walmis commented Dec 9, 2021

Exporting PDO mappings seems to be broken with canmatrix >= 0.9 (haven't checked exactly where it broke).
This PR should fix this.

acolomb
acolomb requested changes Dec 9, 2021
View reviewed changes
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 from the code, I haven't tested anything here.

@walmis
Copy link
Contributor Author
walmis commented Dec 10, 2021

Updated to use add_frame method

@acolomb
Copy link
Member
acolomb commented Dec 14, 2021

This PR should fix this.

Have you done any testing?

Sorry I have never used such functionality and don't see when I'll have time to dig deeper into testing it. I agree that it was broken before, so we can't make it much worse. Just would like to hear that the changes have actually been tested on a real world scenario.

@walmis
Copy link
Contributor Author
walmis commented Dec 21, 2021

This PR should fix this.

Have you done any testing?

Sorry I have never used such functionality and don't see when I'll have time to dig deeper into testing it. I agree that it was broken before, so we can't make it much worse. Just would like to hear that the changes have actually been tested on a real world scenario.

Yes, it exported the mapping as expected. It works as far as it doesn't throw a unknown method exception.

@acolomb
Copy link
Member
acolomb commented Dec 21, 2021

Merging as it cannot realistically make things worse than not working at all.

@acolomb acolomb merged commit 89bc634 into canopen-python:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acolomb acolomb acolomb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@walmis @acolomb
0