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

Suggestion: Avoid ErrorType #526

Open
madsmtm opened this issue Nov 21, 2023 · 1 comment
Open

Suggestion: Avoid ErrorType #526

madsmtm opened this issue Nov 21, 2023 · 1 comment

Comments

@madsmtm
Copy link

madsmtm commented Nov 21, 2023

I agree that the ErrorType trait is somewhat useful, motivation here, but I think that it may actually in most cases be detrimental to the API?

In each of the cases where it is currently used, I think there is a better pattern to use instead, that will be simpler for users to understand, easier for implementers to use, and more flexible and correct.

Let's consider the modules one at a time:

  • embedded_hal::digital: ErrorType should be renamed Pin since that is what it logically represents (it is a supertrait of InputPin and OutputPin).

  • embedded_hal::i2c: ErrorType should be removed, and the associated Error type should be moved to I2c.

    The downside is that now the error type is no longer shared between the blocking and the async version, though I believe this tradeoff is worth it, since it is very rare that you'd be working with both traits at the same time.

  • embedded_hal::pwm: ErrorType should be removed, and the associated Error should be moved to SetDutyCycle (unless you have other plans for extensions to this module, then it may make sense to rename the trait to Pwm or similar instead?)

  • embedded_hal::spi: Unsure about this, pending discussion in Unify embedded_hal::spi traits? #525.

  • embedded-io: More unsure here, but I suspect the ErrorType design is also the wrong design here: an implementation of Read should be allowed to have a different error type from Write, even though it may be a bit more hazzle to work with types that are Read + Write (users would have to write <T: Read<Error = E> + Write<Error = E>, E> if they wanted to force the same error type).

@Dirbaio
Copy link
Member

Dirbaio commented Nov 25, 2023

  • ErrorType is separate even in modules with only 1 trait (like i2c, pwm) to acommodate for future extensions. For example PWM might get a SetFrequency trait in the future. For I2C it's harder to imagine what an extension could be, but it's still nice to leave the door open.

  • Naming it ErrorType always is good for consistency. I wouldn't rename it to e.g. Pin in the GPIO case. Also the Pin name might make sense today but it's not guaranteed to make sense with future extensions, while ErrorType for sure will.

  • more hazzle to work with types that are Read + Write (users would have to write <T: Read<Error = E> + Write<Error = E>, E> if they wanted to force the same error type).

    this is exactly why the error type is shared. Using Read+Write is very common, the workaround of adding an extra E generic param is very annoying especially because the compiler can't infer it in all cases.

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

No branches or pull requests

2 participants