rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
1.95k stars 197 forks source link

spi: add Operation::DelayUs(u32). #462

Closed Dirbaio closed 1 year ago

Dirbaio commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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

Changed:

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.

Dirbaio commented 1 year ago

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 commented 1 year ago

friendly ping @eldruin @ryankurte @therealprof

Dirbaio commented 1 year ago

bors r+