Avoid moving inside SpiDmaBus, abort dropped transfers

[bugadani]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• I have added necessary changes to user code to the Migration Guide.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

This PR simplifies SPI implementation by not relying on the move API internally. The PR also implements stopping transfers when their corresponding handle is dropped. This is probbaly better in line with embedded-hal expectations, and this change is necessary to enable zero-copy implementations of the embedded-hal traits (where applicable, of course. We still need to copy if the source data can't be accessed by the DMA).

Testing

Added and ran HIL tests, observed SPI waveforms using a logic analyzer on ESP32 and ESP32-C6.

[Dominaezzz]

Rip, I had a PR brewing around this stuff. (Maybe we should start coordinating work around this).

What's wrong with the moving? It shouldn't get in the way of implementing zerocopy transfers?
I only ask because I'll have to do some re-designing if it's the only way forward.

[bugadani]

What's wrong with the moving?

It's impossible to piece together the peripheral's state if the user drops the transaction, especially in async.

[Dominaezzz]

What's wrong with the moving?

It's impossible to piece together the peripheral's state if the user drops the transaction, especially in async.

Wasn't the State enum doing that? Or are thinking about some other state.

[Dominaezzz]

Btw how did you know setting the datalen to zero would get the SPI to stop? Trial and error? Or was this a hidden nugget of info in esp-idf?

[bugadani]

Btw how did you know setting the datalen to zero would get the SPI to stop?

Completely by accident. As far as I can see, esp-idf has no concept of aborting a queued (or other in-progress) transfer.

The questions around the async API will be discussed during today's Rust Embedded meeting: rust-embedded/wg#796 (comment)

[Dominaezzz]

What would it take to convince you to keep the moving in SpiDmaBus? 😄

[bugadani]

Just did a quick code size test and there is a 13K difference in my app (for ESP32-S3) between this branch and the branch's base. I'll happily take a 0.9% code size difference from just the SPI driver.

App/part. size:    1,502,816/2,031,616 bytes, 73.97% 4422ed3c
App/part. size:    1,489,536/2,031,616 bytes, 73.32% 1423f84

What would it take to convince you to keep the moving in SpiDmaBus?

I'm not opposed to creating short-lived buffer types that may end up getting moved (and I'm even taking a step in that direction in a later PR), but I'd rather not move the peripheral itself internally, and I think the current state demonstrates that at least that part isn't necessary.

[Dominaezzz]

and I think the current state demonstrates that at least that part isn't necessary.

It never was.
The motivation for moving stuff in the bus was to make sure there was only one SPI API, the one that moves stuff. (Idk if you've followed #1512 and the issues/PRs that spawn from it.)
Any other SPI niceties would be built on top of this move API, and in such a manner that users could copy it and do it themselves with any necessary customization to the wrapper. Stuff like queuing (like esp-idf does it) and double buffering.
Another nice thing from using the move API in SpiDmaBus, was that the SpiDmaBus didn't use unsafe and we could rely on the Rust compiler to check the invariants. HALs (this one and esp-idf-hal) have a tendency to sprinkle unsafe everywhere internally. imo the unsafe should be kept at the lowest level and everything else should be build on top of that without the use of unsafe, but I'm probably the only one who feels this way.

Having said all that, you can disregard that comment/request from me. Adding the new dma buffer view stuff to SPI has been a challenge (due to the full duplex nature of the peripheral), I probably won't be touching the SPI driver until all the other drivers use the dma buffers.

[MabezDev]

The motivation for moving stuff in the bus was to make sure there was only one SPI API, the one that moves stuff. (Idk if you've followed #1512 and the issues/PRs that spawn from it.)

This PR doesn't change that public API, but allows us some more flexibility in our impl, whilst also solving cancellation safety (something that I guess is going to be a problem using the move API in down stream crates, but I don't think we need to worry about that just yet).

Another nice thing from using the move API in SpiDmaBus, was that the SpiDmaBus didn't use unsafe and we could rely on the Rust compiler to check the invariants. HALs (this one and esp-idf-hal) have a tendency to sprinkle unsafe everywhere internally. imo the unsafe should be kept at the lowest level and everything else should be build on top of that without the use of unsafe, but I'm probably the only one who feels this way.

I do tend to agree with your principle in general, but I don't think we're using unsafe for the sake of it, we're doing it for a reason.

Tbh if AsyncDrop existed life would be a whole lot easier - but until then IMO this is a fine trade-off, plus the size reduction is a nice additional benefit.

[MabezDev]

Thanks!