JohnDoneth / hd44780-driver

Implementation of the embedded-hal traits for the HD44780.
MIT License
37 stars 40 forks source link

Handle IO errors by propagation #53

Closed ColinTimBarndt closed 2 weeks ago

ColinTimBarndt commented 2 weeks ago

I've added error handling to all IO operations (especially I2C). My motivation is that this would have saved my hours of debugging because I though that communication with my display was successful. I was unaware that the driver ignored any errors. My implementation propagates those errors to the user, which can then decide what to do with them (or keep ignoring).

I have only tested this on blocking I2C so far. I am very confident that this would work everywhere else, though I'd prefer to first test it on my display. Sadly, mine can only do I2C, though the other implementations didn't change much.

I'd be happy about any feedback on this. Especially because this PR has some breaking changes due to generics that have been added to the Error struct and many functions. Also, all pins must have the same error type with this implementation. It is technically possible to drop this requirement, but it would make the generics on Error explode.

Example

Here's an example stack trace when unwrapping an Err result caused by me unplugging the CLK jumper. Previously, the program wouldn't have been able to notice this error as it would have been ignored and dropped by the driver.

====================== PANIC ======================
panicked at src/main.rs:46:35:
called `Result::unwrap()` on an `Err` value: Io { port: I2C, error: AckCheckFailed }

Backtrace:

0x400d0d96
0x400d0d96 - display_1602::__xtensa_lx_rt_main
    at ??:??
0x400d488e
0x400d488e - Reset
    at /home/colin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/xtensa-lx-rt-0.17.1/src/lib.rs:82
0x400d3f29
0x400d3f29 - ESP32Reset
    at /home/colin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-hal-0.20.1/src/soc/esp32/mod.rs:118
ColinTimBarndt commented 2 weeks ago

Solves #50

ColinTimBarndt commented 2 weeks ago

Another important (possibly breaking) change that I added is that get_position no longer panics. It was already returning a Result, though it was only using the Ok variant and panics otherwise. There's an Error::Position now for this purpose.

su-sd commented 1 week ago

Is there a reason to not implement the trait core::error::Error?

ColinTimBarndt commented 1 week ago

Is there a reason to not implement the trait core::error::Error?

Thanks for pointing that out, that was an oversight.