Async SPI #385
Async SPI #385
Conversation
MabezDev
requested review from
jessebraham,
bjoernQ,
SergioGasquez and
JurajSadel
|
Looks good and works fine Only thing is discovered: we broke the I2S sound example (circular DMA) by 5f933aa So technically this PR isn't the problem but I think the mentioned commit is the ground work for this Reverting that one line makes it work again, apparently but breaks this |
|
Maybe an obvious fix would be to change dw0.set_suc_eof(last);to dw0.set_suc_eof(circular || last);Interestingly the example also works with the original |
|
Thanks for the review!
Sounds good, but perhaps we should do that change in another PR, and maybe take a closer look and make sure that's the real fix? |
You are right it's not really related to this PR - I will create an issue for that (since I2S is already broken) |
| self: core::pin::Pin<&mut Self>, | ||
| cx: &mut core::task::Context<'_>, | ||
| ) -> Poll<Self::Output> { | ||
| TX::waker().register(cx.waker()); |
There was a problem hiding this comment.
@MabezDev Shouldn't the waker only be registered when the poll returns Pending? I know this is a common pattern in embassy but I wonder if it's necessary to execute code unconditionally.
At first I suspected this might be the reason for a bug I'm expeciencing, but that came from me not correctly understanding what's happening here.
There was a problem hiding this comment.
Perhaps? Did you try moving the waker registration? I can't really see it causing an issue, but if it fixes your issue its something to look into.
There was a problem hiding this comment.
No I'm working on another issue. I don't expect this to cause problems, just worse than ideal perf, so nothing terribly important.
This PR implements a
Futurebased abstraction around the current DMA implementations and uses said abstraction to implement the Async SPI traits from e-h onSpiDma.FYI the diff looks kinda weird because of the fmt commit, perhaps compare the commits before that fmt commit for easier viewing.
Breaking changes
dma::private,gdma::private, andpdma::privateno longer exist. All the DMA types/traits are now public. The ideal solution would be to erase these types see #381.Future work
Spistruct using the peripheral FIFO + interrupts, but this is less efficient and unnecessary for this initial implementation.