Add ManagedCS marker trait

[ryankurte]

Per #180 this PR introduces a ManagedCS marker trait to indicate that the CS line on an SPI peripheral is managed by the underlying driver. This is required to address unsoundness due to bus-sharing without explicit management of the CS pin per Rahix/shared-bus#8, and paired with #191 this will significantly improve the ergonomics of writing SPI-based drivers by no longer requiring every implementation to re-implement CS managed transactions and error handling (or to use a third party crate to do this).

I have been using this approach for some time in the embedded-spi crate, for the purposes of evaluation the intent is to refactor both embedded-spi and shared-bus to utilise this marker, as well as some dependent drivers (though this is for me difficult due to a cross dependency on #191).

Questions

• Adding this marker introduces a need for a wrapper type that consumes a SPI object and a pin to provide a ManagedCS interface, should this live here (in the embedded-hal) or in a third party crate?
• Initially I proposed this be called ChipSelect, another option is ManagedChipSelect, and other suggestions are welcome, however, please avoid adding noise to the discussion of this issue / implementation with minor naming concerns where possible.

Demonstration

• add expectations for blocking::spi::* trait use
• add ManagedCS impl to shared-bus
• alias embedded-spi::ManagedChipSelect to embedded_hal::blocking::spi::ManagedCs to update all embedded dependents
• implement basic SpiWithCS wrapper in whatever crate is appropriate (and update embedded-hal docs)
• update a couple of drivers to demonstrate the use of the marker traits

I am assigning this to the v1.0 milestone though it is technically non breaking as it does effect all SPI drivers, and is somewhat critical to support use of shared-bus with SPI peripherals (which is no longer possible due to unsoundness with the previous impl)

update: no longer part of 1.0 to avoid blocking, though hopefully to be landed shortly after this

cc. @Rahix

[rust-highfive]

r? @eldruin

(rust_highfive has picked a reviewer for you, use r? to override)

[eldruin]

A managed CS addition sounds good to me in principle.

• Whether ManagedChipSelect or ManagedCS I have no strong opinions. ManagedChipSelect is self-descriptive, which is a good thing. I think an argument would be looking at how long types get when using it.
• I would say SpiWithCS would be central enough to be included directly here.
• I would say the wrapper in embedded-spi has too many features to be included in e-h, though. e.g. reset, busy, ready pins and probably delay.
• I understand the motivation to release this in 1.0 but I would rather not block that with anything else and just get it out the door as soon as possible. Shortly after that we can do a 1.1 release with this PR, transactional SPI/I2C, I2S or whatever is ready.
• How would you address device timing requirements between setting the CS pin low and starting SPI communication? This might be a good time to think about this.

I can properly review once there is more in this PR.

[therealprof]

@eldruin Thanks for your write-up, so I can save my time saying pretty much the same thing. 😅

[ryankurte]

agreed on pretty much all points, to be clear i only meant the CS part from embedded-spi.

I understand the motivation to release this in 1.0 but I would rather not block that with anything else and just get it out the door as soon as possible. Shortly after that we can do a 1.1 release with this PR, transactional SPI/I2C, I2S or whatever is ready.

my concern with this is that since shared-bus no longer contains an SPI proxy, bumping the embedded-hal and related shared-bus impl in use is going to break any proxied SPI with no useful recourse, which is not going to be a great experience on top of the breaking HAL API changes. maybe re-adding the SPI proxy with some big soundness warnings would be viable to mitigate this? (cc. @Rahix)

How would you address device timing requirements between setting the CS pin low and starting SPI communication? This might be a good time to think about this.

good call. in my experience (where the driver manages CS) this is usually this is implemented by the underlying driver, in this case SpiWithCS will be a lightweight wrapper with no specific timing consideration, if manual timing is required it is reasonably trivial to implement a third party SpiTimedWithCS or equivalent, and where the drivers handle this it's configuration-time which i think is outside the scope for the hal.

[eldruin]

Great work, thank you!

• Another alternative would be skipping e-h 1.0 support for shared-bus and updating only on 1.1 (which would give us more pressure to release 1.1 :), although I understand it does not look good.
• Some more bikeshedding: It seems ManagedCs will not need to be written often, so I think ManagedChipSelect would be preferable.

Another (albeit far-fetched) argument for delaying stuff to 1.1 would be that we could yank version 1.1 if something is terribly wrong with it but yanking 1.0 would be more problematic.

[Rahix]

my concern with this is that since shared-bus no longer contains an SPI proxy, bumping the embedded-hal and related shared-bus impl in use is going to break any proxied SPI with no useful recourse, which is not going to be a great experience on top of the breaking HAL API changes. maybe re-adding the SPI proxy with some big soundness warnings would be viable to mitigate this? (cc. @Rahix)

@ryankurte: Adding the unsound SPI proxy is definitely off the table. We've seen it bite people before so it's safe to assume that it would happen again. We need a different solution here ...

For the time being, we have the single-threaded SPI proxy which should be plenty for many use-cases already. For anything more complex, I'm unsure ... I think the work from @ryan-summers for shared-bus RTIC support could be a viable workaround for the time being. Essentially, it ensures safety by stuffing all bus-users into a struct and wrapping that struct in a mutex:

struct SpiDevices {
    pub device_a: DeviceA<SpiProxy<...>>,
    pub device_b: DeviceB<SpiProxy<...>>,
}

static SPI_DEVICES: Mutex<Option<SpiDevices>> = Mutex::new(None);

fn task_a() -> Result<..., ...> {
    ...

    SPI_DEVICES.lock(|s| {
        let device_a = &mut s.ok_or(Error)?.device_a;

        device_a.do_things();
    })?;
}

fn task_b() -> Result<..., ...> {
    ...

    SPI_DEVICES.lock(|s| {
        let device_b = &mut s.ok_or(Error)?.device_b;

        device_b.do_other_things();
    })?;
}

---

But of course, ManagedChipSelect or later a full transactional SPI interface would be the cleaner solution.

[therealprof]

We discussed this in todays meeting due to the nominated flag but it's not quite clear what needs doing here. Seems like a good idea to have this to me. Can anyone summarize what we would need to discuss on the next meeting? Thanks.

[ryankurte]

updated this PR with documentation around expectations and added an implementation to Rahix/shared-bus#21, just need to update a driver to take advantage of this.

the updated expectation comes down to, if you expect operations to occur in a known order you need to use Transactional, and if you expect CS assertions to be exclusive you need to use ManagedCs.

in practice a bunch of drivers don't do this, primarily because Transactional didn't used to exist, but, it's pretty trivial to update (and i think the only way we get to safe shared SPI access).

last thing on the list is to update a driver / example to talk to a couple of SPI devices but, the behaviour is not something the compiler can check, so, this probably should be run but still doesn't serve as a demonstration of the negative...

[eldruin]

Great. I added some comments but it is looking good. Thanks!

[eldruin]

Didn't we come up with a better word to replace "ordering"? "grouping" or so, perhaps?
This still sounds to me like data from two operations could somehow be sent in the wrong order.
When reading the next sentence in the docs the user may understand that this is about grouping but still get a scare.

(for completeness: same below)

[eldruin]

How would you address device timing requirements between setting the CS pin low and starting SPI communication? This might be a good time to think about this.

good call. in my experience (where the driver manages CS) this is usually this is implemented by the underlying driver, in this case SpiWithCS will be a lightweight wrapper with no specific timing consideration, if manual timing is required it is reasonably trivial to implement a third party SpiTimedWithCS or equivalent, and where the drivers handle this it's configuration-time which i think is outside the scope for the hal.

@therealprof: I had forgotten about it but I already asked about timing considerations here.
Do you have any further concerns with leaving this to the implementation?

I guess a crucial point would be that HALs provide both managed and non-managed CS SPI so that something like SpiTimedWithCs, or something more complex like cycling CS between operations if necessary can be implemented.

[ryankurte]

I guess a crucial point would be that HALs provide both managed and non-managed CS SPI so that something like SpiTimedWithCs, or something more complex like cycling CS between operations if necessary can be implemented.

i think like most of the HAL this addresses the core issue but, cannot solve everything. if you have to configure timing between transactions or after CS it's going to require either device configuration or wrapping and, as you suggest, you can always implement this either in hardware or as another layer.

given the generic SpiWithCS implementation i would only expect HALs to implement ManagedCS where the hardware (or OS) actually does manage the chip select?

[almindor]

How would you address device timing requirements between setting the CS pin low and starting SPI communication? This might be a good time to think about this.

good call. in my experience (where the driver manages CS) this is usually this is implemented by the underlying driver, in this case SpiWithCS will be a lightweight wrapper with no specific timing consideration, if manual timing is required it is reasonably trivial to implement a third party SpiTimedWithCS or equivalent, and where the drivers handle this it's configuration-time which i think is outside the scope for the hal.

@therealprof: I had forgotten about it but I already asked about timing considerations here. Do you have any further concerns with leaving this to the implementation?

I guess a crucial point would be that HALs provide both managed and non-managed CS SPI so that something like SpiTimedWithCs, or something more complex like cycling CS between operations if necessary can be implemented.

These things are usually very specific when using HW. See for example how it's done in the e310x-hal

We decided to expose this so users can configure the SPI in all possible ways. I don't think timings and delays should be considered in the trait abstraction level.

[ryankurte]

tried to update this against master, i am not sure why all those other commits have been pulled in :-/

[ryankurte]

closing in favour of #351