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 IoPin #256

Merged
merged 3 commits into from
Nov 7, 2021
Merged

Implement IoPin #256

merged 3 commits into from
Nov 7, 2021

Conversation

olback
Copy link
Member

@olback olback commented Oct 6, 2021

Implements the IoPin trait from embedded-hal.

src/gpio.rs Outdated
Ok(self)
}
fn into_output_pin(mut self, state: PinState) -> Result<Self, Self::Error> {
self.set_state(state)?;
Copy link
Member

Choose a reason for hiding this comment

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

I see here the problem that if setting the state fails, the pin is dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is something I did not think about. I mostly copied all the implementations from stm32-rs/stm32f4xx-hal where they use into_push_pull_output_in_state which would solve this I think.

Copy link
Member

Choose a reason for hiding this comment

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

The set_low and set_high methods implemented here are infallible (use the Never type as an error), so set_state would also be infallible.

Maybe we should implement our own infallible set_state (as stm32f4xx-hal does) and use that?

Copy link
Member

@richardeoin richardeoin left a comment

Choose a reason for hiding this comment

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

I'd not seen the IoPin trait yet, but it's certainly interesting.

This PR should also bump the embedded-hal version in Cargo.toml to 0.2.6, since 0.2.{4,5} would no longer be supported.

@olback
Copy link
Member Author

olback commented Oct 7, 2021

This PR should also bump the embedded-hal...

Should the other outdated dependencies also be updated?

@richardeoin
Copy link
Member

No, the issue is that with this change the crate would no longer compile with embedded-hal 0.2.4. Therefore it's necessary to increase the minimum patch version (3rd number) so Cargo explicitly knows it can't use 0.2.4 but has to download 0.2.6 (or 0.2.7, 0.2.8 ...). For other dependencies that have a new major or minor version, it's good to update them but probably in a separate PR.

If there's a good solution to avoid ever returning Err from the into_output_pin methods, then I think we can get this merged

@olback
Copy link
Member Author

olback commented Oct 30, 2021

Would that really be necessary though? set_state never fails, it's just the trait impl that requires a Result.

@richardeoin
Copy link
Member

Hmm, yes there is the type Error = Never; in the implementation. I would still replace Self::Error with Never for consistency with every other implementation in this file, and even replace the ? with an .unwrap() to make completely clear that this never fails.

@olback
Copy link
Member Author

olback commented Oct 30, 2021

Somewhat unrelated, is there a reason for using enum Never {} over core::convert::Infallible?

@richardeoin
Copy link
Member

Looks good!

My best guess is that enum Never was the solution before convert::Infalliable was stabilised, and there was an expectation that convert::Infalliable would soon be replaced with the actual ! never type. However stabilisation of ! seems to have hit some roadblocks, and enum Never works ok, so it's not been changed.

@richardeoin
Copy link
Member

bors r+

@bors bors bot merged commit cee3b0e into stm32-rs:master Nov 7, 2021
@olback olback deleted the embedded-iopin branch November 8, 2021 15:04
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