rust-embedded / embedded-hal

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

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

Closed adri326 closed 2 months ago

adri326 commented 2 months ago

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.

Dirbaio commented 2 months ago

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

adri326 commented 2 months ago

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

Dirbaio commented 2 months ago

either separate or this one works, up to you.

adri326 commented 2 months ago

Might as well then :)