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

I2c: simplify, expand docs, document shared bus usage. #440

Merged
merged 6 commits into from
Mar 28, 2023

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Mar 7, 2023

Depends on #441 -- check that one out first.

This does some simplifications to the trait that I think we should do:

  • Implement all methods in terms of transaction. This way HALs have to implement just that.
  • Removed byte-wise-iteration methods: write_iter and write_iter_read. The reason is that they're quite inefficient, especially with DMA implementations. We've already removed these on other traits, so I think we should do as well here.
  • Removed transaction_iter. I don't think it's much useful in practice, because the way iterators work all the yielded Operations must have the same lifetime. This means that, even if the user can generate the Operations on the fly, they can't allocate buffers for these on the fly, all buffers must be pre-allocated. So it's not useful for, say, streaming a large transfer by reusing some small buffer repeatedly. See I2C transaction_iter should take an iterator of mutable references #367
  • Removed useless lifetimes
  • Standardized buffer names on read and write, I think they're clearer.

It also specifies how i2c bus sharing is supposed to work. This is an alternative to #392 . After the discussions there, I don't think we should split I2C into Bus and Device anymore. For SPI it makes sense, because drivers want to enforce that there's a CS pin (SpiDevice) or not (SpiBus). This is not the case with I2C, the API is exactly the same in the shared and non-shared case. Drivers shouldn't care which case it is.

So all we have to do to "support" bus sharing is docs, This PR does:

  • Document that it's allowed for implementations to be either shared or not.
  • Document some guidelines for drivers and HALs on how to best use the traits, expand the examples.

@Dirbaio Dirbaio requested a review from a team as a code owner March 7, 2023 23:16
@Dirbaio Dirbaio mentioned this pull request Mar 7, 2023
6 tasks
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 analysis and looks good to me, thank you!
I just added some documentation improvements.

TODO:

  • update changelog

embedded-hal/src/i2c.rs Outdated Show resolved Hide resolved
embedded-hal/src/i2c.rs Outdated Show resolved Hide resolved
embedded-hal/src/i2c.rs Outdated Show resolved Hide resolved
@eldruin eldruin mentioned this pull request Mar 15, 2023
@eldruin eldruin added this to the v1.0.0 milestone Mar 15, 2023
@eldruin eldruin mentioned this pull request Mar 15, 2023
@Dirbaio Dirbaio changed the title I2c: expand docs, document shared bus usage. I2c: simplify, expand docs, document shared bus usage. Mar 15, 2023
@Dirbaio
Copy link
Member Author

Dirbaio commented Mar 15, 2023

Done

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.

Alright, let's get this merged.
Thank you for your work!
bors r+

@bors bors bot merged commit 1d74e89 into rust-embedded:master Mar 28, 2023
bors bot added a commit that referenced this pull request Mar 28, 2023
445: bus/i2c: add RefCell, CriticalSection and Mutex shared bus implementations. r=eldruin a=Dirbaio

Requires #440 

Same as #443 but for I2C.


This adds a few bus sharing implementations, with varying tradeoffs:
- `RefCellDevice`: single thread only
- `CriticalSectionDevice`: thread-safe, coarse locking, nostd.
- `MutexDevice`: thread-safe, fine-grained locking, std only.

Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
tommy-gilligan added a commit to tommy-gilligan/rp-hal that referenced this pull request Apr 17, 2023
tommy-gilligan added a commit to tommy-gilligan/rp-hal that referenced this pull request Apr 17, 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.

3 participants