Spi flushing definition improvements

[pellico]

Issue

In the present definition is of flushing of Spi is required that the transmit FIFO is empty and no activity in the bus.

This definition doesn't require explicitly that also the receive FIFO shall be empty.
This create an issue when using pipelined write operation.

It is already happened that a fully complaint implementation of SpiBus::flush operation caused a shift on read operation after write operation because the read FIFO is not empty.
see atsamd-rs/atsamd#1007 for an example

Proposal

Make explicit that SpiBus flush requires empty read FIFO too.

[Dirbaio]

I don't think this makes sense. The FIFOs are an implementation detail, the SPI traits API doesn't have any methods for manipulating them directly, so it doesn't make sense to refer to them in the contract for flush(). (yes, the tx fifo mention doesn't make sense either, we should remove it).

The issue you link is a bug in the implementation of atsamd-hal, it's not due to the flush contract.

[pellico]

@Dirbaio

The FIFOs are an implementation detail, the SPI traits API doesn't have any methods for manipulating them directly, so it doesn't make sense to refer to them in the contract for flush(). (yes, the tx fifo mention doesn't make sense either, we should remove it).

I am fine to alternative formulation of the definition that make clearer the semantic of flush. I am just wonder how it possible by mentioning only entities that can be manipulated by API.

The issue you link is a bug in the implementation of atsamd-hal, it's not due to the flush contract.

Could you elaborate on this ?

[Dirbaio]

The SPI contract is

• write causes N words to be clocked, where the data on MOSI is user-supplied and MISO is discarded.
• read causes N words to be clocked, where the data on MOSI is dummy bytes and MISO is returned.
• transfer causes N words to be clocked, where the data on MOSI is user-supplied and MISO is returned.
• flush waits until the bus is idle.

That's it. Note there's no mention of any FIFOs.

Pipelining is allowed, meaning:

• write, read, transfer are allowed to return before the operation is done.
• The end result on the bus must be "as if" no pipelining is done.

So for example if you do:

spi.write(&[0xAA]);
spi.read(&mut buf[0..1]);

This should cause 16 bits to be clocked on the bus.

• First 8 bits are 0xAA on MOSI, and MISO ignored.
• Last 8 bits are 0x00 on MOSI, and MISO written to the buf.

And note this is correct usage, the user does NOT need a .flush() in between the write and read.

So if you do a naive implementation like

• write: write words to the TX FIFO
• read: write dummy words to the TX FIFO, read words from the RX FIFO
• flush: wait until bus idle

it doesn't fulflil the contact because read will return stale data that write left in the RX FIFO.

The solution is NOT to require the user to put a flush() between the operations.
The solution is NOT to require flush() to clear the RX FIFO in the embedded-hal trait contract.
The solution is to fix the implementation. For example you can make read do "wait until bus idle, clear RX FIFO, write dummy words to the TX FIFO, read words from the RX FIFO"

[pellico]

@Dirbaio
Thank you for your detailed explanation. Your description is looks me clearer than SPI module description in embedded_hal

What you described was the behavior of SPI driver in atsamd before the modification introduced to support the pipeline the flushing as described in embedded_hal see this commit
Therefore the maintainer of atsamd misunderstood the description in embedded_hal

Will you create a pull request to improve the description of SPI module in embedde_hal ?