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

Add a Rc<RefCell<Bus>>-based implementation of SpiDevice and I2C #613

Merged
merged 5 commits into from
Jul 26, 2024

Conversation

adri326
Copy link
Contributor

@adri326 adri326 commented Jul 5, 2024

The current implementation of RefCellDevice for sharing an SPI bus using RefCell works fine, but restricts users of the library into patterns where the compiler is able to guarantee that the bus reference will outlive all usage of its devices.

In my current work, this isn't really the case, as I would like to hide the SpiBus/OutputPin details behind some structures a shared trait, so that my code can interface with foreign microcontrollers as it would interface with its local peripherals.
This means that I either need to manually manage the memory used for the SpiBus, or I need to use something like Rc to safely manage it for me.

Right now, I can create an ad-hoc ExclusiveDevice with the reference from my Rc<RefCell<SpiBus>> whenever I need to do a transaction, and drop it immediately after, but this is a bit of a hacky solution.

This PR proposes a more ellegant approach, which simply offers an alternative to RefCellDevice that uses reference-counting for its SPI bus.

I hesitated on adding a utility method for acquiring a copy of the Rc<RefCell<SpiBus>>, but I decided against it, since this is a rather niche need and one could just keep a Weak<RefCell<SpiBus>> around and not occur any penalty.

I'm unsure if I did things well regarding adding a new feature, alloc, and making sure that it renders correctly on docs.rs

Edit: also added an implementation for I2C, for consistency. I noticed that the I2C implementations could benefit from Clone, but that's out of the scope of this PR.

@adri326 adri326 requested a review from a team as a code owner July 5, 2024 13:57
@Dirbaio
Copy link
Member

Dirbaio commented Jul 5, 2024

LGTM! can you add it to I2C too for consistency?

@adri326
Copy link
Contributor Author

adri326 commented Jul 5, 2024

I can, yeah; do you want it in a separate PR?

@Dirbaio
Copy link
Member

Dirbaio commented Jul 5, 2024

either separate or this one works, up to you.

@adri326 adri326 changed the title Add a Rc<RefCell<Bus>>-based implementation of SpiDevice Add a Rc<RefCell<Bus>>-based implementation of SpiDevice and I2C Jul 5, 2024
@adri326
Copy link
Contributor Author

adri326 commented Jul 5, 2024

Might as well then :)

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.

Thank you! Looks good to me.
Could you add some documentation about the alloc feature to the readme and also add an entry to the changelog?

embedded-hal-bus/Cargo.toml Outdated Show resolved Hide resolved
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.

thanks!

@Dirbaio Dirbaio added this pull request to the merge queue Jul 26, 2024
Merged via the queue into rust-embedded:master with commit f91fcbc Jul 26, 2024
8 checks passed
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