How to maintain an SPI transaction while manipulating GPIO pins?

[ciniml]

Hi.

Recently, the API of the SpiDevice trait has been updated to use a list of Operation instances instead of passing a closure.

embedded-hal/embedded-hal/src/spi.rs

Line 342 in 9ac61a7

fn transaction(&mut self, operations: &mut [Operation<'_, Word>]) -> Result<(), Self::Error>;

Before the API change, I had written code in the implementation of a driver for the ILI9341/ILI9342 SPI LCD, as shown below, to toggle the DC (Data/Command) pin while the CS is asserted.

// self.spi is a field whose type is `SpiDevice`
// Send command byte [0x04] while DC is low (which indicates command byte)
// Then change DC to high and read thee data bytes into `buffer`
let mut buffer = [0, 0, 0]; 
self.spi.transaction(|bus| {
    self.pin_dc.set_low().unwrap();
    bus.write(&[0x04])?;
    self.pin_dc.set_high().unwrap();
    bus.read(&mut buffer)?;
    Ok(())
})

After the API was changed, it appears that toggling the DC pin within a transaction has become difficult.
So, my question is, how can I implement a similar function using the new API?
I couldn't find any examples to do it.

[Dirbaio]

You can use SpiBus instead of SpiDevice, managing CS and DC pins manually.

The closure design had some downsides (it's not implementable on Linux and some RTOSs, which require specifying all the operations upfront, allowing running arbitrary code prevented implementations that would enqueue the whole transaction and handle it from another task/thread, and the closure was troublesome for the async traits). It's a tradeoff: the "operation list" design fixes all these but in exchange doesn't allow supporting DC pins. After discussing the pros and cons, we decided to go for the "operation list" design.

[Dominaezzz]

I'm having this same issue with SD cards and unfortunately I need to share the bus with an EPD display (See M5Paper). Passing the entire bus to the driver kinda sucks for sharing.

Would you ever consider an additional trait that adds the closure design as well?
That way the HAL implementers that can support it are able to expose it and the subset of drivers that require it can demand it.

[ciniml]

So, is it by design?

As @Dominaezzz pointed out, many affordable embedded devices with SPI LCD face the same issue. (I believe there are numerous ESP32 boards with a similar configuration.)

When implementing BSPs for these devices, if sharing SpiBus instead of SpiDevice is required, it becomes challenging to reuse the HAL SPI driver implementation for target MCUs.

Would it be difficult to add a variant to the Operation enum that accepts a closure only if a feature flag is set?

[Dirbaio]

Yes, it's forced by design. :(

Would it be difficult to add a variant to the Operation enum that accepts a closure only if a feature flag is set?

Rust doesn't like putting closures in data structures. It'd need something like &mut dyn FnOnce(...) which is somewhat awkward. Also it wouldn't be implementable on Linux and some RTOSs for the same reason the previous API wasn't implementable.

How common is putting an LCD on a shared bus? LCDs are somewhat performance-sensitive things so I'd imagine bus sharing is avoided in the wild for these.

It is still possible to implement bus sharing though, it requires manually using the bus with something custom only for the display. Something like this:

pub struct RefCellDisplay<'a, BUS, PIN, D> {
    bus: &'a RefCell<BUS>,
    cs: PIN,
    dc: PIN,
    delay: D,
}

impl<'a, BUS, PIN, D> RefCellDisplay<'a, BUS, PIN, D> {
    pub fn new(bus: &'a RefCell<BUS>, cs: PIN, dc: PIN, delay: D) -> Self {
        Self { bus, cs, dc, delay }
    }

    fn transaction(&mut self, command: &[u8], data: &[u8]) -> Result<(), DeviceError> {
        let bus = &mut *self.bus.borrow_mut();

        self.dc.set_low().map_err(DeviceError::Pin)?;
        self.cs.set_low().map_err(DeviceError::Pin)?;
        bus.write(command).map_err(DeviceError::Spi)?;
        bus.flush().map_err(DeviceError::Spi)?;
        self.dc.set_high().map_err(DeviceError::Pin)?;
        bus.write(data).map_err(DeviceError::Spi)?;
        bus.flush().map_err(DeviceError::Spi)?;
        self.cs.set_high().map_err(DeviceError::Pin)?;

        Ok(())
    }
}

would allow creating on the same bus a RefCellDisplay for the SPI display and RefCellDevices for everything else requiring a SpiDevice.

[Dirbaio]

Would you ever consider an additional trait that adds the closure design as well?
That way the HAL implementers that can support it are able to expose it and the subset of drivers that require it can demand it.

we intentionally decided not to have two traits since it would lead to fragmentation.

BTW, HALs are supposed to implement only SpiBus¹, and leave SpiDevice to be implemented on top of SpiBus by crates such as embedded-hal-bus.

So a driver crate for an SPI display could define its own trait, such as:

trait DisplayDevice {
    type Error: Debug;
    fn transaction(&mut self, command: &[u8], data: &[u8]) -> Result<(), Self::Error>;
}

and then provide builtin impls that take an owned SpiBus for no sharing, or RefCell/CriticalSection/Mutex-based versions for sharing that would work well together with embedded-hal-bus, or leave it up to the end user to implement themselves if they want to do something custom.

Footnotes

1. except when the thing underneath manages CS/sharing, such as Linux spidev. This is never the case in bare-metal though. ↩

[Dominaezzz]

It is still possible to implement bus sharing though, it requires manually using the bus with something custom only for the display.

Sure, it is still possible to achieve this at the end of the day but ideally it should be via a trait rather than a concrete implementation per driver. What if I have two devices on a bus that need the closure approach, both libraries may have bus sharing implementations that don't work with each other (leading to fragmentation) or may just not bother to do it at all since the advice you're currently giving to driver implementers is to simply take ownership of the bus.
e-hal is supposed to aid with interoperability and I think an additional trait would help interoperability.

we intentionally decided not to have two traits since it would lead to fragmentation.

I don't really understand what you mean by fragmentation in this case. Either drivers can support closures or they cannot. I can appreciate that v0.2's separate Read, Write, Transfer and TransferInPlace traits do cause fragmentation, as different HAL implementers implemented different subsets of the traits which meant it was difficult to use some drivers with some HALs. However those traits represent fundamental operations we expect any SPI driver to be able to do so it made sense to merge them into one.
What I'm asking for is an extension; A way for an SPI driver to generically expose an advance feature which is the closure method. Something like this (Names pending bikeshed):

trait SpiDeviceWithClosure<Word: Copy + 'static = u8> : SpiDevice<Word> {
    type Bus: SpiBus;

    fn transaction<R>(
        &mut self,
        f: impl FnOnce(&mut Self::Bus) -> Result<R, <Self::Bus as ErrorType>::Error>,
    ) -> Result<R, Self::Error>;
}

Also, if "HALs are supposed to implement only SpiBus" then we don't have to worry about fragmentation, since crates like embedded-hal-bus would do the simple work of implementing this extension trait, no?

leave it up to the end user to implement themselves if they want to do something custom

While this sorta works, I still don't like the fact that users wanting to do something custom now have to know the DisplayDevice's protocol to implement it, on top of their custom SPI solution. All a user should have to worry about is providing an SPI implementation, not a driver's implementation.

Having said all this, I have no solution for the async side of things, as I barely understood the problem/complexity there.

[Dirbaio]

What if I have two devices on a bus that need the closure approach, both libraries may have bus sharing implementations that don't work with each other (leading to fragmentation) or may just not bother to do it at all since the advice you're currently giving to driver implementers is to simply take ownership of the bus.

No, drivers would provide their own "bus interface trait" like the DisplayDevice I proposed above, and provide implementations for it that take either:

• Bus - exclusive, no bus sharing
• &RefCell<Bus> - interoperates with embedded_hal_bus::RefCellDevice
• &Mutex<Bus> - interoperates with embedded_hal_bus::MutexDevice
• &critical_section::Mutex<RefCell<Bus>> - interoperates with embedded_hal_bus::CriticalSectionDevice

This covers 99% of the use cases, and allows sharing the bus with either SpiDevice impls from embedded-hal-bus, or other drivers that do similar traits. You just share the &RefCell<Bus> or similar.

This "bus interface trait with DC pin" could be ina shared crate, even (display_interface maybe?), so that all drivers for displays with a CS pin can use it without having to reimplement it. My main point is it doesn't have to be in embedded-hal to interoperate with embedded-hal SpiDevice.

I don't really understand what you mean by fragmentation in this case.

If we had both SpiDevice and SpiDeviceWithClosure, we'd be putting both on "equal footing". Driver authors could use SpiDeviceWithClosure if it's slightly more convenient for the use case even if they didn't really need it, which would make their driver not work on Linux and some RTOSs. So, adding SpiDeviceWithClosure would decrease interoperability.

On the other hand, not having SpiDeviceWithClosure does not decrease interoperability, it's still possible for drivers with custom use cases like DC pins to share bus by taking &RefCell<Bus> or similar, it's just that they have to write a bit more code. (and probably not even that, if some crate like display_interface pops up that can be reused across all display drivers).

tldr it is good for interoperability to nudge everyone towards using SpiDevice.

While this sorta works, I still don't like the fact that users wanting to do something custom now have to know the DisplayDevice's protocol to implement it, on top of their custom SPI solution.

If the driver / display-interface crate provides all these builtin impls, "something custom" would only be when you're sharing bus with something that's not RefCell/Mutex/CriticalSection, which should be very very rare.

Having said all this, I have no solution for the async side of things, as I barely understood the problem/complexity there.

see this comment and next ones #347 (comment)

tldr is it's not possible to make no-alloc async closures that borrow things in today's Rust.

[Dominaezzz]

Oh wow, I was still under the impression that embedded_hal_bus only had ExclusiveDevice, the new device types are super handy.

No, drivers would provide their own "bus interface trait" like the DisplayDevice I proposed above

The trait you proposed isn't in terms of SPI though, it's in terms of the display protocol, which should be abstracted away.

which would make their driver not work on Linux and some RTOSs. So, adding SpiDeviceWithClosure would decrease interoperability.

I see your point here about convenience but can't this be easily mitigated with embedded_hal_bus?
If someone needs to talk to a "display with dc pin" (requires closure), an SD card (requires closure) and an EPaper display (does not require closure) all on the same SPI bus, but they're on Linux which can't do closures, couldn't they just use embedded_hal_bus to upgrade the Linux driver to have a lambda?

There's also another side to the convenience argument; Without the SpiDeviceWithClosure trait, "lazy" driver implementers would just require full ownership of the bus. In which case, not have having SpiDeviceWithClosure has decreased interop.
I think it's better if "lazy" driver implementers conveniently required SpiDeviceWithClosure rather than conveniently require the whole bus. At least this way, application developers now have the power to bridge the implementation gap.

[Dirbaio]

The trait you proposed isn't in terms of SPI though, it's in terms of the display protocol, which should be abstracted away.

It's somewhat common to do protocol-specific "bus interface traits" like these on drivers. The most common is drivers for chips that can be talked to through either SPI or I2C. The main driver is generic on some "interface" trait that has "read reg, write reg", and provides impls on top for SPI and I2C.

In this case it'd be a similar driver-specific trait, that abstracts over how to do the sharing and manage the CS/DC pins.

It'd even have other advantages, like it'd allow the user to supply a custom impl that uses hardware CS/DC pin support, for example the nrf52840 has it.

I see your point here about convenience but can't this be easily mitigated with embedded_hal_bus?
If someone needs to talk to a "display with dc pin" (requires closure), an SD card (requires closure) and an EPaper display (does not require closure) all on the same SPI bus, but they're on Linux which can't do closures, couldn't they just use embedded_hal_bus to upgrade the Linux driver to have a lambda?

Yes, if we wanted SpiDeviceWithClosure then we could add it and e-h-b would provide impls on top of SpiBus just like SpiDevice now.

My opinion is we don't want it due to

• the interoperability argument in my last post
• complexity. People trying the 1.0 alphas have already been confused by there being 2 traits (SpiBus, SpiDevice). Adding a 3rd trait would make it much worse, especially because the tradeoffs between SpiDevice and SpiDeviceWithClosure are much much more subtle than with SpiBus vs SpiDevice.
• inconsistency between blocking and async (async would either not have it, or have it with gnarly hacks)

At least not for now. IMO we should launch 1.0 with just SpiBus + SpiDevice and see how the ecosystem develops. If they prove to be lacking, then we can consider adding SpiDeviceWithClosure for 1.1 (we can add it backwards-compatibly). I'm 99% sure it won't be the case, I think driver-specific "bus interface traits" with impls for &RefCell<Bus> etc will work nicely.

There's also another side to the convenience argument; Without the SpiDeviceWithClosure trait, "lazy" driver implementers would just require full ownership of the bus. In which case, not have having SpiDeviceWithClosure has decreased interop. I think it's better if "lazy" driver implementers conveniently required SpiDeviceWithClosure rather than conveniently require the whole bus. At least this way, application developers now have the power to bridge the implementation gap.

We can't fix lazy, all we can do is nudge reasonably-non-lazy driver authors to the maximally-interoperable SpiDevice.

[Dominaezzz]

At least not for now. IMO we should launch 1.0 with just SpiBus + SpiDevice and see how the ecosystem develops.

Fair enough

[bugadani]

Do you actually need to toggle D/C though? The datasheet doesn't require a signal level on it for reading, except for a single bit at a specific time:

[ciniml]

@bugadani I'm not sure why the D/CX signal in the timing diagram doesn't specify the level while reading parameters. However, according to the command list and the description of Read display identification information (04h), the D/CX pin must be set to 1 during parameter reading. (https://cdn-shop.adafruit.com/datasheets/ILI9341.pdf p.91)

@Dirbaio Thank you for the explanation. I'll attempt to implement my driver for &RefCell<SpiBus>.

[bugadani]

I seem to stand corrected. Can you please also confirm the read does not work if you implement it as two transactins? It is interesting because if display identification is a feature people are widely interested in, the mipidsi and display-interface crates are also probably interested parties, not just embedded-hal. They work still if you only write to them, using the current abstractions, but neither support reading currently, and this might be a first step to figure out what is actually necessary to achieve that. Maybe we can get away with two transactions without the need of touching embedded-hal, maybe we can't.

[ryankurte]

No, drivers would provide their own "bus interface trait" like the DisplayDevice I proposed above, and provide implementations for it that take either:

Bus - exclusive, no bus sharing
&RefCell - interoperates with embedded_hal_bus::RefCellDevice
&Mutex - interoperates with embedded_hal_bus::MutexDevice
&critical_section::Mutex<RefCell> - interoperates with embedded_hal_bus::CriticalSectionDevice

i do wonder whether we could provide some abstraction over these in e-h-b so drivers can have a single bound that is fulfilled by any mechanism for bus sharing, might be tricky with mutexen' but something like trait BusHandle { fn take(&mut self) -> &mut Bus; } that would be fulfilled by all the above options?

[Dominaezzz]

I would definitely by happy with a construct like that. I'd be happy with any trait (provided by e-hal) that provides a bus.

I've been working on a generic version of the BusHandle trait you suggested. (I admit it's a bit on the complex side)

pub trait Share {
    type Object;
    type Error<'a> where Self: 'a;
    type Guard<'a>: DerefMut<Target = Self::Object> where Self: 'a;

    fn lock(&self) -> Result<Self::Guard<'_>, Self::Error<'_>>;
}

impl<T> Share for Mutex<T> {
    type Object = T;
    type Error<'a> = PoisonError<Self::Guard<'a>> where Self: 'a;
    type Guard<'a> = MutexGuard<'a, T> where T: 'a;

    fn lock(&self) -> Result<Self::Guard<'_>, Self::Error<'_>> {
        self.lock()
    }
}

impl<T> Share for RefCell<T> {
    type Object = T;
    type Error<'a> = BorrowMutError;
    type Guard<'a> = RefMut<'a, T> where Self: 'a;

    fn lock(&self) -> Result<Self::Guard<'_>, Self::Error<'_>> {
        self.try_borrow_mut()
    }
}

Then drivers can ask for a Share<Object = SpiBus> I guess or a Share<Object = I2c> or whatever.

[fu5ha]

It seems a bit silly to me that we would now have an SpiDevice trait which is meant to represent exactly "shared access to an Spi bus for one device on it" but we now need another answer to "share the SpiBus" for "advanced uses." It seems like we should instead just lean on something like Share above for sharing access to the SpiBus and remove the concept of SpiDevice, or alternatively to make SpiDevice actually flexible enough to support all use cases (for example, transaction "lock guard" based approach. Since the transaction lock concept is the key piece of what makes SpiDevice work, as said by the docs).

[fu5ha]

Okay, so after chatting more on Matrix, I understand more why there's a need for multiple sharing interfaces including the current more locked down SpiDevice trait and a more flexible (set of) shared SpiBus interfaces. Also, that different use cases could define their own abstraction traits rather than needing to be provided as part of e-h or e-h-b. I will open a PR that adds some documentation about this to e-h as well as e-h-b as we discussed on Matrix...