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 embedded-hal 1.0.0-alpha.7 traits #298

Merged
merged 3 commits into from
Feb 26, 2022

Conversation

jannic
Copy link
Member

@jannic jannic commented Feb 22, 2022

Copy link
Member

@9names 9names 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 to me.
Did you manage to find any driver crates that use e-h 1.0 traits?
I spent time adapting some while working on bl602 since it (originally) only supported e-h 1.0 alpha4, but wrangling dependencies sucked.

Also, which of the traits still need testing?
It would be good to have an issue with them so we don't forget to verify everything is working.

@jannic
Copy link
Member Author

jannic commented Feb 26, 2022

No I don't have any drivers using this API, and so all traits need testing.
To be honest, I don't think anybody will use these traits while e-h is still alpha. I just implemented them to try out if there are any issues with the API which make it difficult to work with.
Therefore, merging them is not important. It would be equally fine to just have them on a branch for now, if you are worried about merging untested code.

@9names
Copy link
Member

9names commented Feb 26, 2022

would be equally fine to just have them on a branch for now, if you are worried about merging untested code.

I was more curious than cautious.
No, let's not leave them in a branch. I'm happier with untested code in-tree, as that is way more likely to be noticed and fixed.
And there's not much value in having the alpha traits unless they are the latest version, so I appreciate the effort here.

@9names 9names merged commit 7750781 into rp-rs:main Feb 26, 2022
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.

2 participants