SPI implementation doesn't follow embedded-hal requirements for CS

[adamgreig]

As far as I can tell*, the current implementation for SPIBus and SPIDevice via spidev will assert CS around any of the SPIBus methods like read() and write(), because that's what spidev does for each transfer by default.

However, the SPIDevice::transaction() method expects CS to be asserted once at the start, remain asserted for the duration of the closure, then become deasserted afterwards. Since the closure just calls those methods on SPIBus, CS is toggled for every transfer inside the transaction, which will break drivers.

For example, if a driver has some function that's given a buffer of data to send, and needs to prepend a command byte:

fn write_data(&mut self, data: &[u8]) -> Result<(), Self::Error> {
    self.spi.transaction(|bus| {
        bus.write(&[0])?;
        bus.write(data)
    });
}

It wants to have CS assert, a 0 byte written, the data written, and then CS de-asserted, but instead CS is asserted/deasserted around both the one-byte write of 0 and the multi-byte write for data.

Ideally, the SPIBus methods would not touch CS at all, and only transaction() would somehow assert CS at the start and deassert it at the end. I'm not sure if the Linux spidev interface actually allows for such a thing though, and even if it did, the way it grants you a "bus" that's actually shared with other devices means we sort of break the HAL trait assumptions anyway.

I think using spidev's transfer_multiple() alongside the cs_change setting it might be possible to do the right thing here, though I'm not completely sure how... if transfers were queued up only to be executed at the end of the transaction, using flush() inside the transaction wouldn't work, or would still cause excess CS state changes. Still, it might be better than the current situation?

* Right now I can only test this on a remote device and infer what's not working by how a chip isn't responding, so I might be totally wrong here...

[ryankurte]

huh, yeah bus sharing with linux drivers / other applications is certainly an interesting problem, for now i think the recommendation is probably don't do this :-/

for unshared use i would just set SpiModeFlags::SPI_NO_CS drive CS manually (as is required for any more complex transactions anyway).

-edit-: it looks like spidev has a locking api which we chould implement here / should do the job?
-edit edit-: huh, maybe there's no equivalent exposed to userspace? :-(

[adamgreig]

How do you drive CS manually if that flag is set? Even if we don't worry about sharing with other applications, just having the SpiBus methods not change CS and instead driving it inside SpiDevice::transaction would be a big improvement.

[ryankurte]

How do you drive CS manually if that flag is set?

ahh, i have always just used a separate GPIO (or patched the device tree to disable CS1, never succeeded with CS0) and driven it as a normal digital pin. it does seem like there should be an API for CS assert/deassert tho.

[adamgreig]

We've been discussing this on and off at the weekly meetings, currently the hope is that we can get away without changing the SPI traits but probably documenting where edge cases and constraints exist. For example, in embedded-hal we may document that some implementations will require a transaction contains only 0+ writes followed by 0-1 reads, and that anything else (e.g. write-read-write) will error, and therefore suggest drivers aiming for maximum compatibility follow that suggestion.

On the Linux side the hope is we can do something like...

1. Have a new struct that implements SpiBus + SpiBusRead + SpiBusWrite + SpiBusFlush against a single spidev device, but configures the device to not drive CS at all, and users must use standard GPIO for CS along with external crates to provide SpiDevice from our SpiBus and GPIO OutputPins.
   • In effect we use a single spidev device to access the whole bus, broadly hope that no one else will simultaneously use another spidev device on the same bus (we can at least document that this may break some use cases), and suggest users configure spidev to use an unused/unconnected GPIO for its CS pin (since it can't be re-used as GPIO while reserved by spidev).
   • This gives users a "standard" SpiBus that can be used with multiple software-controlled CS pins, or as a dedicated bus for hacks like driving smart LEDs
   • No restrictions on what you can do inside transaction() closures, all patterns work fine
   • This explicitly doesn't expect to be used for a single device, even though it's using a single spidev device under the hood
2. We also have a struct that implements SpiDevice against a single spidev device, leaving spidev to configure CS. Its transaction() method provides a different SpiBus device, not the one from (1) above, which has a different behaviour:
   • Lets Linux drive CS, so supports normal sharing of a bus between devices, device tree integration, etc
   • Buffers all calls to write() and flushes them at the end of the transaction or whenever read() is called
   • This means all transactions have to be 0+ writes followed by 0 or 1 reads and then 0+ flushes, and can't do other synchronisation or blocking inside the transaction (implied by not being able to flush until the end).
   • We should be able to detect violations of this and return errors rather than failing silently
   • Under the hood, we use spidev's transfer_multiple to send all the writes and any final read in one go, ensuring an atomic transaction bounded by a single CS assertion.
   • This use case is more like using spidev from other languages, uses the kernel's own sharing abstractions, integrates with existing device tree CS pins, but only supports certain transaction patterns (and requires buffering all the data before sending it, but hopefully that's not a big impact on systems that are already running Linux).
   • It doesn't completely comply with the embedded-hal rules, but it is convenient and probably mostly works; for cases where it doesn't, it should be possible to use (1) above but may require changing CS pins or altering the device tree.

I don't have much experience with embedded Linux, does this sound at all sensible?

[eldruin]

Let's keep this open until we actually fix this issue here.

[RaulTrombin]

Had an issue moving to new raspbian kernel.
Previously on raspbian with 5.1 kernel I was able to control the spi through cs on pins 16,17

/boot/config.txt:
dtoverlay=spi1-3cs

on my rust lib i'm using something like:

        //Define CS2 pin ICM-20602
        let cs_2 = Pin::new(16);
        cs_2.export().expect("Error: Error during CS2 export");
        cs_2.set_direction(Direction::High)
            .expect("Error: Setting CS2 pin as output");
            
        let imu = imu_Builder::new_spi(spi, cs_2);

on new 6.1 kernel, it doesn't work.
the CS1 and CS2 keeps busy and can't be used as Pins.

dtoverlay=spi1-1cs fix it for use CS1 and CS2, but i think there is nothing to do with CS0.

[ryankurte]

@adamgreig this approach seems pretty reasonable to me... on linux we build the transaction buffers anyway so checking they work is not big, just need to make sure we guide folks to the right approach for their application.

IMO it's an oversight that there's not an ioctl to assert HW CS / grab process-exclusive access to the device so this kind of unsoundness can be mitigated, but, trying to implement that upstream in spidev is pretty low on my list...

Previously on raspbian with 5.1 kernel I was able to control the spi through cs on pins 16,17

@RaulTrombin if you want to use software CS you have to disconnect these from the kernel. as you've discovered dtoverlay=spi1-1cs free's up CS 1 & 2 but as you note there's no spi1-0cs option.

i've spent some time in the past trying to generate an overlay without the CS0 pin allocated without success, either because i'm missing something or there's a fundamental requirement to have a CS pin attached. for the moment it is easiest to just, use a different pin.

[eldruin]

We have switched to an operation-slice interface instead of a closure since embedded-hal=1.0.0-alpha.10. So this is not an issue anymore.

[adamgreig]

What do you think of still going ahead with the plan to split spi into two structs, one that just does SpiBus and one that just does SpiDevice, as described above?

I think it would still be useful, as a documentation aid if nothing else, as otherwise it's not really clear why you can use the same struct with both traits ATM. Should I make a separate issue?

[eldruin]

Sorry I missed that. Yes, would you mind copying that part and creating a dedicated issue?