rust-embedded / embedded-hal

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

`embedded-hal-bus::i2c::RefCellDevice` should use `Cell<Option<&mut I2c>>` instead #479

Closed Kry-Vosa closed 1 year ago

Kry-Vosa commented 1 year ago

The whole idea is in the title: Since we do not need multiple immutable borrows at the same time, we can use just Cell<Option<&mut I2c>>.

"Borrowing" the reference is just:

let borrowed = cell.replace(None).unwrap();
//do your stuff
cell.replace(Some(borrowed));

(Or something more fancy if you wish)

As for why should we do that, it should (thanks to null-pointer optimization) save a whole isize of memory, which might be actually meaningful on targets like Arduino (or AVR in general), where memory is scarce and likelihood of multiple I2C drivers high.

Kry-Vosa commented 1 year ago

Even better might be another impl with qcell::LCell, mainly to point users to the existing zero-cost abstraction if they really need it.

Dirbaio commented 1 year ago

So I don't think it's worth it, I'd prefer to keep the simpler, more idiomatic RefCell.

Also, note that nothing prevents external crates from implementing SpiDevice with Cell<Option> or LCell or anything else. It doesn't have to be in embedded-hal-bus, it'll still interoperate with the ecosystem fine thanks to the SpiDevice trait.

Kry-Vosa commented 1 year ago

You are indeed correct and I'm dumb. Sorry for the wasted time. However I believe the point with LCell still stands, it would be nice if embedded-hal-bus provided zero-cost way of sharing buses instead of forcing developers to look elsewhere.