-
Notifications
You must be signed in to change notification settings - Fork 140
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
Use embedded-hal 1.0 in tests and examples #436
Conversation
a35a228
to
2677ab1
Compare
2677ab1
to
93ee789
Compare
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.
Can you comment on the change here?
The change from:
use embedded_hal::blocking::spi::Write;
to:
use embedded_hal::spi::SpiBus;
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.
The SPI traits changed significantly in embedded-hal
1.0. You can read more about it in the embedded-hal
documentation, but one difference is that there is now a single trait for a bus or a device, rather than separate traits for read vs. write vs. transfer.
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.
Can you please comment on the change of the use of timer and PWM?
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.
What about it? embedded-hal
1.0 no longer includes traits for timers, so we have intrinsic methods instead. The names of some of the PWM methods have changed.
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.
line 46: Can you please comment on the change to make pulldown_pin mutuable?
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.
embedded_hal::digital::InputPin::is_low
takes &mut self
, so it needs to be mutable for the call on line 49.
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.
line 53-54: Can you please comment on the change made from:
(state.input_pin.as_ref().unwrap().is_low().unwrap());
to:
(state.input_pin.as_mut().unwrap().is_low().unwrap());
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.
Again, embedded_hal::digital::InputPin::is_low
takes &mut self
, so we need a mutable reference.
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.
Responses to comments are reasonable as are code changes. Tested on micro:bit V2, ran into some limitations with certain examples being built for other nrf hals for other chips. Overall, looks great! Approved!
No description provided.