-
Notifications
You must be signed in to change notification settings - Fork 223
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
Upgrading spi to embedded_hal 1.0 #517
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for picking up the changeset from the other PR. How much did you test this on real hardware so far?
Regarding the code, I only have one topic left to discuss, see below.
I implemented the change by using Now that I re-read the code, these function names are confusing I understand this doesn't concern the end user of the library but it makes it confusing to understand by the maintainers. I'll give this some rework and update accordingly. |
Ok, I've reworked the code, it should be functionally the same just slightly more clear. I also moved the comments to the SpiOps definition so they would show up when hovering over the functions. |
The code was tested/re-tested with an Uno sending/receiving packets using the nrf24l01 module, register readings and transmission were successful. The functions directly used were |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good, thanks a lot!
This is my follow up to #503, with your suggestions, since I needed the new Trait for a library I'm using. Perhaps we can continue to refine the implementation.
This might be good progress towards #468.