Improvement: Make PinDriver::set_level infallible

[Phosfor]

trafficstars

I propose to make PinDriver::set_level (and relevant other functions and trait implementations) infallible:

Today PinDriver::set_level returns a Result<(), EspError> because it calls gpio_set_level which itself might return an error. However, the only possible error gpio_set_level can return is ESP_ERR_INVALID_ARG (see here). This error is returned if the provided pin is not a valid output pin.

With Rust's type-system we can guarantee that this function is never called with an invalid pin number at compile-time. The esp-idf function gpio_set_level should therefore always return ERR_OK, making PinDriver::set_level infallible. This in turn would allow making the implementation of the InputPin- and OutputPin-traits from the embedded-hal crate infallible.

A special case are the Any*Pin types as they might contain an invalid pin number. However this can already create unexpected behaviour today (e.g. reading never returns an error).

My motivation for this change came when I wanted to use a driver crate with an ESP. It requires OutputPin::Error to be Infallible. Even though I think this is a problem with their API, it got me thinking. I noticed that it would be possible to guarantee that gpio_set_level would never return an error at compile-time as described above.

If you think this would be a helpful improvement and don't see any mistake with my reasoning I would be willing to work on the implementation 😉

[ivmarkov]

First of all, you are completely right that a driver crate cannot assume anything about the returned errors, beyond what the embedded-hal contract had specified, so it is their problem.

Regarding the Any*Pin types - in theory they cannot contain invalid pin numbers, as the only way to get an Any*Pin instance is to downgrade a typed pin into Any*Pin, and there you still have the warranty that the returned pin number is valid (that is, unless you use the unsafe new constructor, but then you are on your own anyway, and you are not supposed to use it unless you know what you are doing).

Regarding switching to infallible API for PinDriver and switch to Result<T, Infallible> in the trait implementations two concerns:

• For the latter, you have to make sure that all methods called from trait implementations can be reworked as infallible (i.e. all errors can be caught up by the type system). If there is even one method that can fail at runtime, the plan would likely not work. What we can do in that case is keep the Result<T, EspError> in trait implementations but switch those methods on PinDriver that can be made infallible to infallible. We are in a little bit this situation already with PinDriver::get_level & friends, which are infallible on the PinDriver level, but their impls in the e-hal traits are fallible. In any case, we should at least panic if the C gpio impl returns an error code, as that would be a situation which is not expected and should be considered a bug.
• The EspError thing is an open-ended API. It is very unlikely, but what happens if ESP-IDF introduces a new error code in future, that we cannot defend against using Rust type-safety? We would be caught in a trap by declaring our trait implementations to return an Infallible error type?

[Phosfor]

Thank you for your fast reply. For clarification: The driver crate was only the reason I was looking into this and is not the reason I want to change this. I since heavily modified that crate to fit my needs anyway but I still thought (and still think) that it would be nicer to move the checks (done by gpio_set_level in the ESP-IDF) from runtime to compile-time.

Regarding Any*Pin: that is pretty much what I thought.

Now to your concerns:

• As you described, the first step would be to make PinDriver::set_level infallible. Then one would have to check again that all methods called from the trait implementations are infallible (or make them infallible if possible). But even if that is not possible, making PinDriver::set_level infallible alone would be nice in my opinion.
• It is true that ESP-IDF might change their API to include more possible errors for gpio_set_level or even return errors from other functions, for example, gpio_get_level. However I think that the API of these functions is very unlikely to change in the foreseeable future; and even less likely in a way that we cannot guarantee that they cannot fail with Rust's type-system. Of course it is still possible, but I think that a change in the underlying framework's API would justify a change to our own API.

[Vollbrecht]

There are some upstream changes in embedded-hal. embedded-hal digital has now an ErrorKind enum for gpio's. This will probably lead us to create an wrapper Error(something like GpioError) for it, similar to the cases of spi and i2c. An example of it is implemented in PR #224

[ivmarkov]

I think in the meantime the conversation is not so much about e-hal, but for the public methods on PinDriver itself - whether we should make these infallible. The e-hal impl can still be fallible.

I have not pushed here further for this infallibility because I'm not sure whether the benefit would be so big as to justify the effort and the risk if some of the esp idf methods do become fallible at some point.