Introduce traits for the DMA buffer objects

[Dominaezzz]

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.
• 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

Introduces DmaTxBuffer and DmaRxBuffer. These are implemented by DmaTxBuf, DmaRxBuf and DmaTxRxBuf.
This also opens the door to other buffer types like ones that live in PSRAM, flash (ESP32-P4), IRAM, etc. with different alignments and burst modes/sizes. (I won't be implementing these enhancements anytime soon, I'm just opening the door)

Naming is hard, I'm happy to rename things to something else. 🙂

EDIT: This PR also allows DmaTxRxBuf to be used for TX-only or RX-only transfers. This will be convenient for bidirectional but half duplex peripherals.

Testing

Ran the SPI examples.
(I couldn't run the HIL tests myself, my probe-rs setup seems to be broken.... CI should check now though)

[MabezDev]

I'm wondering how useful this will be in reality. I understand the premise, but for example, the way spidmabus currently works it can only "hold" one dma buffer type for tx and rx (because it's encoded in the state enum).

My hope was to be able to optimize these copies away:

esp-hal/esp-hal/src/spi/master.rs

Line 1744 in 84a0600

tx_buf.fill(chunk);

when the read or write buffer is provided is suitable for DMA usage.

As it stands, if we added a DmaConstTxBuf type which could do that, we wouldn't be able to use it.

How do we go about solving that?

[Dominaezzz]

The usefulness of the trait doesn't show very well for SPI. I think it really shines for LCD_CAM, where you're more likely to get creative with framebuffers.
For example, in a project of mine I have a 800x600 RGB screen and fitting a framebuffer of 800x600x2 bytes in memory is not possible (can't use psram due to clock speed). However, as most of my images are solid, I can use a small 2048 byte buffer and send it over and over again using multiple descriptors pointing to the same buffer. This sort of thing doesn't belong in the hal and the traits allow me to implement it myself.
The SpiDmaBuf has no business with this dma buffer implementation of course.

About the zero copy thing. In the RFC, the plan was to always do the copy in the high level wrapper and anyone who wanted performance would just use the move based API.
Though I can understand the desire for optimisation, so an option to avoid the copy would be to split() the DmaTxBuf then combine the descriptors with the slice the user passed in? This may complicate the State enum.

There's also the option of adding more traits of course.
What did you envision the DmaConstTxBuf to look like? Just so we're on the same page.

[Dominaezzz]

As it stands, if we added a DmaConstTxBuf type which could do that, we wouldn't be able to use it.

How do we go about solving that?

Oh wait, after re-reading your comment I think I have a better understanding of what you're looking for.
I initially thought you were looking to make the SpiDmaBus be smart and choose to not do memcpy when the conditions were right per transfer.
I now understand you're looking to straight up create an SpiDmaBus instance that does not memcpy at all.

Having understood better, I have a decent solution. Another trait 😄.
This trait will be an SpiDmaBus specific one (support for other peripherals may come later if it makes sense).

trait SpiDmaTxThingy : DmaTxBuffer {
    fn fill(&mut self, data: &mut [u8]);
}

impl SpiDmaTxThingy for DmaTxBuf { /* do memcpy */ }
impl SpiDmaTxThingy for DmaConstTxBuf { /* no memcpy, connect buf to descriptors */ }

SpiDmaBus would then have another generic parameter (for this with a default of DmaTxBuf to make the type easier to name).

What do you think of that?

[Dominaezzz]

@MabezDev are there any objections to these traits themselves? The problem of optimizing those copies away is orthogonal to them.

I can look into optimizing away the copies but that will need either a separate trait or a dedicated struct for managing the optional memcpy (depending on what you're picturing).

[MabezDev]

@MabezDev are there any objections to these traits themselves? The problem of optimizing those copies away is orthogonal to them.

You're right I did get a bit side tracked with that, sorry 😅. It would be nice to have zero copy by default where possible though.

I don't have an objection perse, I think they're a good idea, my real concern is that these are encoded in the public SpiDma and SpiDmaTransfer types. Now, I know that's already the case, so I won't block the PR on that but IMO these traits should offer some "shared" format (the preparation struct is already close), where anything that impls the dma traits can be converted to a concrete struct which is then passed to the Dma APIs, meaning eventually we can hide the impl details completely (remove the generic params).

I'm kind of rambling a little bit 😅 but I hope that makes sense. Let me know if you need anything clarified.

TL;DR: This looks fine as is, after a rebase, but I hope we can get to some kind of concrete intermediate format to reduce the number of generics associated with this.

[Dominaezzz]

I don't have an objection perse, I think they're a good idea, my real concern is that these are encoded in the public SpiDma and SpiDmaTransfer types. Now, I know that's already the case, so I won't block the PR on that but IMO these traits should offer some "shared" format (the preparation struct is already close), where anything that impls the dma traits can be converted to a concrete struct which is then passed to the Dma APIs, meaning eventually we can hide the impl details completely (remove the generic params).

I'm kind of rambling a little bit 😅 but I hope that makes sense. Let me know if you need anything clarified.

Yeah it makes sense, less generics is better in general.

I'm planning on moving these to the dma module in the next few PRs, so the SPI driver won't have these buffers encoded in the driver types at least.
Erasing them completely would be an interesting task, one that becomes more feasible with an idea like #2035, I don't have a clear idea of what it would look like yet but I'd like less generics too.

[MabezDev]

LGTM, thanks!

[bjoernQ]

I guess at some point we need to align things between the DMA-capable peripherals - we now have WriteBuffer/ReadBuffer, DmaTxBuf/DmaRxBuf and DmaTxBuffer/DmaRxBuffer (and its implementations)