Document implicit assumption in non-blocking spi trait #120
Conversation
david-sawatzke
requested review from
ilya-epifanov,
thejpster and
a team
as code owners
david-sawatzke
changed the title
|
Removed the change for the default implementation, otherwise releasing cs after doing a blocking write will lead to unintended consequences |
|
Any progress here? |
|
I've thought about this a bit more: If any hal (with hardware fifos) wants an efficient implementation with these restraints, they'd have to count the current spi transactions. This would also make utilizing the receive fifo pretty much impossible. This slows bidirectional communication (which, in my experience, is pretty uncommon) down, especially when considering techniques like dmas or platforms like linux. In contrast leaving it like it's currently often implemented (just giving an error if the receive fifo overflows and otherwise returning the oldest byte) makes it uncertain when the fifo overflows, forcing chip independent implementations to read after every write (and worse, if the peripheral is somehow shared, making off-by-one errors possible). But it enables chip-specific implementations to utilize the receive fifo to it's full potential. I'm not happy with both solutions, but the proposed one sounds a lot better to me. Open to alternatives As-is, this PR is done, not needing any changes, but some discussion could improve it. (I think) This is also a breaking change, since existing implementations didn't have to consider any restraints. |
Disasm
added
S-waiting-on-author
|
It seems we need more discussion here. Will you be able to participate in our weekly meeting? Next will be on Feb 26. |
|
As this hasn't been resolved yet, I'd like to try & get this (or some alternative) into v1.0. |
|
Any progress here? |
|
This is basically ready as-is. If we want to do the simpler, but way less useful alternative of unspecified behaviour for unpaired read & send calls, that would also work. I don't care much either way, I just want there to be some thought given to it & documented instead of "the most convenient interpretation". |
Co-authored-by: Vadim Kaushan <[email protected]>
|
👎 Rejected by code reviews |
|
bors r+ |
|
👎 Rejected by code reviews |
|
Maybe the unresolved review? I've resolved it, might work now. |
|
bors r+ |
|
👎 Rejected by code reviews |
|
Nope, bors is picky about that. @Disasm will need to explicitly approve to get rid of the concern. |
|
bors r=therealprof |
|
🔒 Permission denied Existing reviewers: click here to make Disasm a reviewer |
|
bors r+ |
It should only be able to read the data returned by the last write call. Otherwise the overflow handling isn't really possible when doing only write calls (at least on stm32f0). @therealprof and me came to this conclusion after chatting on irc that this is the intended usage of this api.