`embedded_hal::spi::Operation::DelayUs` is not granular enough

[korken89]

Hi,

When porting drivers to use the new HAL traits we've found an issue that was "implementation specific" before, but now is part of the traits, and that is how to insert delays due to chip requirements.

Usually this means for example (these come from the DW1000 chip):

• CS must be low for 50 ns before a transaction starts
• CS must stay low for 200 ns after a transaction finishes
• CS must stay high for 250 ns between two transactions

With the current API there is no way to represent these kinds of delays in the Operation API as it is microsecond based.
As it is now, either we need to pay up to 20x more time than required by selecting a 1 us delay, or more as it is in practice due to implementation details of timers, one will get between 1 and 2 us of actual delay.
So with the requirements stacked, one get 3-6 us of delay (!) instead of 500 ns in total.

I propose an Operation::DelayNs is added to cover this use-case.

For the requirement between two transactions, I think this might be out of scope of this API?
But if there are any ideas here I'd love to hear them.
I would argue that if it cannot be added to this trait then it's not possible to achieve generic drivers over chips with CS requirements. This because you must always sidestep this trait then to actually implement support for a chip in a robust manner.
Generally though chips "work" even when these times are not upheld, but I do not think we should build the SPI traits on hope that it will work when requirements cannot be specified.

If I was to shoot from my hip I'd say that the trait could have a constant that is

const TIME_BETWEEN_TRANSACTIONS_NS: u32 = 0; // 0 for the current behavior

and that would use the same nanosecond API required above.

Looking forward to your feedback!

BR Emil

[adamgreig]

We also don't have any way to specify SPI clock frequency, or clock polarity/phase, which are just as (or more) important than CS timing for getting a device working - I think they're all considered "configuration" and outside the scope. The delays in Operation were not intended for CS-to-clock, but for chips that for various reasons require some long delay between parts of the same transaction.

I think whatever is providing the SpiDevice is a much better place for this - since it will be actually driving the CS pin, perhaps it can be created with a CS-to-clock and data-to-CS delay, or perhaps one is given alongside a bus configuration for each SpiDevice, and it's applied whenever that device uses the bus. Putting DelayNs into Operation means every implementation of SpiDevice must be able to handle nanosecond-resolution delays, which for example isn't an option on Linux, or has to just round them up to 1us anyway.

[korken89]

For my understanding, are you proposing one would (for each driver) create a ManagedCs pin which would bundle timing requirements with an actual pin?

Pseudo-code for the DW1000 example in my original post:

pub struct ManagedPin<TIM: SomeTimerApiWithNsResolution, CS: OutputPin> {
    tim: TIM,
    cs: CS
}

impl<TIM: SomeTimerApiWithNsResolution, CS: OutputPin> OutputPin for ManagedPin<TIM, CS> {
    type Error = <CS as OutputPin>::Error;

    fn set_low(&mut self) -> Result<(), Self::Error> {
        self.tim.delay_ns(250); // One could check the time to a previous `set_high` and maybe optimize this away
        self.cs.set_low()?;
        self.tim.delay_ns(50);
    }

    fn set_high(&mut self) -> Result<(), Self::Error> {
        self.tim.delay_ns(200);
        self.cs.set_high()?;
    }
}

[adamgreig]

I don't love it as an API, but yea, that's basically what I'm proposing. I don't see it as something provided by the driver but rather as something provided by the HAL though, and the user just has to configure it with whatever delay is required the same as they ahve to configure the bus to have the right clock freq, polarity etc. Maybe instead of the CS pin, it could be done by whatever splits the bus into devices, but since ideally that's non-hal-specific, it probably shouldn't be there. Maybe a HAL could provide helpers for this, but since the CS pin doesn't have access to the bus object, it probably can't change the bus config between peripherals easily.

Dunno. I don't think we can reasonably add DelayNs to the operations, even though it seems like an easy way to solve this particular issue, because it's simply not supported on Linux. Not the first time that's constrained us somewhat! We could work to include SPI bus configuration in the shared embedded-hal traits eventually - clock speed, phase, polarity, and perhaps information about CS to clock timing, but I don't know what that would look like or how the drivers would signal what they need to the user/HAL.

I think for most drivers they basically ignore this and get away with it, because there's often a bit of a delay between setting the CS GPIO and starting an SPI transfer, but it's hard to rely on that.

[korken89]

Alright, then the intent of embedded-hal is clear to me. Thanks for clarifying!

I think for most drivers they basically ignore this and get away with it, because there's often a bit of a delay between setting the CS GPIO and starting an SPI transfer, but it's hard to rely on that.

We are here specifically that each new version of the stable compiler randomly broke our radio driver due to it optimizing the CS handling differently. 😅

[adamgreig]

We had a big discussion around this idea in the meeting yesterday, but I'm not sure we got very far about any actual changes. We could add DelayNs (or just Delay(Duration)) and have linux round it up to the nearest µs, but obviously that would hit performance pretty bad.. and probably makes the e-h-bus impls quite tricky too as e-h itself only has DelayUs/DelayMs at the moment.

It seems like we might well need more bus configuration anyway to enable more sharing use-cases, as right now there's no way to change the bus settings (clock, phase, polarity, [CS pin timing]) as you swap between devices. We could add this as an extension trait later though, SpiBusWithConfig or something, which a more sophisticated version of the e-h-bus sharing providers could use, and perhaps that could introduce more fine-grained CS delay.

We are here specifically that each new version of the stable compiler randomly broke our radio driver due to it optimizing the CS handling differently. 😅

Ah, this really sucks. I think for SPI devices that are secretly whole microcontrollers running firmware, like radio modules, there's often much longer CS timing requirements as the firmware maybe has to trigger an interrupt or something rather than just be a shift register. In the worst case, today, you could add the 1µs delay, but it's really not great. The custom CS pin that does a delay is pretty annoying too but maybe it's the most ergonomic option, at least it can wrap an e-h digital output.

[adamgreig]

@korken89 as an update, we discussed this point in the recent weekly meeting and ended up changing Operation::DelayUs to Operation::DelayNs, so it's now possible to specify ns-precision delays inside an SPI transaction. On Linux and other platforms these will be rounded up to the smallest possible delay (e.g. 1µs), so the intent is still that they should not be used for CS-to-data delays in drivers... but for the time being, a driver could use it to ensure correctness, even at the cost of worse throughput on some platforms.

Eventually, the goal would instead be for the embedded-hal-bus SpiDevice implementations to support ns-level delays when swapping to specific devices, alongside perhaps reconfiguring the bus mode/clock. At that point, drivers could use that interface instead as a "more sensible" way to specify the delay which can potentially be implemented with finer resolution.

Anyway, I hope this at least solves your immediate problem.