Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 19, 2023

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented May 10, 2023

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.

  • write-only SPI: for SPI displays you don't want to read from, or to abuse it to bitbang stuff like ws2812b,
  • read-only SPI: some ADCs that can't be configured at all, you just clock out bits. Or even weirder abuses, like to build a logic analyzer

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.

@Dirbaio Dirbaio requested a review from a team as a code owner May 10, 2023 20:41
@Dirbaio Dirbaio force-pushed the spi-remove-unidirectional branch from c87f597 to 1567f25 Compare May 10, 2023 20:43
Copy link
Contributor

@almindor almindor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defs a fan of simplification, seems good to me. other @rust-embedded/hal folks?

@Vollbrecht
Copy link

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 😺

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the assessment, thank you!

@Dirbaio
Copy link
Member Author

Dirbaio commented May 19, 2023

let's merge then! :D

bors r+

@bors
Copy link
Contributor

bors bot commented May 19, 2023

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.

@bors bors bot merged commit 30a8190 into rust-embedded:master May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants