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

Extract SPI exclusive device into shared crate #398

Merged
merged 2 commits into from Aug 17, 2022

Conversation

eldruin
Copy link
Member

@eldruin eldruin commented Aug 10, 2022

Some concerns have been raised regarding the error handling in spi::blocking::ExclusiveDevice.
I am not sure what the problem was, though.
I think @GrantM11235 was the one to raise the concerns so maybe he can elaborate and then I can improve this description.
Since that struct is just a helpful wrapper for a very specific situation, I propose we move it out of embedded-hal itself. For example into an embedded-hal-shared crate (or some other name, feel free to bikeshed).

We should note that we (probably) have the same problem in embedded-hal-async where there is also an ExclusiveDevice for SPI, so if we create such a crate, we should agree on what to put in there:

  1. Only blocking ExclusiveDevice
  2. blocking + async ExclusiveDevice
  3. any of the above + RefCell-based sharing
  4. any of the above + some mutex / critical section

We should also note that the great shared-bus crate also solves some of these problems so we might also put (some?) of this there.

cc: @Rahix

@eldruin eldruin requested a review from a team as a code owner August 10, 2022 16:04
@rust-highfive
Copy link

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

@eldruin eldruin added this to the v1.0.0 milestone Aug 10, 2022
@Dirbaio
Copy link
Member

Dirbaio commented Aug 10, 2022

👍 to splitting, this way we can do breaking changes to these without disturbing the main crate.

or some other name, feel free to bikeshed

not sure on -suared, it's not only for shared buses (ExclusiveDevice isn't shared). Maybe embedded-hal-bus, embedded-hal-util, embedded-hal-adaters?

@GrantM11235
Copy link
Contributor

My concerns with ExclusiveDevice are mostly theoretical, I just don't want us to be stuck with it forever (or until embedded-hal 2.0, whichever comes first 😁 ) in the event that we want to make breaking changes to it.

I like the name embedded-hal-adapters because it would also be a good place for other non-bus-related adapters, such as these blocking-to-async adapters.

@adamgreig
Copy link
Member

Maybe embedded-hal-extras?

@eldruin
Copy link
Member Author

eldruin commented Aug 17, 2022

In yesterday's weekly meeting we discussed a bit about this and I think I like the proposal of naming this embedded-hal-bus and put in there blocking and async ExclusiveDevice implementations. We could also add at least some RefCell-based or other bus/device connection implementations.
I think that is enough stuff to grant its own crate, which would all be about buses, fitting its name nicely and keeping the focus tight.
For additional stuff like async<->blocking adapters, we could also have an additional crate but we can discuss that in a separate issue.

@eldruin eldruin mentioned this pull request Aug 17, 2022
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

-bus LGTM. Better than -adapters or -util, that'd inevitable become a dumping ground of random stuff. 👍

bors r+

@bors bors bot merged commit da1a4d5 into rust-embedded:master Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants