[RFC] Revamp `Error` types

[therealprof]

As demonstrated by #181 and other issues there's a real world usability issue with the Error type of the Result of operations being an associated type. An associated type means that a driver or user of an HAL impl is required to adapt to the custom Error type; while possible for applications this makes universal drivers impossible.

In my estimation the main reason for this was probably caused by embedded-hal predating the improvements to Rust type and specifically Error handling. Now we do have better possibilities and with the upcoming big 1.0 breakage it should be possible to also revamp the Error types without too much grief.

Currently our trait definition e.g. for I2C looks as follows:

pub trait Read {
    /// Error type
    type Error;
    ...
    fn try_read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Self::Error>;
}

My proposal would be to instead provide a specific Error type such as:

#[non_exhaustive]
pub enum Error {
    /// An unspecific bus error occured
    BusError,
    /// The arbitration was lost, e.g. electrical problems with the clock signal
    ArbitrationLoss,
    /// A bus operation received a NACK, e.g. due to the addressed device not being online
    NoAcknowledge,
    /// The peripheral buffer was overrun
    Overrun,
    /// The peripheral buffer ran out of data
    Underrun,
}

and then redefine all traits as:

pub trait Read {
    fn try_read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error>;
}

The #[non_exhaustive] flag (available from Rust 1.40.0, cf. https://blog.rust-lang.org/2019/12/19/Rust-1.40.0.html) allows us to easily extend the reasons in the feature without being a breaking change because it requires implementations which care about the actual error to also have a wildcard match branch, which is a good idea anyway.

By having the Error types defined in embeded-hal we do some advantages:

• Drivers can now universally check the error kind reported by a HAL implementation (cf. I²C Polling (detecting NACKs) #181 where a chip might return a NACK due to currently not being able to handle more requests, yet the driver cannot make use of this information and retry later) and act accordingly
• More uniform and exhaustive documentation of available error conditions next to the API itself
• Better guidelines for HAL authors which cases might be work distinguishing
• Improved portability of embedded applications between different hardware

Dowsides:

• More breakage right now
• MRSV 1.40

[ryankurte]

Yeah #181 is an annoying case, and it would definitely be nice to improve the error handling situation. From quick read / chat I have a few of thoughts:

• Is this a case for any drivers other than I2C?
• #[non_exhaustive] looks like a super useful thing, is this an acceptable bump in MSRV? (seems okay to me)
• How do we communicate underlying errors (for example, a network failure with an I2C-over-ethernet bridge) without erasing useful information?
  • In the standard flat-error case I believe this is usually achieved via boxing inner types
  • If using the e-h error types is optional we create a green/blue driver situation

In my estimation the main reason for this was probably caused by embedded-hal predating the improvements to Rust type and specifically Error handling

I might not be totally up to date on this but as far as I am aware std::error::Error is not exposed in core (rust-lang/rust#33149), and there was intent to move it to alloc which also would seem unsuitable. It'd be great to pick up something like anyhow but even the no_std support for this would appear to require a global allocator, so AFAIK the best options we have are failure ?, ? and enums.

There are a couple of possible alternatives that would solve the i2c problem (though could be used in other similar cases) without erasing types / require global allocation:

A generic error type with NACK

#[non_exhaustive]
pub enum Error<E> {
    /// An unspecific bus error occured
    BusError<E>,
    /// The arbitration was lost, e.g. electrical problems with the clock signal
    ArbitrationLoss,
    /// A bus operation received a NACK, e.g. due to the addressed device not being online
    NoAcknowledge,
    ...
}

pub trait I2cRead {
    /// Error type
    type Error;
    ...
    fn try_read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error<Self::Error>>;
}

This would work now, allows drivers to handle the i2c appropriate errors, and is consistent with nb traits and other things, though is somewhat unwieldy.

An is_nack trait and bound (or more generic I2cError if there are other methods) on the I2C Error Type

pub trait I2cError {
  fn is_nack(&self) -> bool;
  ...
}

pub trait I2cRead {
    /// Error type
    type Error: I2cError;
    ...
    fn try_read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error<Self::Error>>;
}

This requires Generic Associated Types (much like the work on async wrappers), so, isn't yet stable / available. It'd be neat to combine this with `core::error::Error` but, 

I'm not really sure what the best option here would be, I prefer the latter but it's not currently available and a future change to this would be breaking, and whatever we do propose I think we should demonstrate reasonably thoroughly in the same manner as hal traits?

[eldruin]

A real error type for I2C sounds like a good improvement.

FWIW, the only error that I have actually needed is NoAcknowledge due to the exact same issue described in #181 (in the eeprom24x driver waiting for a write to finish). I also remember seeing the possibility for I2C polling in some other device but I cannot remember exactly which one it was.
With this I could transform NoAcknowledge into nb::Error::WouldBlock and bubble up anything else, so that would be great.

Regarding the suitability of a similar Error type for other protocols than I2C, I am unsure. I would have said I2C is the only "real" protocol that e-h supports which has errors of its own and not just kind of "hand off some bits, no strings attached" :)
Nevertheless, maybe someone here has more experience on this and can think of some errors for SPI or so.

My biggest concern with the OP proposal is the point about transportation of underlying errors as @ryankurte already mentioned. I think the BusError<E> is a fair mechanism.
An alternative I could also think of would be an additional Other<E> instead:

#[non_exhaustive]
pub enum Error<E> {
    /// An unspecific bus error occurred
    BusError,
    // ...
    /// Something else happened
    Other<E>,
}

This would distinguish itself from something which is wrong with the bus. However, I think this is not a great solution:

• I would need to see examples of BusError but I would have said they should be quite rare/unclear and I can imagine somebody wanting to add more information to BusError as otherwise this gives zero guidance on an error recovery strategy.
• Two variants BusError<E1> and Other<E2> seem like an overkill and not worth the additional parameter type.

That is why I think that BusError<E> is a fair solution offering some extensibility.

Of course, (at least at first sight) I like the GAT version from @ryankurte the most due to the transparent extensibility but we have been waiting for GAT for a long time and I think I would rather not do that.

I am also a bit concerned about the MSRV bump but I personally have no need for an older compiler. I would say #[non_exahustive]'s avoidance of future breaking changes is definitely worth it.

[therealprof]

Thanks for your responses. I don't have time to read them all in detail right now but here's one paragraph which stuck out in the notifcation going by:

Regarding the suitability of a similar Error type for other protocols than I2C, I am unsure. I would have said I2C is the only "real" protocol that e-h supports which has errors of its own and not just kind of "hand off some bits, no strings attached" :)
Nevertheless, maybe someone here has more experience on this and can think of some errors for SPI or so.

I can see other applications as well. For instance if you have an application running e.g. over a UART it would be very helpful being able to see buffer overruns so a driver could start over. For GPIO (where we're discussing things like tri-state GPIOs and changing directions) it might be useful to know that a pin is in a wrong mode for some operation.

But even other than in drivers, whenever you're doing real error handling in an application (other than the unfortunately common plain unwrapping) this is currently not at all portable so whenever you want to run an application on a different MCU and properly handle different error situations you'd have to write MCU specific error handling -- which is not great.

[dbrgn]

I only found this RFC now, but independently suggested the same solution as @ryankurte / @eldruin:

#181 (comment)

I think it's important that the implementation can provide custom errors if it wants to, but that we can have implementation-independent error variants for things like Nack / ArbitrationLoss / etc so that I²C device drivers can use polling instead of fixed sleep times (when waiting for something to be processed).

This should probably be decided before the 1.0.0 release, right?

[therealprof]

I think it's important that the implementation can provide custom errors if it wants to, but that we can have implementation-independent error variants for things like Nack / ArbitrationLoss / etc so that I²C device drivers can use polling instead of fixed sleep times (when waiting for something to be processed).

Do you have an example for a custom error which would be worth the additional complexity of generic parameter? If we say we would like to entertain the idea of custom errors I'd rather see something like a Custom enum type with a static error string but TBH if we have non-exhaustive errors we can extend them any time if the need arises and everyone gets to profit. Otherwise you'd still have the same handling mess as now and people could decide to rather keep their own proprietary error types instead of shared error types.

This should probably be decided before the 1.0.0 release, right?

Yes.

[dbrgn]

From looking through the different HALs, error types that exist right now:

• STM32L0: Overrun, Nack, PECError, BusError, ArbitrationLost
• STM32L1: OVERRUN, NACK
• STM32L4: Bus, Arbitration, Nack
• STM32F0: OVERRUN, NACK, BUS
• STM32F1: Bus, Arbitration, Acknowledge (Nack), Overrun
• STM32F3: Bus, Arbitration
• STM32F4: OVERRUN, NACK
• STM32F7: Bus, Arbitration, Acknowledge, Overrun, Busy
• STM32G0: Overrun, Nack, PECError, BusError, ArbitrationLost
• nRFx (TWIM): TxBufferTooLong, RxBufferTooLong, Transmit, Receive, DMABufferNotInDataMemory
• LPC8xx: EventTimeout, MasterArbitrationLoss, MasterStartStopError, MonitorOverflow, SclTimeout
• Linux: Nix(nix::Error), Io(io::Error)
• TM4C: Bus, Arbitration, DataAck, AdrAck

I don't know enough about the different implementations (and SMBus, which seems to add some other error types) to know what all those variants mean, but it feels a bit wrong to throw away error information for error types that we as embedded-hal developers did not think of, or where platform-specific error handling could be added. Maybe some people with more experience can judge that.

[therealprof]

I'm not proposing to throw away error types at all, my proposal is to add all error types (which make sense in some form) into the shared error type. In fact most of the HAL impls only have enum kinds for the errors they're detecting right now, they could provide more of them...

[therealprof]

I also should add that unlike for std applications the focus here should be on the ability to distinguish and handle different kinds of errors in a useful manner. Presentation of an error to a user requires a bespoke implementation implementation anyway so investing extra effort to provide as much detail in the errors as possible is actually counterproductive.

[ryankurte]

hmm, some other things I didn't really consider in the last reply:

• is there a difference between a recoverable-error and a non-recoverable error?
• is a NACK actually an error, or, normal part of bus interaction?
• could this normal-failure be represented in a clearer way on the interface? (Result<Option<...>, Self::Error> or I2cResult::Ok|Nack|Whatever|Err as crude examples)
  • are there other errors that are perhaps, not errors, or is this the only case (impacting whether this is viable)?
  • would an API change of this nature resolve both the specific (i2c) and general case problems in a satisfactory way
• should retry etc. (which is part of the justification for non-NACK errors) be driver or application level (and is that a decision we can/should make here)

I totally agree that we are missing some API detail, but am still not swayed by the exhaustive (yet non-exhaustive) enum approach. Aside from the other complaints it seems like introducing a bunch of application detail into an otherwise abstract crate and I don't know how we would choose to accept or not-accept enum variants.

[ryankurte]

Oh wait, the trait bound is only blocked on GATs where lifetimes are involved (ie. with futures and buffers), so you can pretty trivially implement the second of the options in #229 (comment) on stable.

Demonstrated in embedded-hal and stm32f4-hal, should probably try with a consumer / example but, it's time for bed here.

[therealprof]

• is there a difference between a recoverable-error and a non-recoverable error?

As far as I am concerned it's up to the application to decide what is recoverable and what is not.

is a NACK actually an error, or, normal part of bus interaction?

Both. Originally it was supposed to signal that a device does not exist on the bus but nowadays is sometimes is used to signal that the device is busy.

could this normal-failure be represented in a clearer way on the interface?

You do have a strange interpretation of "clearer"... 😅

should retry etc. (which is part of the justification for non-NACK errors) be driver or application level (and is that a decision we can/should make here)

I think both. E.g. if you have a driver talking over some interface and it is waiting for a certain stream of data to arrive and the HAL signals an overrun or a jammed line, the driver might be able to retry the last communication in order to fix the issue. If you don't have a driver but your application is handling the data directly you similarly might want to be able to handle such situations (which is currently not possible in a generic way).

totally agree that we are missing some API detail, but am still not swayed by the exhaustive (yet non-exhaustive) enum approach. Aside from the other complaints it seems like introducing a bunch of application detail into an otherwise abstract crate

I absolutely disagree. Having a common set of error types for general use is one the achievements of the latest iterations of e.g. libstd. I think it is fundamental for portable and composable software to have a common set of error kinds to rely on and that's also the reason why custom approaches like failure are now deprecated.

and I don't know how we would choose to accept or not-accept enum variants.

Easy: if it makes sense, is not covered by an existing error kind we'll accept it. There should be a very low bar to including new error kinds and since additions are non-breaking I would not worry about that at all.

[therealprof]

Demonstrated in embedded-hal and stm32f4-hal, should probably try with a consumer / example but, it's time for bed here.

I don't think that solves any of the issues and adds more convolution to the API.

[therealprof]

@ryankurte @eldruin So I was looking into your proposal to have a generic custom error kind. I do see a few problems with that approach:

• You still have to parametrize each type use with the HAL impl specific Error which funny enough makes the implementation less universal despite being more generic. The current associated type does not have suffer from the problem this approach would introduce
• It essentially allows a HAL impl to opt out of using the standard types by wrapping it in a Custom or Other type rendering the plan to a good deal toothless; we absolutely want to encourage implementers to use new shared error kinds and extend them within e-h if they're not sufficient (I so think that with a good look at some powerful implementation we can get a very good idea of what should be included in a first throw)
• I don't think nesting errors makes a lot of sense for embedded applications. std applications are moving away from nesting due to useability issues and crates like anyhow making it really easy to get a nice chain of errors iff error variants are not manually nested and embedded applications are even more simplistic than that due to the requirement that the application actually needs to know about the used peripherals and even initialise them specifically; there's no such thing as a library hidden several layers down magically initialising an peripheral and doing fancy stuff without the application knowing anything about that

I think with the new fallible-first approach it is very important to deliver a decent set of standard error kinds, too, and not leave it to impls to come up with a wild variety of custom errors (or none at all).