Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Embedded error #218

Closed
wants to merge 6 commits into from
Closed

Embedded error #218

wants to merge 6 commits into from

Conversation

therealprof
Copy link
Member

No description provided.

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@@ -294,7 +294,7 @@ macro_rules! gpio {
}

impl<MODE> OutputPin for $PXx<Output<MODE>> {
type Error = Infallible;
type Error = GpioError;
Copy link
Contributor

Choose a reason for hiding this comment

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

For the other errors, I see the advantages, but replacing infallible by some non empty error, that's not great. I personally into_ok (a personal implementation for stable) on these errors, and it would not be possible after this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't make it uniform then we can't use this universally with drivers and applications, this is just a preview of what's to come in embedded-hal. There're plenty of cases like on a Raspberry Pi or behind GPIO expanders where GPIO is not infallible.

What is into_ok? And how is it different from .ok()? I don't think applications will change a lot but there might be a slight size overhead (not sure, really, Infallible is also not quite ideal). If you have any ideas for improvement, I'd be all ears.

Copy link
Contributor

Choose a reason for hiding this comment

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

In which case would you need to know the error? Can you point on real example? I suppose some intermediate driver between embedded_hal and another driver where you have to manage some kind of error?

https://doc.rust-lang.org/core/result/enum.Result.html#method.into_ok and my "waiting to stabilise implementation" https://github.com/TeXitoi/keyseebee/blob/master/firmware/src/main.rs#L32-L42

That's an infallible .ok().unwrap(). As I know I don't have to manage error, I can remove the complexity of managing them for nothing. And it won't be possible anymore with this modification.

Maybe we can have Error: Into<GpioError> and Infallible can implement this conversion. But it will complexify the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

In which case would you need to know the error? Can you point on real example? I suppose some intermediate driver between embedded_hal and another driver where you have to manage some kind of error?

Yes, for example. No, I don't have an example yet but support for fallible GPIOs has been requested a few times and was incorporated into embedded-hal (aka the digital::v2 debacle).

Maybe we can have Error: Into and Infallible can implement this conversion. But it will complexify the interface.

When we change the Error types in embedded-hal the bounds will be Into<GpioError> + Clone + Debug. I don't think we can implement Into<GpioError> for Infallible but if you have any idea how to solve that your input would certainly be appreciated over at rust-embedded/embedded-hal#229.

Copy link
Contributor

Choose a reason for hiding this comment

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

Implementing Into for Infallible is easy: you just have to implement From, that you should implement if possible instead of Into anyway: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=86a6dabe6ccc820f756c03718d5e4cc4

Will try to look at the linked issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do a PR for implementing From for *Error

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be careful with doing that for the other Error. Many (most?) of the other peripherals are impossible to implement correctly in an Infallible way and I'd be wary to add a loophole to avoid a correct implementation out of the gate; we can always add it later if it makes sense.

Copy link
Member

@nickray nickray Sep 29, 2020

Choose a reason for hiding this comment

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

If I'm not mistaken, only embedded-error can implement From<Infallible> for its types, and at least mathematically/logically, this conversion should exist (and there's only one implementation "up to canonical isomorphism"...). So I think the implementation should be added, with warnings about (peripheral) implementation correctness, no?

Copy link
Member Author

@therealprof therealprof Sep 29, 2020

Choose a reason for hiding this comment

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

@nickray Yes, adding it to embedded-error and using Infallible at least for GPIO is what we were talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made a different proposition: rust-embedded/embedded-hal#229 (comment)

I'll wait a bit the feedbacks before doing a PR to embedded-error, if you don't mind.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants