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

digital v2 #93

Merged
merged 2 commits into from
Aug 4, 2019
Merged

digital v2 #93

merged 2 commits into from
Aug 4, 2019

Conversation

burrbull
Copy link
Member

@burrbull burrbull commented Aug 2, 2019

No description provided.

Copy link
Member

@TheZoq2 TheZoq2 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! If I understand this correctly, all an end user has to do to use this version with a v1 driver is what's done in examples/mfrc522.rs? Perhaps it would be a good idea to add that to the docs to make things less confusing for new users.

Apart from that, and the note about the never type, I'd say this looks good to merge

src/gpio.rs Outdated
@@ -143,49 +143,52 @@ macro_rules! gpio {
}

impl<MODE> OutputPin for $PXx<Output<MODE>> {
fn set_high(&mut self) {
type Error = ();
Copy link
Member

Choose a reason for hiding this comment

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

I think I read somewhere that the prefered error type for a non-failable pin is ! rather than (). Though the never type isn't fully implemented iirc

@burrbull
Copy link
Member Author

burrbull commented Aug 2, 2019

Perhaps it would be a good idea to add that to the docs to make things less confusing for new users.

My English not good enough to write documentation.

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 2, 2019

My English not good enough to write documentation.

No problem. I was thinking something like this

Interfacing with v1 traits

embedded-hal has two versions of the digital traits, v2 which is used by this crate and v1 which is deprecated but still used by a lot of drivers. If you want to use such a driver with this crate, you need to convert the digital pins to the v1 type.

This is done using embedded-hal::digital::v1_compat::OldOutputPin. For example:

let nss = gpioa.pa4.into_push_pull_output(&mut gpioa.crl);
let mut mfrc522 = Mfrc522::new(spi, OldOutputPin::from(nss)).unwrap();

@burrbull
Copy link
Member Author

burrbull commented Aug 2, 2019

Do I need to add this in README or only in docs?

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 2, 2019

Docs should be enough I think

@TeXitoi
Copy link
Contributor

TeXitoi commented Aug 2, 2019

Maybe using the void crate for the error instead of ()?

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 2, 2019

Yea, that might not be a bad idea, especially since we already depend on it.

What are the problems with using ! here?

@TeXitoi
Copy link
Contributor

TeXitoi commented Aug 2, 2019

I think ! is not usable at this position on stable.

@burrbull
Copy link
Member Author

burrbull commented Aug 2, 2019

Done. Rebased.

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 2, 2019

I think it might be better to put the new docs in the digital module since that's probably where people will look when they run into issues

@burrbull
Copy link
Member Author

burrbull commented Aug 2, 2019

What digital module you mean? gpio maybe?

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 2, 2019 via email

@burrbull
Copy link
Member Author

burrbull commented Aug 2, 2019

Done.

src/gpio.rs Outdated
!self.is_low()
type Error = Void;
fn is_high(&self) -> Result<bool, Self::Error> {
Ok(!(self.is_low()?))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why this snippet differs from all the other places ?
Why not the same here:

self.is_low().map(|b| !b)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@geomatsi geomatsi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 4, 2019

It looks like examples/blinky_rtc.rs is missing unwrap, but other than that, this looks good to merge

@burrbull
Copy link
Member Author

burrbull commented Aug 4, 2019

Fixed.

@TheZoq2
Copy link
Member

TheZoq2 commented Aug 4, 2019

Perfect, thanks!

@TheZoq2 TheZoq2 merged commit 33d175c into stm32-rs:master Aug 4, 2019
@burrbull burrbull deleted the v2 branch August 4, 2019 17:22
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.

4 participants