search

rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.01k stars 202 forks source link

require `core::error::Error` for `ErrorType::Error` #633

Open jordens opened 1 month ago

jordens commented 1 month ago

As of #630 the ErrorKind structs implement Error. As explained in https://github.com/rust-embedded/embedded-hal/pull/630#issuecomment-2343945572 its difficult to make good use of that unless the e-h traits add a core::error::Error bound to their ErrorType::Error type: A device driver that uses a e-h trait wants to return the hal's Error from its impl of core::error::Error::source() -> Option<&dyn core::error::Error>. Returning the ErrorKind is hard/annoying/inefficient due to the need to store it. The "good" way to do that is appears to either require core::error::Error for the hal's Errors or for each device driver to implement newtype wrappers for any hal Error (which are then limited as they can only rely on Error::kind() and Debug). Adding that bound is obviously breaking. But the burden is manageable: (1) the MSRV bump is already there (2) a trivial impl of core::error::Error will always be sufficient and in most cases even complete (3) thiserror and other crates make the job easy.

Dirbaio commented 1 month ago

As you mention, this is a breaking change so it'd have to be released as embedded-hal v2.0.

Even if upgrading 1.0->2.0 is easy, the real burden is the ecosystem split it causes. A HAL implementing v2.x won't work with a driver requiring v1.x. There's many lightly-maintained or unmaintained HALs/drivers, we've seen this with 0.2->1.0. Almost 1 year after the release of v1.0 there's quite a few HALs and a lot of drivers that still haven't upgraded to 1.0.

This was discussed in a WG meeting when merging #630, the conclusion was this is something we do want if we do 2.0, but it alone it's not worth the burden of doing a 2.0 release.

jordens commented 1 month ago

Maybe adding the adapters and a trait for those in a separate crate is a non-terrible work around.

jordens commented 1 month ago

https://github.com/quartiq/embedded-hal-error/blob/main/src/lib.rs

jordens commented 1 month ago

FTR and AFAICT this wasn't discussed when merging but two weeks ago.

Also note that this situation is different from the 0.2-1.0 breakage. There seem to be two other options here:

  1. Strongly recommend HALs implement core::error::Error. If/when/once most HALs do, drivers can just demand that explicitly.
  2. Introduce purely additive subtraits that demand core::error::Error and blanket impls. That way HALs don't need to implement multiple e-h traits and drivers can transition smoothly. It is a bit inflationary in terms of traits and doesn't look so nice.

(To be clear: I also think that impl core::error::Error for ErrorKind is correct and good. And I don't want another trait revolution, at least not now.)

ryankurte commented 1 month ago

Strongly recommend HALs implement core::error::Error. If/when/once most HALs do, drivers can just demand that explicitly.

this seems pretty reasonable to me. no weird trait tricks, it's a nice way to push for gradual adoption, and per @Dirbaio's point the strict requirement can go in a pile for a possible (one day, maybe) 2.0.

jordens commented 1 month ago

What's the opinion on the following steps?

  1. Adding the core::error::Error recommendation to the e-h trait docstrings
  2. Moving embedded-hal-error into embedded-hal (just into the repo or even into the crate)