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

Updating to embedded-hal digital::v2 #116

Merged
merged 5 commits into from
Oct 18, 2019
Merged

Conversation

careyk007
Copy link
Contributor

I have updated the GPIO pins to use the embedded-hal v2 traits. I only have an Adafruit nRF52832 board so I haven't been able to test other chips, but since most of the changes were in nrf52-hal-common I am confident that this will work for other chips as well.

@@ -62,7 +62,8 @@ use crate::target::P0;
#[cfg(feature = "52840")]
use crate::target::{ P1 };

use crate::hal::digital::{OutputPin, StatefulOutputPin, InputPin};
use crate::hal::digital::v2::{OutputPin, StatefulOutputPin, InputPin};
use core::convert::Infallible;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does using this increase our MSRV? IIRC this came in in 1.34.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. What is the MSRV you are targeting, and would reworking to use an error type of () instead of Infallible be a solution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the MSRV is, but if Infallible needs to be replaced, it should be replaced with void::Void, which is already a dependency of this crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I went ahead and replaced Infallible with void::Void.

@hannobraun
Copy link
Contributor

Thank you for the pull request, @careyk007!

I have one more nit-pick: You added embedded-hal as a direct dependency to some crates, to import the new traits from there. I think it would be better to add the new traits to the prelude (nrf52-hal-common/lib.rs) and import them through that.

Other than that, I think this is good to merge!

@careyk007
Copy link
Contributor Author

Happy to help! Thanks for the great feedback!

@hannobraun
Copy link
Contributor

Thanks for the changes! Travis is failing. I think you're missing an import somewhere.

Also, I was suggesting to re-export the digital::v2 traits from the prelude module in lib.rs, but that can also be done in a later step. I think this is good to merge as soon as the CI build is green.

@careyk007
Copy link
Contributor Author

Shoot! I'll look into the Travis issue. I can also re-export digital::v2 traits from prelude, that seems like a much nicer approach.

@hannobraun
Copy link
Contributor

Looks good to, thanks!

Also see #117 (comment).

@hannobraun hannobraun merged commit 2432da0 into nrf-rs:master Oct 18, 2019
jonas-schievink added a commit to ferrous-systems/embedded-trainings that referenced this pull request Oct 28, 2019
This is the simplest solution until a new HAL version is released that
includes nrf-rs/nrf-hal#116.
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.

3 participants