-
Notifications
You must be signed in to change notification settings - Fork 93
Description
Disclaimer: I'm not actively using this crate right now. I was made aware of this potential problem by @Maldus512 (in Rahix/shared-bus#18) and wanted to start a discussion about it here.
Currently, the SdMmcSpi
type is based on the FullDuplex
trait from embedded-hal. The contract for FullDuplex
has requirements on the order of calls to its methods which makes it impossible to allow sound bus sharing in the way shared-bus
works. Quoting my comment in Rahix/shared-bus#18 with the reason why:
[...] the
FullDuplex
trait unfortunately has an API that cannot be safely shared in the wayshared-bus
solves sharing. The reason is that there is an implicit dependency between calls tosend()
andread()
which could be broken if multiple bus users interweave these calls. For example:User 1 User 2 | | send() | | | | send() - - # Now received byte from first send is discarded (or worse) | | read() - - - - - - - - - - # Reads data from second write | read() - - # HAL-API violation!!
To allow bus sharing with SdMmcSpi
, I would suggest switching embedded-sdmmc
to use the traits from embedded_hal::blocking::spi
instead as they do not suffer this problem and can be used for bus sharing more safely (though still not concurrently across multiple execution contexts¹).
If I am not mistaken, the only place where this change would need to happen would be this method:
embedded-sdmmc-rs/src/sdmmc.rs
Lines 417 to 422 in 6071df4
/// Send one byte and receive one byte. | |
fn transfer(&self, out: u8) -> Result<u8, Error> { | |
let mut spi = self.spi.borrow_mut(); | |
block!(spi.send(out)).map_err(|_e| Error::Transport)?; | |
block!(spi.read()).map_err(|_e| Error::Transport) | |
} |
By using embedded_hal::blocking::spi::Transfer
instead, I think it could just look like this:
/// Send one byte and receive one byte.
fn transfer(&self, out: u8) -> Result<u8, Error> {
let mut spi = self.spi.borrow_mut();
spi.transfer(&mut [out])
.map(|b| *b[0])
.map_err(|_e| Error::Transport)
}
(Of course, by doing a larger change, this could be done more efficiently).