-
Notifications
You must be signed in to change notification settings - Fork 182
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
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
:ErrorType
should be renamedPin
since that is what it logically represents (it is a supertrait ofInputPin
andOutputPin
).embedded_hal::i2c
:ErrorType
should be removed, and the associatedError
type should be moved toI2c
.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 associatedError
should be moved toSetDutyCycle
(unless you have other plans for extensions to this module, then it may make sense to rename the trait toPwm
or similar instead?)embedded_hal::spi
: Unsure about this, pending discussion in Unifyembedded_hal::spi
traits? #525.embedded-io
: More unsure here, but I suspect theErrorType
design is also the wrong design here: an implementation ofRead
should 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).The text was updated successfully, but these errors were encountered: