spi: add Operation::DelayUs(u32).

[Dirbaio]

depends on #461

There are SPI devices out there that need delays between transfers within a transaction. Not dummy bytes, SCK must be idle during the delay. And it must be a single transaction, CS must not be deasserted.

This is not doable with the new operation-list API (it was doable with the previous closure API).

Examples:

• ADS1255/56 needs a 7 micro second delay between the command to read a register and reading the corresponding data over SPI. datasheet page 34. matrix discussion.
• CAM312 Central Tracking Unit datasheet, Section 9.3. matrix discussion

To support these, this adds a new Operation::DelayUs(u32).

Linux spidev can implement this fine, it supports delaying after each transfer.

The downside is that SpiDevice impls now need a time source. Discussed a bit in yesterday's WG meeting.

I don't think separating the traits by capability is worth it, we end up with the same problem as with the read-only/write-only traits (#461)

[ryankurte]

hmm, is this a similar problem to the need to be able to do weird acknowledgement things within a transaction (eh. send command, wait on pin assertion, fetch data)? iirc for those cases we'd recommend using the bus instead of the device abstraction (which works for most cases with the usual caveats related to manual CS with multiple apps sharing buses under linux)

cc. @eldruin because we had a big chat about this recently

[jannic]

Could there be SpiDevice implementations that can't support the delay operation? Perhaps there should be some documented best practice on how to react in that situation.

For example:

• drivers should avoid the delay operation if possible, to improve compatibility
• SpiDevice implementations should support the delay operation if possible, for the same reason
• in case an SpiDevice can't support the delay operation, it should reject a transaction containing such an operation. (It could return an error with a newly defined ErrorKind OperationNotSupported?) It should return that error immediately, even if Operation::DelayUs is not the first operation

With an OperationNonSupported error kind, one could even make Operation non_exhaustive to allow for later extensions. Who knows, perhaps there are devices which need varying SPI clocks within a single transaction?

[Dirbaio]

Discussed in today's WG meeting (chatlog, minutes)

Changed:

• Added new_no_delay() convenience method to embedded-hal-bus impls. Uses a dummy NoDelay that panics on use.
• The delay is no longer within the RefCell/Mutex. Otherwise the user would have had to pass RefCell<(BUS, NoDelay)> which is awkward. This requires a Delay instance for each device, so it assumes the HAL allows obtaining as many delays as you want (or clone them), which should be the case in general (it's something we want to encourage, at least).

I haven't added new_no_delay() to embedded-hal-async impl because I didn't want to copypaste NoDelay , and the ExclusiveDevice there should be moved to embedded-hal-bus anyway. Will do in a follow-up PR.

[eldruin]

I think this might make embedded-hal-bus unusable for linux-embedded-hal (or any implementation which provides meaningful buffering of the SPI transfers).

While this would work for SpiDevice implementations provided by l-e-h, I think it would not work for its SpiBus implementation, since delays there are appropriately marked transfers to the kernel and not something handled externally without SpiBus knowing about it.

This means that when using e-h-b on a l-e-h::SpiBus, we are delaying in "program-defined time", not in "bus-transfer-defined time".
Depending on the internal queue fill-up status of the SPI handler in the kernel, we might delay for a shorter time than requested to, down to no delay at all.

However, I do not know how the linux kernel buffers SPI transfers, and if there is a queue at all, so somebody with knowledge of the matter should clarify.
Nevertheless, the same issue applies to any HAL implementation which provides some kind of buffering due to the fundamental time difference.

[Dirbaio]

Good point about buffering/queueing. I've added bus.flush() calls before the delays. This ensures the queued operations are fully finished before delaying, so the delay now becomes bus time, not just program time.

(Note that supporting flush() was already mandatory for SpiBus, and is also needed for correct software CS. So we're not ruling out any impls by doing this. Also I'm pretty sure linux spidev doesn't buffer transfers, or if it does then there must be a way to flush it for sure)

[Dirbaio]

friendly ping @eldruin @ryankurte @therealprof

[eldruin]

Alright, I checked a bit over at l-e-h and this seems fine although we would not be able to use the kernel API for the delay when using e-h-b, but for that we would need to make the e-h::SpiBus know about delays and that would further the impact of this special case, without a clear benefit. When absolutely needed, it would be possible to define a different implementation for e-h-b. Let's get it merged.
Thank you!

bors r+

[Dirbaio]

bors r+