SPI Move API is difficult to use

[MabezDev]

I know we literally just merged it, but I've just spent the afternoon porting (or I guess I should say, failing to port) my basic firmware for my keyboard. It's incredibly difficult to use, and requires wrapping transfers and the Spi device itself in Option across the entire API surface (unless I'm missing something) and swapping between.

I like everything else about the PR, except for the move changes, and I think we should revert to allowing non-static buffers etc which we decided on in #1245. I believe it should still be possible to allow for queuing etc without the moves, and the only thing we 'miss out' on is mem::forget safety, which we already document not to do within the hal.

I'm happy to work on this, but I wanted to discuss before I do, just in case I'm missing something obvious.

cc: @Dominaezzz @bjoernQ

[Dominaezzz]

Were you not able to use the easy wrappers? Or were you just trying out the move API?

I believe it should still be possible to allow for queuing etc without the moves, and the only thing we 'miss out' on is mem::forget safety, which we already document not to do within the hal.

The safety was a secondary goal. The main goal was to be able to store the transfer object in a static variable, to allow interrupts to progress the transfer.

After #1976 lands I can add a 'static free implementation, though it's c'tor will probably be unsafe. Sprinkling lifetimes everywhere without the traits might become a PITA.

[MabezDev]

Were you not able to use the easy wrappers? Or were you just trying out the move API?

Trying out the move API, because I do actually want the perf

The main goal was to be able to store the transfer object in a static variable, to allow interrupts to progress the transfer.

I'm wondering if this is ever really necessary, wouldn't it be better to just signal from the interrupt via an atomic variable or signal if async that the transfer is done? The transfer can then 'stay local' but with the new added benefit that we can prepare the next transfer thanks to your recent changes to use a DmaTxBuf instead of just a slice? I haven't tried this, but I will play with it locally and see if I can get something to work. (FYI the signalling approach is what I plan on using in my keyboard: https://github.com/MabezDev/mkey/blob/e9ea41c31231d2101b1c52aeb7b5a8e1345c7aa4/firmware/src/main.rs#L212-L227)

[MabezDev]

Hmm I guess predictably, this ends in lifetime issues. The move API is required.

This just feels like something that should be easy to do, prepare another buffer whilst one is in flight 🤔.

[MabezDev]

I suppose I need to wrap SpiDma in its own struct to do the double buffering, just like you did for the embedded-hal wrappers (maybe that was in your plan and I forgot 😅). I'll try and do this for my keyboard and see if its generic enough to be added to the HAL.

[Dominaezzz]

I'm wondering if this is ever really necessary, wouldn't it be better to just signal from the interrupt via an atomic variable or signal if async that the transfer is done?

In my case it is. The scheduler just takes too long to poll the future again, leaving the spi underutilized.
What I want is a mix of both (this is what idf does), the interrupt should consume the previous transfer, store the results somewhere, start the next transfer and signal the application that a transfer has finished.
The application can, eventually, in its own time, collect the stored results of the finished transfer. During this time the next transfer is already happening. Minimizing latency.

I admit it's somewhat extreme but it's a valid use case I think.

[Dominaezzz]

(maybe that was in your plan and I forgot 😅)

It was 😂.

My current predicament is deciding whether to make the double buffering optional or not.

I suppose I need to wrap SpiDma in its own struct

I think this is probably the answer to all the ease of use problems, more wrappers haha.

[MabezDev]

I'm satisfied that the common path is easy to achieve, and improvements to manual, advanced path are on the way. Closing.