rust-embedded / embedded-hal

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

spi: remove write-only and read-only traits. #461

Closed Dirbaio closed 1 year ago

Dirbaio commented 1 year ago

When introducing the Bus/Device split in #351, I kept the ability to represent "read-only" and "write-only" buses, with separate SpiBusRead, SpiBusWrite buses. This didn't have much discussion, as it was the logical consequence of keeping the separation in the already existing traits (Read, Write, Transfer, ...).

Later, in #443, when switching from the closure-based API to the operation-slice-based API, this required adding SpiDeviceRead, SpiDeviceWrite as well, because you can no longer put a bound on the underlying bus with the new API.

This means we now have seven traits, which we can reduce to two if we drop the distinction. So, is the distinction worth it?

I've always been on the fence about it, now I'm sort of leaning towards no.

First, using write-only or read-only SPI is rare.

Second, for it to be useful HALs have to track "read-onlyness / write-onlyness" in the type signature, so a read-only SPI really only implements SpiBusRead and not SpiBus. HALs already have enough generics. For example, Embassy HALs don't implement the distinction (you can create MOSI-only or MISO-only SPIs, but they all impl the full SpiBus, because I didn't want to make the types carry pin information).

Third, it makes the API easier to use. Simpler, users only have to import one trait, docs have all the methods in one place. Much less boilerplate to impl the traits (look at how shorter embedded-hal-bus gets!).

So I think we should remove them.

Vollbrecht commented 1 year ago

spi.rs https://github.com/esp-rs/esp-idf-hal/blob/master/src/spi.rs#L1337 well i can live with reducing that line count :smiley_cat:

Dirbaio commented 1 year ago

let's merge then! :D

bors r+

bors[bot] commented 1 year ago

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here. For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.