Devices on a shared SPI bus need different configuration.

[hydra]

As noted from conversations in the embassy matrix chat, devices attached to an SPI bus often have different characteristics, such as maximum bus speed, polarity, phase and bit order. This config can also differ for different operations on the SAME device. e.g. some SPI devices have a maximum speed for initialisation commands and then a high speed for read commands. thus the config needs to be able to be changed for the device once it's created.

https://github.com/embassy-rs/embassy/blob/master/embassy-embedded-hal/src/shared_bus/spi.rs#L62

The signature of SpiBusDevice::new is currently:

pub fn new(bus: &'a Mutex<M, BUS>, cs: CS)

it feels like it should be:

pub fn new(bus: &'a Mutex<M, BUS>, cs: CS, config: Config)

and that once you've got a an SpiBusDevice instance you should be able to supply a new config either a) on a per transaction basis, or b) arbitrarily.

equally, to be performant, the spi bus should not be reconfigured if the last config is the same as the one previously used. that is:
a) don't reconfigure the spi peripheral if the last transfer for the same device uses the same config.
b) don't reconfigure the spi peripheral if the last transfer for a different device uses a config with the same settings.
and perhaps: c) only reconfigure the aspects of the spi device that are actually different, e.g. if the speed is not the same change it, and don't change the polarity/phase if they are the same, but this checking might actually be slower and take more code space than just re-configuring the SPI peripheral registers...

[Dirbaio]

The core issue is the embedded-hal traits don't allow changing the config, so a SpiDevice impl using just the SpiBus traits can't do it.

There's been talks of adding something like it, but there's not been much luck since it's all very HAL-specific. rust-embedded/embedded-hal#79

Perhaps we could do it on our own, having a SetConfig trait, and then a SpiDeviceWithConfig that sets the config on every transaction.

[hydra]

@Dirbaio You certainly don't want to set the config on every transaction when dealing with devices that need to be read at 32khz. Ideally, peripheral config should be refreshed on an as-needed basis, as above.

[Dirbaio]

The impl could check if the new config is equal to the current one, then skip configuring if so.

Anyway, setting the config is just a bunch of reg writes so it's extremely cheap. Doing this checking could make it slower vs simply doing the reg writes without checking.

[hydra]

The impl could check if the new config is equal to the current one, then skip configuring if so.

Anyway, setting the config is just a bunch of reg writes so it's extremely cheap. Doing this checking could make it slower vs simply doing the reg writes without checking.

yes, I did mention that above too 😄

[Dirbaio]

done in #850