Skip to content

WIP: Update embedded-hal to version 1.0.0-alpha.6 #71

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

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

caemor
Copy link
Contributor

@caemor caemor commented Nov 30, 2021

The delay update is pretty straightforward.

The error update is more difficult: The current implementation creates a new struct including a single element: the upstream linux error, and implement the e-h error trait for it. I've got two issues with this.
Is creating a new struct the best way to do this?
For matching the e-h trait kind: Currently I just matched everything to Other because I couldn't find any good matches when taking a quick glance. Is there something that should be matched?
The creation of a central error enum for linux e-h would be an other alternative way to solve this.

@caemor caemor requested a review from a team as a code owner November 30, 2021 22:54
@rust-highfive
Copy link

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Review is incomplete T-embedded-linux labels Nov 30, 2021
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Nice, thank you for this PR!
The important part would be extracting information from the underlying nix/io errors in the embedded_hal::*::Error implementations so that they can be converted to a meaningful embedded_hal::*::ErrorKind.
Converting all errors to Other would render all drivers unable to act on things like i2c::NoAcknowledge.
For this one needs to dig a bit into the Linux kernel, and so on, though.

As for the error struct itself, in order to be able to implement the traits you already highlighted the possibilities:

  1. Wrapper struct with the error instance.
  2. Wrapper tuple with the error instance.
  3. Central enum.
    3.1. With variants per protocol
    3.2. Flat with all variants in one level

I think 2 looks simpler than 1 but the code would look weirder with the .0s so I think 1 would read better. Whether the inner element should be called err error, inner or something else, I am not sure. Probably any.
I think 3 would be more complex to implement.
3.1. Would definitely more cumbersome to match for the user, as non-relevant variants would need to be matched as well (e.g. spi error variant on i2c operations).
3.2. Seems very unpractical and confusing to match for the user.
All in all, I think the current 2 approach is the right one.

@caemor
Copy link
Contributor Author

caemor commented Dec 14, 2021

If someone else wants to take over, go for it. I am currently missing the time to finish this.

@ryankurte ryankurte added the help wanted Extra attention is needed label Dec 14, 2021
@ryankurte
Copy link
Contributor

ryankurte commented Feb 1, 2022

it doesn't look to me like any of the IoErrorKinds have great correspondence with our ErrorKinds except for I2C, see changes here to match. i'd be happy to merge this then that and keep moving forward, any other opinions @rust-embedded/embedded-linux ?

@eldruin
Copy link
Member

eldruin commented Feb 2, 2022

Ok, Let's merge this then and I will create an issue about improving the match.
@caemor could you resolve the conflict?

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you!
bors r+

@bors
Copy link
Contributor

bors bot commented Feb 2, 2022

@bors bors bot merged commit e7fd628 into rust-embedded:master Feb 2, 2022
@ryankurte
Copy link
Contributor

thanks folks! @eldruin what do you think of another alpha release to keep up with e-h?

@eldruin
Copy link
Member

eldruin commented Feb 2, 2022

Sure. I would prefer doing #74 beforehand, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed S-waiting-on-review Status: Review is incomplete T-embedded-linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants