RFC: Use the SPI traits to represent a single peripheral on an SPI bus

[GrantM11235]

What?

Currently, the SPI traits are used to represent an entire SPI bus. I propose using them to represent a single peripheral on an SPI bus.

Why?

Generic device drivers want to be able to communicate with an SPI device or devices, they don't need access to the whole bus. In fact, giving the entire bus to a driver has a number of downsides:

• The driver must manually toggle it's chip select. This is inconvenient and can lead to problems like Clarify SPI timing behaviour (currently completely broken!) #264.
• Because the driver must hold the SPI bus struct (or at least a &mut), nothing else can use that bus for the lifetime of the driver. shared-bus exists to work around this problem, but it has limitations (edit: and it is unsound SpiProxy is still unsound, even in a single thread Rahix/shared-bus#23) due to the manual handling of chip selects.
• A driver that communicates with two SPI devices must decide whether to take one bus (and only work if both devices are on the same bus) or two buses (and only work if the devices are on separate buses).
• A driver has no way to communicate with two SPI devices on the same bus if they use different bus configurations (polarity, phase, speed) because there is no way to change the configuration of the bus using embedded_hal.

How?

embedded_hal

We don't need to actually change any code in embedded_hal, we just need to mandate that any implementations of the spi traits must hold their chip select pins low for the duration of the transfer.

This is similar to #180/#245, but it would be mandatory, not optional.

Generic device drivers

Generic device drivers can be updated by simply removing any manual toggling of chip select pins.

HALs

This is where it gets interesting...

HAL implementations

Instead of implementing the SPI traits directly on an spi bus, HALs will instead create device structs. Here are some simplified examples:

A device struct which shares it's bus:

pub struct Device {
	bus: Mutex<Spi1>,
	cs_pin: OutputPin,
	config: Config,
}

impl spi::Write<u8> for Device {
	fn write(&mut self, words: &[u8]) {
		self.bus.lock(|bus| {
			bus.reconfigure(self.config);
			self.cs_pin.set_low();
			bus.write(words);
			self.cs_pin.set_high();
		});
	}
}

A device struct with exclusive access to it's bus, which doesn't need a mutex and doesn't need to reconfigure the bus every time:

pub struct ExclusiveDevice<'a> {
	bus: &'a mut Spi1,
	cs_pin: OutputPin,
}

impl<'a'> spi::Write<u8> for ExclusiveDevice<'a> {
	fn write(&mut self, words: &[u8]) {
		self.cs_pin.set_low();
		self.bus.write(words);
		self.cs_pin.set_high();
	}
}

What could this look like?

let spi1 = hal::spi::Spi1::take();

let lcd_spi = spi1.new_device(lcd_cs_pin, MODE_1, MegaHertz(20));

let sensor_spi = spi1.new_device(sensor_cs_pin, MODE_2, MegaHertz(1));

let mut lcd = Lcd::new(lcd_spi, lcd_reset_pin);

let mut sensor = Sensor::new(sensor_spi);

loop {
    let data = sensor.read_data();

    display_data(&mut lcd, data);
}

Unresolved questions

• What guarantees do we need to provide about the chip select pin? Obviously it needs to be low during a transfer, but do we need to guarantee that it goes high afterwards? What about between Transactional operations?
• How hard is it to implement this in HALs?
• How much overhead is introduced by reconfiguring the bus every time a trait method is used? Can it be mitigated with Transactional?
• How would this work for the nonblocking SPI trait?

[eldruin]

Thank you for this proposal. It sure seems interesting.
Just setting expectations here: I might be wrong but I think this is too big to iron out before 1.0.

In my opinion, redefining such behavior without changing code would be difficult to handle across the ecosystem. Driver and HAL writers would only have the embedded-hal version to decide whether they need to handle CS or not. These changes are not obvious when updating and might lead to broken code.
I would rather favor adding a marker trait like ManagedCS.

[Rahix]

With #351, something similar is now implemented so this issue can be closed I think...