Suggestion: Avoid `ErrorType`
madsmtm opened this issue · 1 comments
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:ErrorTypeshould be renamedPinsince that is what it logically represents (it is a supertrait ofInputPinandOutputPin). -
embedded_hal::i2c:ErrorTypeshould be removed, and the associatedErrortype should be moved toI2c.The downside is that now the error type is no longer shared between the blocking and the
asyncversion, 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:ErrorTypeshould be removed, and the associatedErrorshould be moved toSetDutyCycle(unless you have other plans for extensions to this module, then it may make sense to rename the trait toPwmor similar instead?) -
embedded_hal::spi: Unsure about this, pending discussion in #525. -
embedded-io: More unsure here, but I suspect theErrorTypedesign is also the wrong design here: an implementation ofReadshould be allowed to have a different error type fromWrite, even though it may be a bit more hazzle to work with types that areRead + Write(users would have to write<T: Read<Error = E> + Write<Error = E>, E>if they wanted to force the same error type).
-
ErrorType is separate even in modules with only 1 trait (like i2c, pwm) to acommodate for future extensions. For example PWM might get a
SetFrequencytrait 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
ErrorTypealways is good for consistency. I wouldn't rename it to e.g.Pinin the GPIO case. Also thePinname might make sense today but it's not guaranteed to make sense with future extensions, whileErrorTypefor 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+Writeis very common, the workaround of adding an extraEgeneric param is very annoying especially because the compiler can't infer it in all cases.