imxrt-rs / imxrt-hal

Rust for NXP i.MX RT
Apache License 2.0
141 stars 33 forks source link

Designate LPSPI hardware chip select #131

Open mciantyre opened 1 year ago

mciantyre commented 1 year ago

We're starting to add support for more hardware-managed peripheral chip selects (PCS) in the LPSPI driver (imxrt-rs/imxrt-iomuxc#34). This issue tracks a feature to control which PCS output is used by the LPSPI driver.

I think the baseline API should assume that a user called Lpspi::without_pins() to construct their driver. The other initialization paths assume that users supply the first PCS output, PCS0. Extending those resource-checked initialization paths could happen here, but I consider that a nice to have. Essentially, the feature shouldn't require that we're tracking the user's PCSn pin in the type system.

I'll leave some tips and suggestions as to what might work. No preference for making this a (non)breaking change.

mciantyre commented 1 year ago

Tips, pointers, suggestions:

teburd commented 1 year ago

Some maybe random thoughts on this, I haven't and can't really look too deeply but...

There might be some advantage to tracking a peripheral controlled chip select (pcsX) vs having a gpio controlled chip select in some manner, perhaps using a trait marker.

A SPI peripheral is a bus with potentially many devices hanging off it. Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange. IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature.

I know some folks will even use the SPI peripheral to try and hack up neoled control signaling in which case cs may not even be used!

There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

mciantyre commented 1 year ago

Good thoughts. I'm happy to help with you / others with that deeper look.

Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange.

Lpspi::without_pins() helps us with that, at least today. (I'm using this to bypass the perpetually-incomplete imxrt-iomuxc package.) Nevertheless, we can easily drop the chip select requirement in new as a breaking change.

For readers using imxrt-hal 0.5, I'd expect Lpspi::without_pins() to work for that CS-less neoled control and also more advanced software-managed control.

IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature. [...] There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

I like this. If we're going this route, we could start future proofing for embedded-hal 1.0.The blocking SPI traits in alpha-9 differentiate "no chip select" upport from "hardware-managed chip select" support; I like the latter. I also appreciate how we can always get software-managed chip selects for free through embedded-hal-bus. It's worthy to break our LPSPI API for these goals.


I have no immediate plan to write any of these ideas into code myself. I'm happy to help others put this into the HAL. Leave questions and comments here if you'd like more insight / mentorship.

ameliamariecollins commented 1 year ago

These things are import to me:

Perhaps we may even think of some representation of devices on the SPI bus. In my case, an ST7789V uses Pcs0, and the SD card on the display module uses Pcs1.

ameliamariecollins commented 1 year ago

So the question is where the split happens in the representation. If we have one lpspi object, the split may happens by calling a method that switches to a desired Pcs — but this invites trouble when pre-empted, because even if some intervening code is equipped to store the previous Pcsx selection and restore it, things can go wrong, leaving the hardware device in the wrong state.

Perhaps there's a representation of a combination of SPI bus and a particular chip select, and someone owns the &mut to that. We're attempting to deterministically abstract a tricky situation, so I want to help the user by sweeping the details under the rug and presenting the cleanest possible API at the top level.

ameliamariecollins commented 1 year ago

Good thoughts. I'm happy to help with you / others with that deeper look.

Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange.

Lpspi::without_pins() helps us with that, at least today. (I'm using this to bypass the perpetually-incomplete imxrt-iomuxc package.) Nevertheless, we can easily drop the chip select requirement in new as a breaking change.

For readers using imxrt-hal 0.5, I'd expect Lpspi::without_pins() to work for that CS-less neoled control and also more advanced software-managed control.

IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature. [...] There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

I like this. If we're going this route, we could start future proofing for embedded-hal 1.0.The blocking SPI traits in alpha-9 differentiate "no chip select" upport from "hardware-managed chip select" support; I like the latter. I also appreciate how we can always get software-managed chip selects for free through embedded-hal-bus. It's worthy to break our LPSPI API for these goals.

I have no immediate plan to write any of these ideas into code myself. I'm happy to help others put this into the HAL. Leave questions and comments here if you'd like more insight / mentorship.

As you can see, I'm already agonizing over the possibilities. I will gladly add such a system to HAL (with a bit of help) when the time comes. First, we need to figure out what the shape of this should be.

ameliamariecollins commented 1 year ago

I know some folks will even use the SPI peripheral to try and hack up neoled control signaling in which case cs may not even be used!

My APA102Cs and SK9822s feel seen! 😃

ameliamariecollins commented 1 year ago

Some maybe random thoughts on this, I haven't and can't really look too deeply but...

There might be some advantage to tracking a peripheral controlled chip select (pcsX) vs having a gpio controlled chip select in some manner, perhaps using a trait marker.

A SPI peripheral is a bus with potentially many devices hanging off it. Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange. IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature.

I know some folks will even use the SPI peripheral to try and hack up neoled control signaling in which case cs may not even be used!

There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

I like spi_thingy::with_cs() a lot. It would take a Pin and initialize it. In this case, it would return some object that was comped from the SPI hardware device, and the CS pin it initialized. Reads and writes would always assert the CS pin for safety (and other CS pins would need to be de-asserted), and would be atomic to ensure the device wouldn't be interrupted. Plus then you could have the regular spi::new() great a CS-less SPI for situations where someone wanted to control CS themselves, or if no CS was needed.

So I guess there's a CSPinGroup somewhere in this, that knows all the in-use PcsX pins for a SPI bus and some SPI device has a way to get at their pin and can select()/deselect() it. select() would first deselect other pins, then select the desired pin.

mciantyre commented 1 year ago

If you're curious about how RTIC manages resources, it's documented here. RTIC uses granular critical sections to ensure mutual exclusion of a resource. Here's some pseudo-code to show how a single SPI bus could be shared across two tasks, each using blocking I/O operations.

#[shared]
struct Shared {
    lpspi4: Lpspi<MyPins, 4>,
    // ...
}

#[local]
struct Local {
    pcs0: Pcs0Pin,
    pcs2: Pcs2Pin,
    // ...
}

#[task(shared = [lpspi4], local = [pcs2], priority = 2)]
fn task_a(cx: task_a::Context) {
    let mut buffer = [0; 16];
    cx.shared.lpspi4.lock(|lpspi4| {
        lpspi4.set_chip_select(&mut cx.local.pcs2);
        lpspi4.transfer(&mut buffer).unwrap();
    });
    handle_spi_data(&buffer);
}

#[task(shared = [lpspi4], local = [pcs0], priority = 1)]
fn task_b(cx: task_b::Context) {
    cx.shared.lpspi4.lock(|lpspi4| {
        lpspi4.set_chip_select(&mut cx.local.pcs0);
        lpspi4.write(&[3, 4, 5, 6, 7]).unwrap();
    });
}

Task A uses PCS2 to interact with one device, and task B uses PCS0 to interact with another device. RTIC prevents wrong peripheral states by requiring a lock to access the LPSPI4 bus.

This pseudo-code only has a single object for the SPI bus, or what the latest embedded-hal expresses with *Bus traits. We're not splitting the bus into devices with chip selects, called SpiDevice in the latest embedded-hal. We could support that splitting, and I think it would work in RTIC. But if we're optimizing for RTIC, I'm curious to see what that split device objects look like. We need to manage the locking ourself if we're attempting a SpiDevice implementation.

ameliamariecollins commented 1 year ago

I like this approach. I was looking through the HAL code earlier today. Perhaps a new Lpspi has no initial CS pin, but when set_chip_select is called, it looks it up in a list of CS pins. If it's not there, it configures it and adds it to the list. The select method can be sure to deselect the other CS lines so it doesn't leave two devices selected with their SDOs fighting each other.

mciantyre commented 1 year ago

Sounds great. Feel free to remove the PCS0 requirement from new and from the struct Pins.

I'm interested to see what that list of CS pins looks like. Some maybe gotch-yas and considerations:

In the pseudo-code above, set_chip_select is called with an imxrt_iomuxc pad object that implements a LPSPIn PCSx function. The user implicitly manages the collection of CS pins so we don't have to.

ameliamariecollins commented 1 year ago

I'm having trouble budgeting time for this project right now. Can you mark this feature as "Needs Help"?

Many thanks.