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 (https://github.com/rust-lang/rust/pull/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?

ryankurte on 22 Jun 2020

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.

eldruin on 22 Jun 2020

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.

therealprof on 22 Jun 2020

👍2

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

https://github.com/rust-embedded/embedded-hal/issues/181#issuecomment-647401030

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?

dbrgn on 22 Jun 2020

👍1

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.

therealprof on 22 Jun 2020

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.

dbrgn on 22 Jun 2020

👍1

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 on 22 Jun 2020

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.

therealprof on 22 Jun 2020

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 on 22 Jun 2020

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 https://github.com/rust-embedded/embedded-hal/issues/229#issuecomment-647201344 on stable.

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

ryankurte on 22 Jun 2020

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 on 22 Jun 2020

👍3

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 on 22 Jun 2020

@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).

therealprof on 22 Jun 2020

👍2

@therealprof i agree with most your points _except_ for the error nesting bit, ime the flat approach only works in std because they nest Box<dyn Error> so you can handle flat cases _without_ erasing useful underlying information. The critiques of the alternatives I proposed are totally valid, however, as noted above _none_ of the modern rust error handling stuff works without alloc, which i do not think an acceptable compromise for us :-(

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 it's also important to remember that this is not _only_ used in no_std contexts? I think linux-embedded-hal is a pretty good example, especially where async implementations are calling out to libc and mio.

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

This solves the, _there is a specific error that may occur during normal operation of the interface that a driver may wish to handle_, problem. Which may be specific to I2C, and in a way that doesn't restrict error types or delete error information, which afaik would resolve #181, and is generalisable to other cases similar to this.

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.

The other question about this is how you ensure people use the correct error types, and how you interpret and use those both at the driver and application level? Say we end up with BusFaultA and BusFaultB or something, we loose abstraction in that the driver author now needs to be aware of what an underlying impl _might_ return, and any addition to this enum that should be handled at the driver level requires a driver update, and the application author now needs to know _both_ what the underlying impl can return, and what the driver handles / forwards. Some of this is implicit to having defined errors, some of this is a facet of the flat approach I think.

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).

A single flat non-generic set would simplify things like this which is nice, but at the same time, seems to make it functionally impossible to report meaningful errors (device disconnect, permissions, pin not mounted, etc.) at the application level, which does not seem to me to be tenable.

It would be great to improve the situation, but, I would posit that any improvement should not be worse / less-expressive than the current approach, even if only for a subset of uses :-/

ryankurte on 23 Jun 2020

👍1

This solves the, there is a specific error that may occur during normal operation of the interface that a driver may wish to handle, problem. Which may be specific to I2C, and in a way that doesn't restrict error types or delete error information, which afaik would resolve #181, and is generalisable to other cases similar to this.

By adding another layer (the Option) you're forcing everyone to care about the possibility of a NACK being returned (which normally is an error) on the happy path in addition to regular error handling.

The other question about this is how you ensure people use the correct error types, and how you interpret and use those both at the driver and application level?

To be totally honest: I fully expect some people continuing to blindly unwrap the Result and panic, while others will continue to to wildcard matching on the Error and just use a best general error handling. What changes is that they now actually have the chance to do better because now they can for the first time have universal error kinds.

I do not expect that applications/drivers will handle each error individually but go "hey, I have a nice way of dealing with this of error (like NACK)" and just use some generic "let's start over" for anything else.

Say we end up with BusFaultA and BusFaultB or something, we loose abstraction in that the driver author now needs to be aware of what an underlying impl might return, and any addition to this enum that should be handled at the driver level requires a driver update, and the application author now needs to know both what the underlying impl can return, and what the driver handles / forwards. Some of this is implicit to having defined errors, some of this is a facet of the flat approach I think.

Yes, we should not allow have duplicate types and the description should be very clear so there's no confusion what's what.

However any problem around the error kinds in HAL impls, drivers and applications will be trivial to fix in a non-breaking way and can easily done by community contributions. Currently in most impls any change around errors is a breaking change.

A single flat non-generic set would simplify things like this which is nice, but at the same time, seems to make it functionally impossible to report meaningful errors (device disconnect, permissions, pin not mounted, etc.) at the application level, which does not seem to me to be tenable.

I am not proposing a single flat of set of errors, I'm proposing a set of errors per trait. I do not quite see how that makes reporting of any of the above impossible, certainly not any less possible than it is right now.

therealprof on 23 Jun 2020

Let's take a look at a real public example I know of:
In the e.ziclean-cube the MCU (stm32f101) is connected via _GPIO_ to an I2C accelerometer (KXCJ9).
In order to interact with the accelerometer, a bitbanging I2C implementation must be used (bitbang-hal).
If the MCU hal returns an error when trying to set or read a pin during an I2C operation, how would you give that information to the application?

I do not know if it would be fine to just start over, but I think if this comes up frequently, it would be useful to the application to be able to know that the MCU _pins_ are acting up, and not the accelerometer, for example, and that is the reason for some failures. This would immediately simplify diagnostics.
I think it would be useful to make something like this _somehow possible_.

eldruin on 23 Jun 2020

👍1

In order to interact with the accelerometer, a bitbanging I2C implementation must be used (bitbang-hal).
If the MCU hal returns an error when trying to set or read a pin during an I2C operation, how would you give that information to the application?

The bitbang-hal would take the digital::Error and turn it into an appropriate i2c::Error which the application can then handle. I would expect the accelerometer driver (if any) to properly relay the error to the application, in such a case it's totally fine if the accelerometer driver defines its own Error type with one of the enum variants being I2CError(i2c::Error) in order to relay any low level problems to the application.

I do not know if it would be fine to just start over, but I think if this comes up frequently, it would be useful to the application to be able to know that the MCU pins are acting up, and not the accelerometer, for example, and that is the reason for some failures. This would immediately simplify diagnostics.
I think it would be useful to make something like this somehow possible.

Absolutely.

therealprof on 23 Jun 2020

After reading the comments in addition to the trait specific error types which should cover the peripheral specific interaction with a peripheral it might also make sense to have a general error type for peripheral initialisation to handle the specific errors which usually occur during initialisation only as mentioned by @ryankurte.

therealprof on 23 Jun 2020

In order to interact with the accelerometer, a bitbanging I2C implementation must be used (bitbang-hal).
If the MCU hal returns an error when trying to set or read a pin during an I2C operation, how would you give that information to the application?

The bitbang-hal would take the digital::Error and turn it into an appropriate i2c::Error which the application can then handle. I would expect the accelerometer driver (if any) to properly relay the error to the application, in such a case it's totally fine if the accelerometer driver defines its own Error type with one of the enum variants being I2CError(i2c::Error) in order to relay any low level problems to the application.

Maybe there is a misunderstanding here.

The bitbang-hal I2C instance contains the GPIOs. If some kind of digital::Error is returned when setting a pin, the bitbang-hal I2C impl must transform this into some kind of i2c::Error. Let's say simplify and say just i2c::BusError.
The accelerometer driver will see i2c::BusError, wrap it into its own Error::I2C(i2c::BusError) and pass that back to the application.

The point is that it would be interesting for the application to be able to get the the original digital::Error, which was the source of the whole thing, and not a plain BusError, which contains no further information.

eldruin on 23 Jun 2020

The point is that it would be interesting for the application to be able to get the the original digital::Error, which was the source of the whole thing, and not a plain BusError, which contains no further information.

We could support this with this proposal. I do not think this is very practical though to allow passthrough errors and also encourage implementations to make use of them because you have an explosion of possible error cases and the whole point of the proposal is to unify them.

What would happen to your application if we did that? Well, if your application uses I2C with a bitbang-hal implementation a read might return i2c::Error(digital::Error(BogusPin)) while any other I2C peripheral implementation would return i2c::Error(BusError) (or a more specific Error) for the very same thing so your error handling needs to cater for all possible cases.

therealprof on 23 Jun 2020

What would happen to your application if we did that? Well, if your application uses I2C with a bitbang-hal implementation a read might return i2c::Error(digital::Error(BogusPin)) while any other I2C peripheral implementation would return i2c::Error(BusError) (or a more specific Error) for the very same thing so your error handling needs to cater for all possible cases.

Hmm, there is a mismatch in the types. I am not sure if it is intended.
The bitbang-hal::i2c::Read::try_read() method would return embedded_hal::i2c::Error::BusError(embedded_hal::digital::Error(BogusPin)).
A hardware I2C peripheral would return embedded_hal::i2c::Error::BusError(Something else or just nothing).

If somebody is _not interested_ in causes for the BusError and just wants to retry, they would write this:

use embedded_hal::i2c;
match i2c.try_read() {
  Err(i2c::Error::BusError(_)) => // retry or so, no dependencies on whatever is in there
  // ...
}

If somebody is interested in some underlying causes, then of course they have a dependency to whatever is inside, but this dependency is already there. e.g. they already know they are using bitbang-hal and that the error might be from the pins the application itself put into it.
Here the code:

use embedded_hal::i2c;
match i2c.try_read() {
  Err(i2c::Error::BusError(digital::Error(pin))) => // log diagnostics or so
  Err(i2c::Error::BusError(e)) => // something else happened
  // ...
}

So the point is that code that does not care about underlying errors can just ignore them and have no dependencies and code that cares about them can use them.

eldruin on 23 Jun 2020

👍1

I think there are two cases that we need to consider: A generic driver (which will not be able to deal with any platform-specific or project-specific error) and a hardware-specific application.

A generic driver will be very happy about unified errors because it allows it to handle certain cases like NACKs. I think nobody disagrees with the idea that there should be a list of predefined error variants.

However, an application like the vacuum robot may want to handle different kinds of bus errors differently.

If the e-h trait does not allow nesting errors (and thus the error information is not available by using the e-h API), then the driver will need to be modified with a custom non-e-h-compatible interface that returns the needed error information. For example:

// Pseudocode

// This is the hardware-specific API with more error information
impl Driver {
  fn write_with_error_details(&mut self, bytes: &[u8]) -> Result<(), DriverError> {
    ...
  }
}

// This is the generic driver API
impl Write for Driver {
  fn write(&mut self, bytes: &[u8]) -> Result<(), I2cError> {
    self.write_with_error_details(bytes)
      .map_err(|e| match e {
        DriverError::Gpio(_) => I2cError::BusError,
        DriverError::I2c(_) => I2cError::BusError,
      })
  }
}

The problem is that now the driver contains "prorietary" APIs that need to be called by the application. The developer of the application would need to fork or copy-paste the driver code in order to modify it.

If instead the e-h error enum allows wrapping a generic error, then the driver crate could simply pass through the underlying error, and the application could handle it if desired.

Of course this does not mean that we should not try to cover all possible error cases, but it still offers an escape hatch for developers that don't want to go through the "propose-change-to-e-h > get-change-accepted > wait-for-release > update-driver > wait-for-release > update-application" cycle.

dbrgn on 23 Jun 2020

👍2

Hmm, there is a mismatch in the types. I am not sure if it is intended.

Sorry, just me being sloppy.

Err(i2c::Error::BusError(digital::Error(pin))) => // log diagnostics or so

That will not work. You either need have a dedicated type for the BusError which includes all possible variants, like digital::Error or you need to make it a generic type which is incredibly painful to use.

therealprof on 23 Jun 2020

However, an application like the vacuum robot may want to handle different kinds of bus errors differently.

I still don't see it. If you have a universal application then you have no idea (and shouldn't!) what to do about a GPIO error that causes a bit-banging implementation to fail, if the bit-banging driver fails to work around the problem (e.g. by retrying the transaction) then it's game over anyway and you can only tell the application: "Sorry, something's wrong with the I2C bus."

If you want to have a proprietary application which cares about those implementation specific details then you can still offer an API in your bitbang-hal driver to provide more detailed information about the problem and ways to recover but that's off the path for what embedded-hal can provide IMHO.

therealprof on 23 Jun 2020

Hmm, there is a mismatch in the types. I am not sure if it is intended.

Sorry, just me being sloppy.

No problem.

Err(i2c::Error::BusError(digital::Error(pin))) => // log diagnostics or so

That will not work. You either need have a dedicated type for the BusError which includes all possible variants, like digital::Error or you need to make it a generic type which is incredibly painful to use.

Yes, we would need a generic error variant like already proposed:

#[non_exhaustive]
pub enum Error<E> {
    /// An unspecific bus error occured
    BusError(E),
    // ...
}

This is nothing really new, all drivers already define a generic Error<E> type and to me it has been quite fine so far.
For example, in a driver:

pub enum Error<E> {
  I2C(E),
  InvalidInputData,
}

Would now become:

pub enum Error<E> {
  I2C(i2c::Error<E>),
  InvalidInputData,
}

We now have an unified error layer in between but otherwise the driver code would see very few changes.

eldruin on 23 Jun 2020

👍1

If you want to have a proprietary application which cares about those implementation specific details then you can still offer an API in your bitbang-hal driver to provide more detailed information about the problem and ways to recover

Yes, that's precisely the problem. A user would need to fork all driver crates and modify them with a custom API in order to provide detailed error information.

Especially if we want to promote embedded-hal in a commercial environment, it seems a bit weird to first advertise a "production-ready ecosystem of mature generic and compatible drivers" but then say "if you want to do error handling for your hardware-specific project, you will need to fork all generic drivers and adjust them to your needs".

but that's off the path for what embedded-hal can provide IMHO

No, embedded-hal can definitely provide a solution for this if we want (the solution with a generic parameter that @eldruin and others proposed). The only question is whether we want to sacrifice this use case in order for the syntax to look slightly more clean (without the type parameter).

dbrgn on 23 Jun 2020

👍1

Well, at the moment it has to be generic because the error is in associated type within the HAL impl. I would like to get rid of that, that's part of my proposal.

The problem with the generic being all the way generic is that now the application needs to know implementation details which makes the approach less universal. I planned to get rid of the associated error type in the impls, your suggestion would turn that into an inner error type which now needs to be carried around anywhere and quite frankly makes things a lot worse.

therealprof on 23 Jun 2020

Yes, that's precisely the problem. A user would need to fork all driver crates and modify them with a custom API in order to provide detailed error information.

That is not correct. We already have proprietary methods on HAL impls, e.g. to construct them or initialise or make them change something internally (like switching modes on GPIO pins). You will always have implementation details which cannot be governed by e-h. Figuring out why bitbang-hal failed is nothing that should concern e-h at all.

No, embedded-hal can definitely provide a solution for this if we want (the solution with a generic parameter that @eldruin and others proposed). The only question is whether we want to sacrifice this use case in order for the syntax to look slightly more clean (without the type parameter).

I strongly disagree. We're not sacrificing any usecase here, it's merely a matter of the amount of precision we want to provide out of the box at the cost of complexity. I am of the opinion that simplicity should be the goal. The vast majority of usecases will benefit from the simple approach and only very few specific usecases _might_ actually benefit from the ability to nest errors; do we really want to complicate the implementation and add boilerplate for every user because of some obscure usecases which also could be easily implemented in a different way?

therealprof on 23 Jun 2020

The alternative being a totally implementation-defined errno-like mechanism to get the rest of the information whenever an error pops up, which is bolted manually onto other libraries.
I am sorry but to me that sounds pretty horrible.

eldruin on 23 Jun 2020

👍2

We already have proprietary methods on HAL impls, e.g. to construct them or initialise or make them change something internally (like switching modes on GPIO pins).

That's true for HALs, yes. But if you have a device driver crate that has a driver.measure(&mut i2c) API, then that driver will not be able to return HAL-specific information because it does not know about the HAL.

This would mean either the driver would need to be modified so it knows HAL-specific APIs, or the HAL would need some kind of "return the last error that happened" method. Which is horrible and doesn't work if a driver chains multiple calls to the HAL's APIs. It can also break completely in an async context.

dbrgn on 23 Jun 2020

👍2

The alternative being a totally implementation-defined errno-like mechanism to get the rest of the information whenever an error pops up, which is bolted manually onto other libraries.

I don't see what you mean. Your suggestion is also totally implementation defined if you actually want to handle the error. Diagnosing/repairing errors on that level will always require implementation defined code, and worse you're subjecting other unrelated code to the same scrutiny of error handling.

My point is that usually you don't need or care about these details and if you do you will have specific implementations anyway.

I really don't see what's so bad about:

match i2c.try_read() {
  Err(i2c::Error::BusError) => { 
    if i2c.get_err() == bitbang_hal::GPIOWentBonkus {
      // Whee, let's reinitialize that
      ...
    } else
    {
       // Just a glitch, retry  
       ...
    }
  }
}

It is just as implementation specific as your suggestion without all the boilerplate for dragging the generic type around...

therealprof on 23 Jun 2020

I really don't see what's so bad about: (...) i2c.get_err()

What if the driver does this?

impl TempHumiSensor {
    /// Wake up the sensor, try to do a measurement, go to sleep.
    pub fn low_power_measure<I2C: Write>(i2c: &mut I2C) -> Result<u8, i2c::Error> {
        i2c.try_write(WAKEUP)?;
        let result = i2c.try_write(DO_MEASUREMENT);
        i2c.try_write(SLEEP)?;
        result
    }
}

This method would ensure that the sensor goes back to sleep even if the measurement failed. If you get back an error, and then call i2c.get_err(), how do you know whether it's the error from the try_write(DO_MEASUREMENT) call or from the try_write(SLEEP) call?

Also, as mentioned above, asynchronous execution could also break such an API if I2C calls are interleaved.

dbrgn on 23 Jun 2020

👍1

That's true for HALs, yes. But if you have a device driver crate that has a driver.measure(&mut i2c) API, then that driver will not be able to return HAL-specific information because it does not know about the HAL.

Of course the driver can pass through the first layer of the HAL-specific error, why not?

This would mean either the driver would need to be modified so it knows HAL-specific APIs,

If you want to handle HAL specific errors in the driver you will always need to do that... If the type is generic as suggested by @eldruin you need to know exactly what the type will because you can't match on it either way. If you receive a I2CError::BusError(bitbang_hal::GPIOError) you need to match exactly on that or you match I2CError::BusError(_) which makes you no wiser than just I2CError::BusError.

or the HAL would need some kind of "return the last error that happened" method. Which is horrible and doesn't work if a driver chains multiple calls to the HAL's APIs. It can also break completely in an async context.

If you use an alternative I2C impl like from bitbang-hal you'll always have control over the instantiated I2C driver. There's no global errno as suggested by @eldruin, it's all to be managed by the impl.

therealprof on 23 Jun 2020

Of course the driver can pass through the first layer of the HAL-specific error, why not?

But then the driver needs to know about the HAL. If the driver should be generic (only using embedded-hal APIs, e.g. a driver crate published on crates.io) then that's not possible without modifying the generic driver for the application-specific code.

If you want to handle HAL specific errors in the driver you will always need to do that... If the type is generic as suggested by @eldruin you need to know exactly what the type will because you can't match on it either way.

The driver doesn't need to handle the error. It would simply pass it on to the application, which can then decide to handle the error, or not. The driver isn't able to do hardware specific error handling if it's written in a generic way.

If you call std::fs::read, then you will get back a generic error that wraps the platform-specific error. A generic library can still handle certain classes of errors without needing to know what platform the user is on. It cannot handle platform-specific errors, it just passes the error to the application. If the application targets Windows, then that application can use the Windows specific errors to do additional error handling. Nobody would expect you to fork and modify the generic library so it knows about the Windows specific I/O error messages.

Note: My mental model is a hardware-specific application, using the hardware-specific HAL, but calling generic drivers (e.g. from crates.io) through embedded-hal.

dbrgn on 23 Jun 2020

👍1

@dbrgn Not sure, but I feel you're missing my point: You seem to assume that revamped error types will somehow improve the handling of weird error cases and it absolutely does not do that.

I have no idea how this particular application uses the additional information that the I2C bus crashed due to a GPIO pin not doing what it is supposed to do (or how that even will be discovered by the bitbang-hal). If you say it can use this very specific detail somehow, that's great and I would love to hear more about it.

therealprof on 23 Jun 2020

But then the driver needs to know about the HAL. If the driver should be generic (only using embedded-hal APIs, e.g. a driver crate published on crates.io) then that's not possible without modifying the generic driver for the application-specific code.

No, it does not.

Nobody would expect you to fork and modify the generic library so it knows about the Windows specific I/O error messages.

Please stop claiming this proposal would encourage anyone to fork a library to use it. The whole proposal is about turning implementation specific errors into e-h defined general errors to make it more universal and easier to use.

I could go into the details of your std::fs::read example but I don't think this belongs here. I just would like to point out that is has other problems which anyhow looks to solve and is unfortunately not an option for embedded.

therealprof on 23 Jun 2020

I strongly support having specific error types per trait or group of traits (e.g. embedded_hal::I2cError) that are based on domain expertise, and that users of an implementation in general can rely on. Always felt weird to have completely unbounded error types (e.g. the time/frequency/duration types).

As suggested in today's meeting, the traits could also keep an associated error type, but add a trait bound like

type Error: core::fmt::Debug + Into<I2cError>;

which would:

let implementations add additional/granular detail specific to the underlying hardware
let users .into() errors and handle them such

A side effect would be to also allow for experimentation outside of embedded HAL (by defining a NewI2cError and implementing Into<NewI2cError> for Self::Error for >1 implementations of the trait together with use cases, before PR-ing a modification of the standard I2cError)

It would be even nicer if the trait bound required the existence of From<Error> for I2cError instead of the Into, as then ?'s implicit error conversion behaviour could be used, but I think it's not possible to express such a bound? Using the necessary relaxed trait implementation restrictions (RFC 2451) for this would raise MSRV to 1.41 in any case.

nickray on 23 Jun 2020

@nickray Thanks for the suggestion.

I'm curious about the benefits of such an associated trait.
What are the trait functions supposed to return? I would guess Result<_, Self::Error>?
Wouldn't that require all users to always call .into() in order to be able to use the predefined types?

It would be even nicer if the trait bound required the existence of From for I2cError instead of the Into, as then ?'s implicit error conversion behaviour could be used, but I think it's not possible to express such a bound? Using the necessary relaxed trait implementation restrictions (RFC 2451) for this would raise MSRV to 1.41 in any case.

I don't think a MSRV of 1.41 would be a blocker if it provides a major benefit for years to come, we already require 1.40 for the #[non_exhaustive] attribute which is a must have in my opinion. As far as I understand this would still require all users to cast to I2cError either implicitly via ? or explicitly. So if someone was to use such a trait function in an application directly they'd have to add an explicit conversion while drivers will likely not suffer because they're likely to shortcut with ??

I'm also interested in learning why we need suddenly need this extra amount of details for an error case: most impls don't even bother to define proper error types and I've yet to see one which contains error details in the variant at all. Most applications also don't bother with even checking the error type; plain unwrapping or ignoring the Result is very common and if errors are checked it's mostly a check wether the Result is an error without looking at the specifics of that error.

I appreciate that people are concerned that if we make such a breaking change it rather do everything righter than right rather than requiring another breaking change down the line but I haven't seen any believable evidence that this provides any benefits to the recovery from an error situation which is the only reason why such a complication could be accepted as a trade-off IMHO.

therealprof on 23 Jun 2020

Yes, the same Result as before.

If you add a Clone (or even Copy...) bound, you could pick off and handle the NACK by matching on error.clone().into(), and otherwise throw up the detailed error as with the generic approach suggested by @ryankurte (for situations that want/need this kind of granular/passthrough behaviour), but without imposing the generics tax on everyone else (hope not!). You're right that everyone else would have to pay the .into() price. So this would be a compromise solution - my preference is to KISS it like your original suggestion and agree with all your points.

Is it possible to bound on "there exists From for I2cError" (I really don't know)? This is just an aside to potentially lower the .into() tax via the implicit ? conversion.

nickray on 23 Jun 2020

hmm the type Error: core::fmt::Debug + Into<I2cError>; idea seems pretty interesting to me, though does seem that it would require Clone which is not _always_ viable for errors (especially on linux), and how should inner types not-mappable to flat types be handled?

it seems like TryFrom<&Self::Error> for I2cError would be pretty were it achievable?

None communicates the case where the inner error is not representable using the flat type
HAL impls that don't have other methods should be able to use the I2cError types directly, others can define still define their own
Drivers can choose to map into generic errors for handling without consuming the error / while still bubbling the underlying error to the application if desired.

The only real issue I have is that these approaches might promote error erasure in drivers, if you have to .into() to get the I2cError type the odds of a driver author returning the generic I2cError instead of the underlying one would seem high.
This is not an concern with the type Error: I2cError bounds approach, but, could probably be mitigated in documentation, and this way match works.

I'm also interested in learning why we need suddenly need this extra amount of details for an error case:

we have always had this facility, and i at least have used it _heavily_ in my radio work (thought the applications using this are not public :-/). even where some of these cases are not handled in the driver, you have the expressiveness to manage errors at the application, to attempt reconnections at different levels of the stack, and even just to let the developer/user know what actually happened. I would be personally v frustrating to have to connect a debugger to work out whether it's USB or SPI or networking that failed and returned me a BusError, or as mentioned above, to have to fork drivers to add out-of-band mechanisms for propagating the actual underlying errors.

Please stop claiming this proposal would encourage anyone to fork a library to use it. The whole proposal is about turning implementation specific errors into e-h defined general errors to make it more universal and easier to use.

i completely understand this perspective (and have had to do this a couple of times for some cursed C/FFI work), it's a real pain to have to add underlying error context to driver objects and, nearly impossible to do so in a way that is _safe_ with multithreading, async, or even multiple calls in a no_std context.

I don't think a MSRV of 1.41 would be a blocker if it provides a major benefit for years to come

It's not something to be done without consideration and deliberation, given the impact on everyone else in the ecosystem. But I too would be okay with a bump in MSRV, especially packages with another pile of breaking changes.

ryankurte on 24 Jun 2020

👍2

Since there seems to be an interest in relaying underlying error causes to a user, how about we add a universal ImplError enum with a number of common error cases and add this to each of the trait specific Error variants?

Like:

enum ImplError {
  /// Unspecified internal driver error
  Internal,
  /// Connection lost, e.g. device adapter was unplugged
  Disconnected,
  /// Ran out of memory while trying to allocate required buffers
  OutOfMemory,
  /// Operation timed out, please retry
  TimedOut,
  // ...
}

This would allow an implementation or driver to notify an implementation specific problem in a generic to an application and take appropriate action.

therealprof on 27 Jun 2020

enum ImplError {
  /// Unspecified internal driver error
  Internal,
  /// Connection lost, e.g. device adapter was unplugged
  Disconnected,
  /// Ran out of memory while trying to allocate required buffers
  OutOfMemory,
  /// Operation timed out, please retry
  TimedOut,
  // ...
}

In my experience lots of drivers do not need this and it would force users to match against situations that by design never come up. In many of my drivers the Error type is just like this:

/// All possible errors in this crate
#[derive(Debug)]
pub enum Error<E> {
    /// I²C communication error
    I2C(E),
    /// Invalid input data provided
    InvalidInputData,
}

eldruin on 27 Jun 2020

In my experience lots of drivers do not need this and it would force users to match against situations that by design never come up.

I agree. But this it was raised as desirable to be able to flag system/implementations errors and we want to get rid of the generics and associated types.

it would force users to match against situations that by design never come up.

If we go for the universal and #[non_exhaustive] error types then this will always be case. However since you already have to ignore non-handled errors this will make no difference in practice. Specifically for your example there's no difference at all in the application handling the Error.

therealprof on 27 Jun 2020

@eldruin @ryankurte Any ideas how to solve your objections to this proposal?

Otherwise I'd suggest to close this ticket to not further distract from getting the 1.0 release out and we can revisit a different proposal for 2.0.

therealprof on 2 Jul 2020

I do not have any other ideas than what already highlighted here.
Postponing this for 2.0 would be fine for me. Too bad we could not reach an agreement.
Thank you for your participation, everyone.

eldruin on 2 Jul 2020

Oh, please don't close it! While I advocated for a way to pass through custom errors, I would much rather see a "fixed errors only" proposal shipping in 1.0 than not changing the current APIs. It solves real driver problems that cannot be solved with the current API.

Right now there are a few proposals that have been discussed. Every approach would be better than the status quo. I tried to list them, as a basis for further discussion.

1a: Non-generic Error Enum

First, the original proposal by @therealprof:

#[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,
}

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

Pros:

It allows drivers to handle things like NACKs in a hardware agnostic way. Right now this is something that's not possible at all in embedded-hal.
The Error enum can still be extended without breaking compatibility thanks to #[non_exhaustive]
Gets rid of the associated Error type, making APIs cleaner

Cons:

Requires Rust 1.40
No way to pass through implementation-specific errors (e.g. network errors from an I2C-over-Ethernet implementation)

1b: Non-generic Error Enum plus ImplError

This is the second proposal by @therealprof, as suggested here.

#[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,
    /// Implementation specific error (shared across all peripheral specific error kinds)
    Impl(ImplError),
}

#[non_exhaustive]
enum ImplError {
  /// Unspecified internal driver error
  Internal,
  /// Connection lost, e.g. device adapter was unplugged
  Disconnected,
  /// Ran out of memory while trying to allocate required buffers
  OutOfMemory,
  /// Operation timed out, please retry
  TimedOut,
  // ...
}

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

Pros:

Same as variant "1a: Non-generic Error Enum" above
A fixed set of implementation-specific errors can be passed through, adding more information

Cons:

Requires Rust 1.40
No way to pass through custom implementation-specific errors, if an ImplError variant is missing then a PR against embedded-hal will need to be filed first

2: Standardized Error enum plus Other(E)

This is "a standardized set of errors plus an escape hatch". The Debug trait bound is just an example, that could be bikeshedded later on.

#[non_exhaustive]
pub enum Error<E: Debug> {
    /// 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,
    /// Another error occurred
    Other(E),
}

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

A HAL would impl it like this:

struct I2c1 { ... }

impl Read for I2c1 {
    type ImplError = I2cError;
    fn try_read(&mut self, _address: u8, _buffer: &mut [u8]) -> Result<(), Error<Self::ImplError>> {
        ...
    }
}

A HAL-agnostic driver would consume it like this:

struct Driver<I2C> {
    i2c: I2C,
}

impl<E, I2C> Driver<I2C>
where
    E: Debug,
    I2C: Read<ImplError=E>,
{
    fn query_sensor(&mut self) -> Result<(), Error<E>> {
        let mut buf = [0; 16];
        loop {
            // Polling as long as NACKs are returned
            match self.i2c.try_read(0, &mut buf) {
                Ok(val) => return Ok(val),
                Err(Error::NoAcknowledge) => continue,
                Err(other) => return Err(other),
            }
        }
    }
}

Pros:

It allows drivers to handle things like NACKs in a hardware agnostic way. Right now this is something that's not possible at all in embedded-hal.
The Error enum can still be extended without breaking compatibility thanks to #[non_exhaustive]
Implementation-specific errors can be passed through, adding more information (future proof)

Cons:

Requires Rust 1.40
Type soup (although there are not more type parameters than right now, they are now nested)

3: Generic with Type Conversion

Another approach suggested by @nickray and @rnestler would keep the current generic errors, but add an Into<Error> trait bound. I hope I interpreted their suggestion correctly.

#[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,
    /// Another error occurred
    Other(&'static str),
}

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

This would allow hardware-agnostic drivers to convert the error into a standardized set of variants for handling if it wants to:

struct Driver<I2C> {
    i2c: I2C,
}

impl<E, I2C> Driver<I2C>
where
    E: Clone + Into<Error>,
    I2C: Read<Error=E>,
{
    fn query_sensor(&mut self) -> Result<(), E> {
        let mut buf = [0; 16];
        loop {
            // Polling as long as NACKs are returned
            let result = self.i2c.try_read(0, &mut buf);
            match result.map_err(|e| (e.clone(), e.into())) {
                Ok(val) => return Ok(val),
                Err((_, Error::NoAcknowledge)) => continue,
                Err((e, _)) => return Err(e),
            }
        }
    }
}

HALs would simply need to provide a way to convert their custom errors into the standardized errors.

Pros:

Almost no changes requred for HALs, except for the additional trait bound and an Into<embedded_hal::blocking::i2c::Error> impl
The Error enum can still be extended without breaking compatibility thanks to #[non_exhaustive]
HALs are still completely free in how they design their error types
Drivers can - if they want - handle specific standardized errors while still passing through the underlying error

Cons:

Requires Rust 1.40
APIs are not cleaner than right now (type arguments are still there, plus the new trait bound)

dbrgn on 2 Jul 2020

After playing around with variant 2 (Other(E)) a bit on the Rust playground, I do agree that the additional nesting of the error type doesn't make it easier to impl and read the interfaces... Variant 1 does look attractively simple.

Variant 3 is very similar to the status quo. It gives lots of flexibility and just adds a trait bound. That trait bound adds no runtime/conversion cost when not used, but it allows solving the NACK-problem and other errorhandling situations in generic drivers. Since variant 3 solves the same problem as 2 but with a nicer API, I'd vote against 2.

Which ever approach we pick, that decision could always be revisited for e-h 2.0. However, evaluating the success of a solution is different:

If we'd implement variant 1 for e-h 1.0, we could evaluate the situation when discussiong e-h 2.0. If necessary, we could move to a more flexible approach like variant 3 later on (because if not being able to pass through custom errors is really a problem, we would hear from annoyed users).
If we'd implement the flexible variant 3 for e-h 1.0 first, we could take a look at Into<Error> impls. If a lot of people make use of the Other(&'static str) variant, then we know that we either missed some cases, or that the approach is not flexible enough.

I'm torn between 1 and 3. It's a tradeoff between a nice API and more flexibility for the ecosystem.

dbrgn on 2 Jul 2020

Non-generic Error Enum plus ImplError

No, the type would be:

#[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,
    /// Implementation specific error (shared across all peripheral specific error kinds)
    Impl(ImplError),
}

Anything involving generics or associated types defeats the purpose and reason behind my proposal which is to get rid of custom errors. That's also the reason why I am not hot about an escape hatch because if it exists people will use it instead of improving the errors with concrete variants which would allow others to profit.

The proposal was inspired by "modern" Rust Error handling (sans the stuff we cannot use in no_std, e.g. the std::error::Error trait which would be very cool to have and would conveniently give us the error nesting, too), cf. https://doc.rust-lang.org/std/io/enum.ErrorKind.html.

I wrongly assumed this would be low-hanging fruit, hence the proposal to change it before 1.0 (though in my defense I am stereo blind so I might have misjudged the "low-hanging" part). Suprisingly few people seem to be bothered enough by the current state of affairs to chime in so I think it is best to punt this for later instead of rushing this. 🤷🏻‍♂️

therealprof on 2 Jul 2020

No, the type would be:

Thanks for clarification, I updated my post above.

Anything involving generics or associated types defeats the purpose and reason behind my proposal which is to get rid of custom errors. That's also the reason why I am not hot about an escape hatch because if it exists people will use it instead of improving the errors with concrete variants which would allow others to profit.

Yes, after playing with the three variants a bit, I definitely do see the big appeal of removing that associated trait. I think it would be worth trying whether this works in practice, even at the cost of less flexibility.

Suprisingly few people seem to be bothered enough by the current state of affairs to chime in so I think it is best to punt this for later instead of rushing this. 🤷🏻‍♂️

I'm quite bothered by it (and wanted to create a concrete proposal for a long time, but never got around to doing it), since it's a blocker for optimizing some of my I2C drivers that currently wait potentially too long for a measurement to finish (e.g. shtcx). I'd love to see it in e-h 1.0.

After some thought and experimentation I think I'd be in favor of trying out the variant 1b ("Non-generic Error Enum plus ImplError"). If this proposal gains more support I could try to come up with a PR against e-h and an experimental branch for stm32l0xx-hal.

dbrgn on 2 Jul 2020

thanks for the summary @dbrgn! i think you may have missed the error-specific-trait-bounds option that is in the direction of 3 (ie. type Error: IsNack), which allows the same expressiveness as we currently have, while solving the decision making problem (at least for I2C) as far as i can see. i though when proposing it this was blocked on nightly but, i believe this is incorrect. You can see an example here and implementation here.

After some thought and experimentation I think I'd be in favor of trying out the variant 1b ("Non-generic Error Enum plus ImplError")

My main critique of this (having an Other(ImplError) where we define/maintain an ImplError enum) is that we are no longer providing only the interface, and expect the set of enum values to be a fast and moving target. If we wanted to experiment with this approach, maybe we could ship an embedded-errors crate that defines all of these and let people opt-in to using / trying them with the existing constraints? (though this does not solve the driver-decision-making problem).

I haven't dug into this _at all_, and it's terrible in at least a couple of ways, but, another approach to error-flattening _could_ be:

/// A hal error trait because we cannot use std/alloc ones
trait HalError: Debug {
  // whatever
}

/// A magic impl for types that already implement Error
/// (this adds some confusion when switching between std/no_std so might be disastrous / better to have an actual type wrapper
#[cfg(feature="alloc")]
impl <T> alloc::error::Error for <T> where T: HalError {
...
}

/// Enums with a "generic" escape hatch for logging
enum GenericI2cError {
  Nack,
  ...
  Other(&'static dyn HalError),
}

The pro of this is that you still get debug information about the underlying error, the con is that you can't make any decisions about this (because you don't have the actual type, though maybe it's possible to compare memory addresses or something else terrible like that), and as far as i can see this will be a nightmare to implement if you have more than one layer (because the error instances must all be statically declared and referenced).

ryankurte on 3 Jul 2020

The error trait bound is an interesting idea. Matching on the user side would be quite different, though:

match result {
  Err(e) => {
    if e.is_nack() {
        // retry
    } else if e.is_overrun() {
        // do something else
    }
  }
  Ok(_) => ...
};

Benefits:

Freedom to add further error information.
User code does not break if adding further methods to the Error trait.

Drawbacks:

Possible runtime cost on the error branch due to repeated if checks. Possibly small if sorted correctly, plus it is only on the error case.
It is not possible to enforce wildcard error matching for future errors so these might go unnoticed.
Error implementations must be adapted when adding a new error method.

`ImplError`

The problem I see with the ImplError variant is that there is no way to get it right:
For ImplError to be useful, the information transmitted by it must be very specific to the implementation problem. This naturally leads to a high amount of errors, _if embedded Rust is to become popular_.

If we accept these, after some time several similar variants of an error may be available so that users do not know what could actually be returned when calling a method, then they need to look into the implementation thus defeating its purpose.
To avoid this, slightly similar variants could be united into one, but then implementations return the same error for different situations, thus defeating the purpose again since an error reporting back-channel is necessary.

It seems like it is either overwhelming and thus difficult to use or just an extension of the available variants and thus imprecise.

Furthermore, in the case of users in control of both the HAL and drivers, existing ImplError variants which never come up might be repurposed just to be able to return "that one special error I need right now", which cannot be returned otherwise. Sure, the same thing could be done without ImplError but the number of variants are very limited and thus probably necessary. You may also think that this may only happen behind closed doors so who cares but let's be honest, you might inherit that code :)

Summary

So far I still think the version that serves our needs the best is OP+Other(E). It adds an unified I2C-error layer but does not break anything that is possible to do right now. It provides a clear and simple way to adapt existing implementations. Furthermore error matching allows you to get as much detail as you want from it and no more than that. The price to pay is a generic type, yes. But we have generics all over the place anyway.
@dbrgn can you post some code about what difficulties you encountered while playing with it?

Alternatively, Into<Error> is an elegant choice, but it comes at the price of Clone/Copy in order for it to be useful. I am a bit wary if that is always possible but we should check on this.

Other(&'static str) seems like a last-resource logging escape hatch. It might be useful sometimes but you can pretty much do nothing else with it. (what should you do now? panic? retry?). Alternatively, you can also start misusing it and do stuff like comparing memory addresses, rendering it as nice to use as Other(usize).

Then there is the "1a Non-generic Error Enum" (OP), which is really nice to use but only as long as everything is easy and weird errors never happen or nobody is interested. Sadly, it offers no way to scale from there.

I would like to hear more on @ryankurte's the error trait bound. I have not thought about it enough yet.

Finally, something not mentioned here so far is that I think it is just disappointing when you realize that the design of an API does not provide a good standard way of doing something that could be easily done and you need to do some tricks around it to make it work for you. It might sound exaggerated but simplifying errors for newcomers might come at the price of professional adoption/satisfaction.

PS: I hope this post is not too ranty :). My critique is exclusively technically motivated.

eldruin on 5 Jul 2020

The problem I see with the ImplError variant is that there is no way to get it right:
For ImplError to be useful, the information transmitted by it must be very specific to the implementation problem. This naturally leads to a high amount of errors, if embedded Rust is to become popular.

I disagree. There're only so many errors which can be usefully discerned and in most cases the only relevant information is: Is an error an intrinsic problem of the peripheral or is it a problem of the implementation; drivers don't care about implementation problems and only pass it along, applications may be able to handle specific implementation errors but even then there are at most a handful of different error kinds which can be usefully handled, depending on the application.

therealprof on 5 Jul 2020

I'm looking for some feedback on this idea.

Definitions:

Firmware: hardware-specific and/or implementation-specific code
Software: hardware- and implementation-agnostic "business logic"
Application: the user's code consisting of both firmware and software

Goal: Eliminate the need for implementation-specific errors to be propagated while allowing special treatment of those errors using the knowledge the application firmware possesses while maintaining modularity, dependency structure, separation of concern, etc.

Block Diagram

(Not all the abstractions shown would necessarily need to be used.)

Sequence Diagram

Any handling of specific errors (I2C-specific, hardware-specific, etc) beyond what is possible in the driver can be handled by the application's firmware by way of injecting an error handler into the driver. For example, the driver would provide a trait (or use a universal one) with method(s). That trait is implemented (using the application's knowledge of the system architecture) and instantiated by the application's firmware. The resulting error handler object is sent to the driver (during construction). In that way the driver can call a method from that object for any error that it can't handle itself. The handler might be able to resolve the issue using its special knowledge and return a good result, translate the error to something that makes sense to the _caller_ of the driver, return a value to be used in the event of this error, panic, etc.

The I2C driver can be generic (no need to propagate any impl-specific errors)
The EEPROM chip driver can be generic, only dependent on a Serial Comms trait (The same core chip product may have versions using different serial interfaces)
The Business Logic "software" remains clean, only dependent on a NonVolatile Memory trait

The application's hardware-specific "firmware" layer makes the connection between a NACK received by the I2C driver and the meaning of that NACK in the context of what is using that driver. It's at this step that the application firmware can try any "tricks" to resolve the error. In this case, it translates the NACK to an error known to the EEPROM chip driver that means "busy", "not ready", "try again later", etc. It can then be propagated or error handled again before finally returning up to the Business Logic software.

PTaylor-us on 3 Aug 2020

Thanks @PTaylor-us, that sounds like an interesting solution.
It would allow for using only OP's Error without needing ImplError or Other(E) by introducing an error handling "hook" which could process any implementation-specific error and if appropriate return one of the standarized I2C errors.
Maybe we can break the deadlock here, then.

I see a couple detail differences to what I understand from your comment, though:

I understand the error handler would need to be injected into the HAL or into drivers for "special" peripherals like an I2C switch or I/O expander. i.e. embedded-hal trait implementations.
I am not sure if we mean the same when we say "driver". i.e. "I2C driver" vs. "EEPROM driver".
Also, an EEPROM driver would be _the_ place to make the NoAcknowledge -> WouldBlock transformation.

But anyway these are just unimportant details and the proposal still stands.

I think it would be most interesting if we can come up with just one error handler trait which we can use for all fallible things (i.e. GPIO, SPI, etc), although we have not extended the discussion to those traits just yet.

For I2C, I understand an error handler would provide the chance to process the implementation-specific error and would itself transform that into a variant of I2C::Error or even Ok.
So I came up with this:

trait ErrorHandler<T, InError, OutError> {
    // takes a reference to the input error to avoid copy requirements
    fn handle(&self, e: &InError) -> Result<T, OutError>;
}

The trait type parameters allow for several implementations depending on the input/output errors that it should handle.

I have put together a demonstration of how an error handler could be injected into a bitbanging I2C implementation here in the playground.

Here is the handler implementation from there. It is a bit like Into<Error>:

impl ErrorHandler<(), digital::Error, i2c::Error> for Handler {
    fn handle(&self, e: &digital::Error) -> Result<(), i2c::Error> {
        match e {
            digital::Error::PinError => {
                // maybe log something
                Err(i2c::Error::BusError)
            },
        }
    }
}

Thoughts?

eldruin on 4 Aug 2020

👍1

@PTaylor-us Thanks for your input, though even with @eldruin's interpretation and implementation my imagination is not willing to cooperate and come up with an understanding of your idea. 😅

I found an hour yesterday and started to implement my concept here: https://github.com/therealprof/embedded-error, this is something I'll definitely use for my own crates and drivers to get rid of those hopelessly useless implementation depednent custom error types. It's very much WIP and used for experimentation (which is why I've not announced it yet or even written a README, though I did publish it to allow for easier use) but I'd like to invite anyone to PR their ideas to discuss them in a slightly less abstract manner (mainly to cater for my lack of imagination).

therealprof on 4 Aug 2020

I have updated my last post and inlined the diagrams. @therealprof Perhaps the diagrams will help.

@eldruin I think we're more-or-less on the same page. I think the only thing we differ on is your interface-specific device drivers as opposed to me talking about generic device drivers.

PTaylor-us on 4 Aug 2020

A few hours ago we were discussing the issue again and the solution proposed in https://github.com/rust-embedded/embedded-hal/issues/229#issuecomment-648403992 came out very much in favour.

I haven't realised it at the time but after some careful explaining by @eldruin of ideas behind this approach, I did some exploration work to figure out whether there's any cognitive or programming overhead or other stumbling blocks with this approach and I'm happy to report that I couldn't.

Much to my surprise and delight it is not even necessary to .into() the Err in case one is matching against the T, so there's virtually no downside in comparison to fixed error types for the typical case while still allowing to go wild with custom errors for special cases.

So 👍 from me.

therealprof on 26 Aug 2020

🎉6

@ryankurte Would Into<Error> work for you? Or would the Clone bound be a big problem?

eldruin on 4 Sep 2020

hey that's excellent news! i still think the clone bound is important so we don't erase the error information any time we use .into(), but this can either be resolved by expanding the bound to type Error: Into<I2cError> + Clone + Debug;, or by ensuring where Error: Clone at the driver level.

with more mulling i think the non-clonable error type problem i raised earlier is probably not so bad in that usually where there is an io::Error one can instead use the inner io::ErrorKind, and at least in general we control these error types via -hal implementations and can thus ensure they are Clone.

so, i have a preference for the former option to encourage not erasing types / standardise this for everyone, but, would be okay with the other the latter if there are reasons we should not do this?

ryankurte on 5 Sep 2020

Great! I would be fine with type Error: Into<I2cError> + Clone + Debug;. If somebody actually has a problem with it, we can relax it in a later non-breaking release.

eldruin on 5 Sep 2020

Excellent, let me work this into my experiment to see whether I would have a problem with the Clone bound.

therealprof on 5 Sep 2020

Seems to work, now to add Clone to all error kinds in embedded-error. 😅

therealprof on 5 Sep 2020

Great! I would be fine with type Error: Into + Clone + Debug;. If somebody actually has a problem with it, we can relax it in a later non-breaking release.

i believe relaxing this bound would be fine for HALs but breaking for drivers as anything depending on it could no longer depend on Clone so, we may not be able to relax this later. i do feel pretty comfortable with testing this in another e-h alpha release though.

another caveat is how to signal non-representable types (ie. where the base error cannot represent the bus error)? this was the justification behind TryInto rather than Into in https://github.com/rust-embedded/embedded-hal/issues/229#issuecomment-648463707. i can also see the argument here that all possible underlying errors should be broadly representable by the generic type and if so we have no need for failure. either seems roughly okay to me, though i would like the decision to be explicit and whatever we do well documented ^_^

Seems to work, now to add Clone to all error kinds in embedded-error

nice work! feels like we can start moving towards an RFC PR for this?

ryankurte on 6 Sep 2020

i believe relaxing this bound would be fine for HALs but breaking for drivers as anything depending on it could no longer depend on Clone so, we may not be able to relax this later. i do feel pretty comfortable with testing this in another e-h alpha release though.

True, good point.

another caveat is how to signal non-representable types (ie. where the base error cannot represent the bus error)? this was the justification behind TryInto rather than Into in #229 (comment). i can also see the argument here that all possible underlying errors should be broadly representable by the generic type and if so we have no need for failure. either seems roughly okay to me, though i would like the decision to be explicit and whatever we do well documented ^_^

I think TryInto would also be ok (and technically more correct) but it would make the code a bit more complicated since the conversion will not be as seamless as with Into due to the need to unwrap the result.

I thought in those cases the conversion should result in a sink error variant like Other or so. e.g. for implementation errors. Since it remains a possibility to get the information from a cloned instance, the difference between TryInto and Into is not as decisive as it normally is.

An alternative could be to add a dedicated variant like ErrorConversionFailed and document how it is to be used.

Seems to work, now to add Clone to all error kinds in embedded-error

nice work! feels like we can start moving towards an RFC PR for this?

Indeed!

eldruin on 6 Sep 2020

I think TryInto would also be ok (and technically more correct) but it would make the code a bit more complicated since the conversion will not be as seamless as with Into due to the need to unwrap the result.

I believe the seamlessness of the associated type + Into<_> bounds approach is one of the key elements here since you absolutely don't want error handling to be awkward or people will find rather invent nasty shortcuts instead of doing it right.

therealprof on 6 Sep 2020

👍1

I believe the seamlessness of the associated type + Into<_> bounds approach is one of the key elements here since you absolutely don't want error handling to be awkward or people will find rather invent nasty shortcuts instead of doing it right.

yep, anything that we can do to make the _right_ approach the easy one is definitely worthwhile! and the associated error bound and handling does add some complexity that it would be nice to avoid.

an Other sink type on each e-h error seems like an okay compromise to me, provided it is well documented (ie. when you see Other you cannot really do anything other than return the original error). if we go with the non_exhaustive attribute @therealprof suggested earlier the addition of this later would be non breaking, though given the general case then has to be handled the Other variant might not even be required and (if you needed to extract the error) i believe you would end up with something like:

pub fn whatever(&mut self, ...) -> Result<(), WhateverError> {
  let r = self.i2c.read(addr, &mut buff);
  match r.clone() {
      Err(I2cError::Blocking) => ...
      Err(e) => return r,
      _ => (),
  }
}

and either would seem okay to me. the need to split the .read() and the match statements and for the .clone() are perhaps not immediately obvious. i can't see a way to improve this (short of Copy but, seems too much), but this is readily mitigated by documentation i think.

-edit- i haven't thought about this in any detail, or tried it, but, is the bound we actually want HalImplError: PartialEq<I2cError>? not sure if that would be enough to make match statements work.

ryankurte on 7 Sep 2020

if we go with the non_exhaustive attribute @therealprof suggested earlier the addition of this later would be non breaking, though given the general case then has to be handled the Other variant might not even be required and (if you needed to extract the error)

The error receiver code would indeed look the same but whoever implements the Into needs to return an existing variant when the conversion fails. So something like Other is necessary.

and either would seem okay to me. the need to split the .read() and the match statements and for the .clone() are perhaps not immediately obvious. i can't see a way to improve this (short of Copy but, seems too much), but this is readily mitigated by documentation i think.

Agreed.

-edit- i haven't thought about this in any detail, or tried it, but, is the bound we actually want HalImplError: PartialEq<I2cError>? not sure if that would be enough to make match statements work.

At least with PartialEq it does not work. I could not immediately find anything about pattern matching overload.

eldruin on 7 Sep 2020

Hi there!

I might came a bit late, but I have a proposition.

So first, I like the current idea, that is, if I've well understand, to have the different errors be:

trait SomeEmbeddedTrait {
    type Error: Into<I2cError> + Clone + Debug;
    // ...
}

The drawback already exposed is the Clone need. I also think that using the trait is a bit strange, with the match (e.clone(), e.into::<I2cError>()) pattern that is not really readable and implies some strange clones.

My proposition mimic a bit std::io::Error as a trait: the error traits would have a method fn kind(&self) -> I2cErrorKind that return the error kind, that is an enum with the "standard" error kinds possible that the user might want to match. This I2cErrorKind also implement I2cError, allowing to use it directly as an error if nothing more is needed.

Here a "full" example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b3221c8411b3f5db506f837899b31c8a

Extract of the linked sample: checking the Nack case:

fn generic_read<R: Read>(i2c: &mut R) {
    match i2c.read(42, &mut []) {
        Ok(()) => println!("Ok"),
        Err(e) if e.kind() == I2cErrorKind::Nack => println!("Nack"),
        Err(e) => println!("Error: {:?}", e),
    }
}

Some helper function can also be added to the traits as is_nack(). For the multiple error of UART, kind can be UartError::Multiple, and you can check the different is_xx on the interface.

What do you think?

TeXitoi on 29 Sep 2020

I also think that using the trait is a bit strange, with the match (e.clone(), e.into::()) pattern that is not really readable and implies some strange clones.

I don't follow. There's no requirement to use clone if you don't want to and also you don't need to explictly call into at all; you can directly match on the error.

therealprof on 29 Sep 2020

you can't directly match on the error. You can only if your type is I2cError, not if it's some other type that Into<I2cError>

TeXitoi on 29 Sep 2020

you can't directly match on the error. You can only if your type is I2cError, not if it's some other type that Into<I2cError>

Making that the main error kind is the key idea here; others were keen on making that a trait bound instead of using it directly for more flexibility. I totally expect that most implementations will simply use it directly.

therealprof on 29 Sep 2020

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dde1c8b7bdf5cc50d962aba39f99262f

TeXitoi on 29 Sep 2020

If that's a trait bound, even if most people use it directly, you can't rely on it on generic code, and that's the central point of the current problem (or I misunderstood something).

TeXitoi on 29 Sep 2020

It's not just a generic type with bounds. It's an associated type with trait bounds.

therealprof on 29 Sep 2020

Same thing: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7eb265f15af811d55d39bea35b4d7303

TeXitoi on 29 Sep 2020

Compiling version need some strange hacks: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4c564b26793ea2101f6885481c3265a2

TeXitoi on 29 Sep 2020

Using a generic function like that is still a special case, but what's wrong with:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=02258d4f43254ea121d7b6bb6e949d3a
?

therealprof on 29 Sep 2020

I thought the main point of this proposition is to be able to implement generically on embedded hal trait, and be able to match some error kind as nack on I2C, so it is exactly the kind of function you'll be writing.

In your example, you loose the original type, printing I2cError in the last branch, and not the original R::Error, that might contains additional information that the end user might be interested.

TeXitoi on 29 Sep 2020

A better example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7d5a996e815b169f88e2fc7782bfb76f

TeXitoi on 29 Sep 2020

I thought the main point of this proposition is to be able to implement generically on embedded hal trait, and be able to match some error kind as nack on I2C, so it is exactly the kind of function you'll be writing.

Yeah, but how often do you use freestanding generic functions like that?

In your example, you loose the original type,

Which is fine. We cannot do stacked Errors, that has been discussed to exhaution and it is way more important to support common Error types which any implementation can understand than having bespoke custom errors. As you've shown this can still be done but requires more effort, which is also fine.

therealprof on 29 Sep 2020

A better example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7d5a996e815b169f88e2fc7782bfb76f

Looks reasonable to me if that's what you want to do. What is the problem there?

therealprof on 29 Sep 2020

Need clone, hackward tuple matching. With my proposition, you have: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9b6cbefb20f5c8dea820f37b70fbc63f

TeXitoi on 29 Sep 2020

I thought the main point of this proposition is to be able to implement generically on embedded hal trait, and be able to match some error kind as nack on I2C, so it is exactly the kind of function you'll be writing.

Yeah, but how often do you use freestanding generic functions like that?

When you impl a driver, your driver will have this exact generic bound.

TeXitoi on 29 Sep 2020

In your example, you loose the original type,

Which is fine. We cannot do stacked Errors, that has been discussed to exhaution and it is way more important to support common Error types which any implementation can understand than having bespoke custom errors. As you've shown this can still be done but requires more effort, which is also fine.

If you don't care of losing the original type in your driver implementation, then why support custom error in the embedded hal traits? (your original proposition was directly returning a well defined error)

TeXitoi on 29 Sep 2020

Need clone, hackward tuple matching. With my proposition, you have: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9b6cbefb20f5c8dea820f37b70fbc63f

Only if you intend to pass on the original error, which (again) is not the idea here.

If you don't care of losing the original type in your driver implementation, then why support custom error in the embedded hal traits? (your original proposition was directly returning a well defined error)

Having custom errors was never my plan, I think errors should always converge into well defined error kinds and #[non_exhaustive] allows us to express that.

I do also think that it's not really desirable to use and/or pass-through implementation specific errors for a number of reasons. Most prominently because that again ties the specific hardware and implementation to the application running on it while I think it is much more worthwhile expanding the shared set of errors to extend the common knowledge of errors which might occur and allow to handle them universally.

I'm not totally opposed to your proposal as it seems implementable, but then again I don't really see any desirable benefits over the approach we're currently looking at and worse, it makes the encouraged and common use (to actually implement proper error handling) just that little more complicated by introducing the kind() indirection.

therealprof on 29 Sep 2020

If you don't care of losing the original type in your driver implementation, then why support custom error in the embedded hal traits? (your original proposition was directly returning a well defined error)

At least for me, the reason is that a specific HAL implementation can have more error detail than a generic driver can reasonably handle. So:

if you write code for a specific HAL, you can use and handle that specific error type
if you write generic driver code, you can Into the error into a well-defined and handle-able set.

No generic parameter (that's like an exception that nobody can actually handle in an app -> generic driver -> HAL setting, plus it adds syntax noise everywhere especially if you have a bunch of peripherals), no TryInto which is like a backdoor to do the same "dump specifics up the chain" cop-out. Associated types are just much nicer than generic types :)

The embedded-error crate's task is to crystallize what these errors are.
The HAL's author's task (via the Into bound) is to crystallize how to map the specific errors to the generic ones.

A driver still has the option to use an error type directly (with possibly its own bounds on driver applicability), but this would be off the mainstream path, and not inflict corner case pain on the general public, so to speak. I think this is a very important topic in API design (we've had a bunch of clashes between "normal" MCUs and embedded Linux, for instance).

Thanks to your "Infallible converts to anything" insight, a HAL at least theoretically even has the option to declare everything infallible and just panic on every error ;)

As a personal aside, for me as a not-very-embedded person, it's very useful to complement the embedded-hal's "_how can standard peripherals be used, in general_" teachings with the potential embedded-error's "_and what can go wrong, in general_", both nicely outsourced to a group of experts :)

nickray on 30 Sep 2020

Let's resume the different propositions.

The Error to rule them all

Each embedded hal trait define its Error, with no choice for the hal implementation. That's the original @therealprof proposition.

#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[non_exhaustive]
pub enum I2cError {
    Nack,
    Overrun,
    Other,
}
pub trait Read {
    fn read(&mut self) -> Result<(), I2cError>;
}

Advantages:

driver implementation can handle the Nack case if they want
simpler interface: no parametric error type

Disadvantages:

I2cError must propose every imaginable cases
If a hal implementation want a new error, it must modify the embedded hal Error
Error can't be as precise as wanted. For example, a linux driver might want to put the file path of the device, and embedded hal can't handle PathBuf.
final user might have a degraded error message as the error is filtered by the error type of embedded hal

Convertible Error

That's the current proposition. Instead of forcing the Error, some bounds are imposed on the Error to the hal implementors, allowing drivers to convert the error to something fixed.

#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[non_exhaustive]
pub enum I2cError {
    Nack,
    Overrun,
    Underrun,
    Other,
}
pub trait Read {
    type Error: Into<I2cError> + Clone + core::fmt::Debug;
    fn read(&mut self) -> Result<(), Self::Error>;
}

Advantages:

driver implementation can handle the Nack case if they want
hal implementation can use a custom error to be as precise as needed, including having std types on them.
hal implementation can use directly the I2cError as the error if that's enough

Drawbacks:

more complicated than a fixed type
The into trait, consuming the error, force to add the clone bound to the error to be able to get the I2cError without losing the original error
The fact that this is a conversion will suggest that the driver can erase the original error type, exposing only the I2cError in its interface.
If the driver erase the error type, the final user will not have the improved error.

The last drawback afraid me in inconsistent driver implementation: some driver returning Read::Error, some driver returning Read::I2cError, some driver returning sometimes Read::Error, sometimes Read::I2cError. I suppose that the recommendation for drivers is to returns the original Read::Error to avoid lost of information, at using I2cError internally to handle some kind of errors.

An Error bound on the trait with a `kind` method

This is my proposition. The first drawback it tries to fix is the Clone bound: in reality, the embedded hal error is just a plain enum, that can be copy, no reason to eat the original error to do the conversion, just a &Read::Error should be enough, and we can remove the Clone bound.

Then, the desired feature (be able to handle the Nack error in a driver) remind me the handling of the special error "broken pipe", that might be traited as an EOF in some case (think of an implementation of grep for example). In std, broken pipe in an io::ErrorKind. Thus, my idea is to mimick this interface, providing an ErrorKind for each trait providing the common errors that driver implementation might want to manage, and not every possible error.

The ErrorKind also implements its Error trait, allowing to use the ErrorKind directly as an error if this is enough as in the proposition 2.

This proposition allows everything possible with proposition 2, without a Clone bound, and with an interface expliciting that the error must not be lost by the Driver implementation.

So we have:

pub trait I2cError: core::fmt::Debug {
    fn kind(&self) -> I2cErrorKind;
}

#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[non_exhaustive]
pub enum I2cErrorKind {
    Nack,
    Overrun,
    Underrun,
    Other,
}

impl I2cError for I2cErrorKind {
    fn kind(&self) -> I2cErrorKind {
        *self
    }
}
pub trait Read {
    type Error: I2cError;
    fn read(&mut self) -> Result<(), Self::Error>;
}

Advantages:

driver implementation can handle the Nack case if they want
hal implementation can use a custom error to be as precise as needed, including having std types on them.
hal implementation can use directly the I2cError as the error if that's enough

Drawbacks:

more complicated than a fixed type

Conclusion

I think proposition 3 is better than proposition 2. Everything that can be done with proposition 2 can be done with proposition 3, and with a more ergonomic interface. Proposition 1, as more opinionated and simpler, proposes a different alternative.

TeXitoi on 30 Sep 2020

👍3

@TeXitoi We already took what you call "Proposition 1" (and all the others in the discussion) off the table when we agreed on what you call "Proposition 2".

@ryankurte You had some very strong opinions here, what's your take on "An Error bound on the trait with a kind method"?
@eldruin What do you think?

therealprof on 30 Sep 2020

@therealprof yes I know that proposition 2 is prefered to proposition 1, but, talking to you, I think you still prefer proposition 1.

TeXitoi on 30 Sep 2020

At least for me, the reason is that a specific HAL implementation can have more error detail than a generic driver can reasonably handle. So:

if you write code for a specific HAL, you can use and handle that specific error type

if you write generic driver code, you can Into the error into a well-defined and handle-able set.

Imagine the following case:

we have some driver that take gpios pins and interface a switch matrix
we have a driver that expose a gpio multiplexer above I2C
the final user use this gpio expanded pins with the switch matrix driver

The error of the gpio multiplexer just forward the I2C errors, as every other operations are infallible

The final user want to just skip I2C "unreachable" errors, as it just correspond to the 2nd half of the keyboard deconnected.

If the matrix driver erase the original error with the I2cError, the final user can't check if the error correspond to a deconnected half of keyboard, because the information is lost.

So I think that's important that the drivers don't erase the original error.

TeXitoi on 30 Sep 2020

@therealprof yes I know that proposition 2 is prefered to proposition 1, but, talking to you, I think you still prefer proposition 1.

Maybe, but that's not how consensus works. ;)

therealprof on 30 Sep 2020

huh, option 3 seems pretty good to me! resolves the Into/Clone issues where we really wanted a From<&..> as not to erase the inner type and supports matching in a reasonably ergonomic manner.

probably need to play with it still but, the only question i can think of now is whether to return an Option<..> or have an Unknown variant for each of the base error kinds, but I think the latter is more convenient from a matching perspective.

Only if you intend to pass on the original error, which (again) is not the idea here.

I do also think that it's not really desirable to use and/or pass-through implementation specific errors for a number of reasons. [...]

i am still camp, pass the underlying errors up, but it seems to me that this approach would pretty well let you erase that at the application level if that's your jam, with something like Application<I: i2c::Read<..>, E: I2cError>?

ryankurte on 30 Sep 2020

probably need to play with it still but, the only question i can think of now is whether to return an Option<..> or have an Unknown variant for each of the base error kinds, but I think the latter is more convenient from a matching perspective.

For that there's a Impl variant on each Error(kind): https://github.com/therealprof/embedded-error/blob/a22750f8715cb3322d18fc8dfc3c2d3ba5070b68/src/lib.rs#L134

therealprof on 30 Sep 2020

But your Impl variant is not parametric, it's in the hal, so not customisable. And having it dependents of the hal will remove the advantage to have a well defined set of errors for the drivers, no?

TeXitoi on 30 Sep 2020

No, it's not in the HAL, it's a generic set of Errors applicable to any other Error(kind). It's actually the closest thing to the io::ErrorKind you were modelling your proposition on. Of course if we go for the new proposition, the same approach would apply to it.

therealprof on 30 Sep 2020

Yes, sorry, I mean in "embedded hal".

But we still need a catchall " Unknown" or "Other" for the hal with exotic errors.

TeXitoi on 30 Sep 2020

That's exactly what ImplError is for. Your catchall (or rather "I'm-too-lazy-to-bother") error kind is called ImplError::Internal.

therealprof on 30 Sep 2020

OK, I had missed internal as the catchall.

TeXitoi on 30 Sep 2020

I think proposition 3 is better than proposition 2. Everything that can be done with proposition 2 can be done with proposition 3, and with a more ergonomic interface. Proposition 1, as more opinionated and simpler, proposes a different alternative.

Neat! I agree that proposition 3 is a strictly superior formulation of the principles that underlie proposition 2 (basically the unexpressable for<'a>: Into<I2cError> for &'a I2c::Error). I learned something, thanks @TeXitoi!

My only remaining niggle would be a bikeshed, where in my understanding of current best practice all the distinguishing prefixes should be replaced by submodules instead (e.g. embedded_error::i2c::ErrorKind instead of enum I2cErrorKind, and embedded_error::i2c::Error instead of trait I2cError).

(If I don't stop myself, I also feel like the enum and the trait both want to be called Error, and the method into instead of kind, but this use of Kind suffixes has precedent in std, so so be it...).

nickray on 30 Sep 2020

Thank you for this @TeXitoi!

I agree that proposition 3 seems better than proposition 2 for the reasons you listed.
I would also prefer an extra Unknown/Other variant over Option due to matching ease, as previously discussed.
Since we would now have the generic kind() mapping mechanism which can keep the underlying error information, I would not include embedded-error's ImplError into embedded-hal. MCU HALs are free to use it through embedded-error if they want, though.
As for bikeshedding, I would indeed name the errors i2c::Error and i2c::ErrorKind as customary. (same as i2c::Write vs spi::Write)
I would also keep the kind() and -Kind as proposed for consistency with std. Furthermore, into has a specific meaning in Rust which we actually do not want (i.e. consumption).

eldruin on 30 Sep 2020

About I2c prefix, I agree.

You can't have an Error trait and a Error enum in the same module, that's a name clash. And it would be quite confusing.

Naming the method into is a bad idea: a method begining with into in rust means, by convention, that it consumes self. https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

There is a few kind method in std, that seems close to our case https://doc.rust-lang.org/std/iter/trait.Iterator.html?search=kind The most known kind method is https://doc.rust-lang.org/std/io/struct.Error.html#method.kind

kind is the best name I can find, but if you have a better idea, feel free to propose!

TeXitoi on 30 Sep 2020

Since we would now have the generic kind() mapping mechanism which can keep the underlying error information, I would not include embedded-error's ImplError into embedded-hal. MCU HALs are free to use it through embedded-error if they want, though.

I don't follow. Currently there's no plan to include anything in embedded-hal, it would be merely a user of the error types provided by embedded-error, the main reason being that we can swiftly release new patch versions whenever we add a new error kind without having to rely on sluggish embedded-hal release cycles. As such ImplError automatically becomes an integral part of the concept.

I am very much against a trivial escape hatch like an Unknown variant on every error kind; there're pretty much no "unknown" errors!

As for bikeshedding, I would indeed name the errors i2c::Error and i2c::ErrorKind as customary. (same as i2c::Write vs spi::Write)

We could certainly put each category into their own module within embedded-error, not sure using a naked Error is the best of all name choiced though as that's used very inflationary everywhere and thus is a constant source of confusion when multiple of them appear in a file...

therealprof on 30 Sep 2020

I don't follow. Currently there's no plan to include anything in embedded-hal, it would be merely a user of the error types provided by embedded-error, [...]. As such ImplError automatically becomes an integral part of the concept.

I already highlighted here what I think the problem with ImplError is. Here adapted to the current proposal:
For ImplError to be useful, the information transmitted by it must be very specific to the implementation problem. This naturally leads to a high amount of errors.

If we accept these, after some time several similar variants of an error may be available so that users do not know what could actually be returned when calling a method, then they need to look into the implementation thus defeating its purpose.
To avoid this, slightly similar variants could be united into one, but then implementations return the same error for different situations, thus defeating the purpose again since the original error information needs to be processed anyway.

It seems like it is either overwhelming and thus difficult to use or a small extension of the available variants and thus imprecise.

Since generic drivers have no use for ImplError either, why bother adding it?

I would simply leave ImplError out of embedded-hal (in whatever integration form. through dependency or own code) and let applications/HALs define their own.
I would add a simple enum with the error variants from the protocols and make implementations return I2cError::Other in kind() for whatever else. If the user is interested, the original information can still be taken out.

eldruin on 30 Sep 2020

It seems like it is either overwhelming and thus difficult to use or a small extension of the available variants and thus imprecise.

I do not agree. Have you looked at the available kinds? Any kinds you would add from the top of your head?

Since generic drivers have no use for ImplError either, why bother adding it?

Actually they do. Examples include things like handling (or not being able to handle) NACKs, signalling Timeouts, disconnected devices, discovery problems... Those are even more relevant when stacking implementations, e.g. the driver for a GPIO expander connected via SPI would like to signal that a connectivity issue is the problem for your pin toggling problems.

let applications/HALs define their own.

That is an declared anti-goal of this approach, pretty much the only thing I am not okay with.

If some system wants to define their own proprietary errors to allow more fine grained handling or better diagnostics that is okay but it should be exception and not the norm.

We want to give HAL/driver implementers a solid and consistens toolbox to communicate errors and drivers as well as applications universal and consistent information to allow them to handle any situation in an appropriate way (if they so desire).

therealprof on 30 Sep 2020

It seems like it is either overwhelming and thus difficult to use or a small extension of the available variants and thus imprecise.

I do not agree. Have you looked at the available kinds? Any kinds you would add from the top of your head?

I am sure people will come up with more.

Since generic drivers have no use for ImplError either, why bother adding it?

Actually they do. Examples include things like handling (or not being able to handle) NACKs, signalling Timeouts, disconnected devices, discovery problems... Those are even more relevant when stacking implementations, e.g. the driver for a GPIO expander connected via SPI would like to signal that a connectivity issue is the problem for your pin toggling problems.

Things like NACKs are already part of the I2cError enum, not ImplError. Same thing with I2cError::Timeout (although that is part of SMBus, not I2C).

A driver getting an OutputPin should not care / cannot care about whether the pin comes from the MCU or through an I2C multiplexer. It really cannot handle ImplError::PowerDown.

While ImplError allows to get _a bit_ more of information, it cannot tell exactly what went wrong without making ImplError overwhelming (because only the implementation knows), which is precisely my point.

Since it does not provide a complete solution and is also not useful for generic drivers, I would avoid it and just let I2C methods return I2C-proper errors, or Other.

let applications/HALs define their own.

That is an declared anti-goal of this approach, pretty much the only thing I am not okay with.
If some system wants to define their own proprietary errors to allow more fine grained handling or better diagnostics that is okay but it should be exception and not the norm.

MCU HALs can reuse an unified error definition like ImplError by importing it themselves form a central crate like embedded-error. I have nothing against that. Similar to the rand_core dependency, for example.

However, I would not make ImplError part of I2cError.

eldruin on 30 Sep 2020

I am sure people will come up with more.

Which is encouraged. But if you can't come up with additions in a hurry I have my doubts that we're going to be swamped with an unwieldy number of requests. 😏

A driver getting an OutputPin should not care / cannot care about whether the pin comes from the MCU or through an I2C multiplexer. It really cannot handle ImplError::PowerDown.

Maybe not PowerDown but it may want to retry a few times if it receives an ImplError::Timeout.

While ImplError allows to get a bit more of information, it cannot tell exactly what went wrong without making ImplError overwhelming (because only the implementation knows), which is precisely my point.

It doesn't need to tell exactly what went wrong but if it can point into the right direction that is a huge help. Without it all a non-device specific application can only do a best-effort problem resolution (or just crash), with a useful ImplError it has a ton more options.

therealprof on 30 Sep 2020

I am sure people will come up with more.

Which is encouraged. But if you can't come up with additions in a hurry I have my doubts that we're going to be swamped with an unwieldy number of requests. 😏

My ability to come up with examples right on the spot is beside the point. A high amount of requests within a short period of time (leading to "swamping") is also beside the point.
The point is that the number of ImplError error variants is expected to grow, which adds to my point of an unwieldy number of ever-so-slightly-different error variants.

A driver getting an OutputPin should not care / cannot care about whether the pin comes from the MCU or through an I2C multiplexer. It really cannot handle ImplError::PowerDown.

Maybe not PowerDown but it may want to retry a few times if it receives an ImplError::Timeout.

Again, Timeout is already part of I2cError, not ImplError. A generic driver would be able to handle it without needing ImplError.

While ImplError allows to get a bit more of information, it cannot tell exactly what went wrong without making ImplError overwhelming (because only the implementation knows), which is precisely my point.

It doesn't need to tell exactly what went wrong but if it can point into the right direction that is a huge help. Without it all a non-device specific application can only do a best-effort problem resolution (or just crash), with a useful ImplError it has a ton more options.

If it can only point into the right direction, you will still need to process the implementation-specific error if you want to fix it or know what actually happened.
We may as well cut the middle-man and just encourage applications to choose ImplError as their implementation error type if they are satisfied with the level of information. That would keep the errors simple.

Why force an additional set of somewhat unrelated errors into every error definition if we already have a mechanism to get the underlying information?

eldruin on 30 Sep 2020

Why force an additional set of somewhat unrelated errors into every error definition

They are not unrelated, they are general system errors which apply to any peripheral which is exactly why they are in every error definition.

if we already have a mechanism to get the underlying information?

That mechanism is useless in the vast majority of cases because you're back to having device specific error handling which is exactly what we need/want to get away of. I have honestly no idea why some people are insisting on having it but for me this is a compromise I'm willing to make as long as we don't loose the ability to do universal error handling.

therealprof on 30 Sep 2020

To define the set of enum, what about checking a few existing errors in hal, and see how we would transfer it to the corresponding ErrorKind.

For example, the error used in https://github.com/stm32-rs/stm32f4xx-hal/pull/218/files look quite protocol related and should fit in the ErrorKind type.

OTOH, the DMABufferNotInDataMemory error in nrf-hal doesn't seem appropriate for a dedicated entry in kind, and, I think, would enter in the Other case.

linux embedded hal is not really helpful as it seems to directly use std::io::Error as its error type.

Other seems to be the right name, as it is also used in std::io::ErrorKind::Other https://doc.rust-lang.org/std/io/enum.ErrorKind.html#variant.Other

So, maybe we can begin with a small set of variant in the embedded-error kind types, try to use it in a few hal, and see if it need additions. It would be semver compatible.

The only thing to really choose are:

if we add some "generic" error that apply to every errors (as OS related errors, as permission denied and the like, corresponding to the actual ImplError) and its name. ImplError is not really talkative, maybe GenericError or something like that?
if we use a "generic" error, does the Other variant belong to the generic error, or to the specific errors.

TeXitoi on 1 Oct 2020

👍1

@TeXitoi I don't think what HAL impls are doing right now is indicative for what they will look like after we make the change. Since all the moment all error types are bespoke there's no right or wrong choice.

So, maybe we can begin with a small set of variant in the embedded-error kind types, try to use it in a few hal, and see if it need additions. It would be semver compatible.

That's exactly what I did: Considered a few usecases and defined the variants for them.

if we add some "generic" error that apply to every errors (as OS related errors, as permission denied and the like, corresponding to the actual ImplError) and its name. ImplError is not really talkative, maybe GenericError or something like that?

The thought behind the name ImplError was to suggest implementation specific errors. I'm not attached to the name, only to the concept. 😉

I'd be okay adding an Other variant to ImplError but I'd rather call it something like Custom or Extended to indicate that there would is additional custom information available requiring a custom implementation to extract.

therealprof on 1 Oct 2020

@TeXitoi I don't think what HAL impls are doing right now is indicative for what they will look like after we make the change. Since all the moment all error types are bespoke there's no right or wrong choice.

But it gives a good idea of the practical errors. I agree that it doesn't show anything in error analysis.

So, maybe we can begin with a small set of variant in the embedded-error kind types, try to use it in a few hal, and see if it need additions. It would be semver compatible.

That's exactly what I did: Considered a few usecases and defined the variants for them.

As @eldruin thinks there is too much kinds, I propose to begin with a really minimal set. For example, I didn't find a concrete example of GpioError::WrongMode and ImplError::OutOfMemory for example. We can always add them later, that's not a breaking change.

if we add some "generic" error that apply to every errors (as OS related errors, as permission denied and the like, corresponding to the actual ImplError) and its name. ImplError is not really talkative, maybe GenericError or something like that?

The thought behind the name ImplError was to suggest implementation specific errors. I'm not attached to the name, only to the concept. wink

impl has a meaning in rust, thus I didn't understand the name. As it is added to every error kinds, I thought it represents some kind of error that are applicable everywhere.

TeXitoi on 1 Oct 2020

As @eldruin thinks there is too much kinds, I propose to begin with a really minimal set. For example, I didn't find a concrete example of GpioError::WrongMode and ImplError::OutOfMemory for example. We can always add them later, that's not a breaking change.

My understanding of the concern is that in the future there will be too many kinds, which I totally disagree with -- there cannot be too many error kinds: when an error can occur and is not represented by a similar kind yet then it should be added and there should be a very low barrier to getting it in.

Indeed there's no concrete examples of those kinds yet, mostly because there're only very few implementations at the moment. It makes little sense to me to cut down on error kinds which have been already identified only to add them again later when someone complains about them missing. I do already have plans to use GpioError::WrongMode in a GPIO port expander driver.

impl has a meaning in rust, thus I didn't understand the name. As it is added to every error kinds, I thought it represents some kind of error that are applicable everywhere.

Yes, implementation specific errors are applicable everywhere and it's the only common variant on all peripheral specific kinds to express problems caused by HAL implementations or drivers. If you can think of a better name, bikeshed away. 😅

therealprof on 1 Oct 2020

I do already have plans to use GpioError::WrongMode in a GPIO port expander driver.

Shouldn't it be handled by type state?

If you can think of a better name, bikeshed away.

I have GenericError, but I'm not convinced.

TeXitoi on 1 Oct 2020

We have barely started using embedded-error (AFAIK) and right now, embedded_error::I2cError already has two different timeout error variants:

I2cError::Timeout (intended for SMBus timeout)
I2cError::ImplError(ImplError::TimedOut) ("operation timed out, please retry")

Even written differently.
I believe this can be confusing.

eldruin on 6 Oct 2020

We have barely started using embedded-error (AFAIK) and right now,

Sure. We're kind of in a gridlock (again, sic!). For reference, I was trying to introduce embedded-error into various HAL impls, starting with stm32f4xx-hal when someone restarted the discussion: https://github.com/stm32-rs/stm32f4xx-hal/pull/218 . It doesn't make sense to me to implement embedded-error everywhere when people are still requesting major changes.

embedded_error::I2cError has already two different timeout error variants:

I2cError::Timeout (intended for SMBus timeout)

I2cError::ImplError(ImplError::TimedOut) ("operation timed out, please retry")

Even written differently.
I believe this can be confusing.

They do have a different purpose. Anything in the peripheral specific Error is, well, peripheral specific (although we could discuss if we actually want to have a separate Error type for SMBus), anything in ImplError is implementation specific. Timeout is coming coming from the an actual implementation (and I think it might be called that way in the reference manual, too), TimedOut was made up by me; I don't have any real preference though I don't think calling them the same would make things clearer but I'm open to suggestions.

therealprof on 6 Oct 2020

We have barely started using embedded-error (AFAIK) and right now,

Sure [...]

No criticism there. I am trying to highlight that despite a small number of errors (so far), there is already a case of _similar-but-actually-different_ error variants.
I understand these two timeout error variants. However, as I wrote, I believe they can be be confusing, depending on your familiarity with HAL implementations/Rust (embedded) experience.

When more variants are added, this may become more complicated.

there cannot be too many error kinds: when an error can occur and is not represented by a similar kind yet then it should be added and there should be a very low barrier to getting it in.

Exactly because of this, I believe more _similar-but-slightly-different_ error variants will be available in the future and error handling on the user side will become more confusing.

I believe it would be better for everybody if operations only return their own error variants and use Other for any arbitrary "implementation/generic" errors.

eldruin on 6 Oct 2020

I believe it would be better for everybody if operations only return their own error variants and use Other for any arbitrary "implementation/generic" errors.

As mentioned before that's an absolute no-go for me: the only thing I deeply care about is the ability to handle common errors generically and Other takes exactly that ability away. If someone doesn't care for those error kinds they can simply do a wildcard match and get exactly the same effect, the other way around is not possible.

therealprof on 6 Oct 2020

I believe what most people need is to handle protocol-own errors, especially I2C's NACK, not underlying errors.

ImplError started as a later addition to forward underlying errors. @therealprof wrote:

Since there seems to be an interest in relaying underlying error causes to a user, how about we add a universal ImplError enum with a number of common error cases and add this to each of the trait specific Error variants?

eldruin on 6 Oct 2020

I believe what most people need is to handle protocol-own errors, especially I2C's NACK, not underlying errors.

I don't. Quite a few people actually declared it to be a deal breaker not to have the ability to declare custom errors which is even a step further in that direction (and something I do not agree with but I'm willing to compromise on).

ImplError started as a later addition to forward underlying errors. @therealprof wrote:

That is correct, I missed that in the initial proposal and added it after a ton of discussion and requests. So?

therealprof on 6 Oct 2020

I am one of the persons that thinks that forwarding underlying error information must be possible.
That is possible without ImplError.
ImplError is only about standardisation of underlying error causes. That is a _second_ level of standardisation.

I believe error types would be more flexible without attempting to represent every possible hardware failure situation inside each error type. This information can be transported _separately_ with the approach from proposal 3.

ImplError started as a later addition to forward underlying errors. @therealprof wrote:

That is correct, I missed that in the initial proposal and added it after a ton of discussion and requests. So?

So, standardisation of _underlying errors_ was not the most pressing need in the community.

eldruin on 6 Oct 2020

👍1

I am one of the persons that thinks that forwarding underlying error information must be possible.

I am aware. And again that is a compromise I'm willing to make.

I believe error types would be more flexible without attempting to represent every possible hardware failure situation inside each error type. This information can be transported separately with the approach from proposal 3.

If you can get to the underlying error, you can get to the underlying error no matter what the error value is. So there's absolutely no difference in flexibility.

So, standardisation of underlying errors was not the most pressing need in the community.

Err, you certainly have a way of twisting words in your favour. 🙃

therealprof on 6 Oct 2020

Err, you certainly have a way of twisting words in your favour. :upside_down_face:

Please refrain from personal attacks.

dbrgn on 6 Oct 2020

👍3

@dbrgn That's not an attack but an observation. A lot of stuff was added after the initial approach after a ton of discussion; I don't see why this specific addition has any lesser value than the downcasting which was suggested even later.

therealprof on 6 Oct 2020

Embedded-hal: [RFC] Revamp `Error` types

Most helpful comment

All 123 comments

A generic error type with NACK

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

1a: Non-generic Error Enum

1b: Non-generic Error Enum plus ImplError

2: Standardized Error enum plus Other(E)

3: Generic with Type Conversion

`ImplError`

Summary

The Error to rule them all

Convertible Error

An Error bound on the trait with a `kind` method

Conclusion

Related issues

Embedded-hal: [RFC] Revamp `Error` types

Most helpful comment

All 123 comments

A generic error type with NACK

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

1a: Non-generic Error Enum

1b: Non-generic Error Enum plus ImplError

2: Standardized Error enum plus Other(E)

3: Generic with Type Conversion

ImplError

Summary

The Error to rule them all

Convertible Error

An Error bound on the trait with a kind method

Conclusion

Related issues

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

`ImplError`

An Error bound on the trait with a `kind` method