rp-rs / rp-hal

A Rust Embedded-HAL for the rp series microcontrollers
https://crates.io/crates/rp2040-hal
Apache License 2.0
1.4k stars 226 forks source link

Cycling a buffer with DMA #789

Open jsgf opened 5 months ago

jsgf commented 5 months ago

I'd like to set up a single buffer which is sent continuously to a pio fifo, and then be able to cancel it. It's not clear to me whether this is possible with the current DMA API. double_buffer seems the most plausible, but it doesn't seem possible because you can only call read_next on Transfer, not in Config.

Is double_buffer intended to support this kind of use-case? Otherwise would it make sense to have a dedicated cycle module (or something) to support this use?

jannic commented 5 months ago

I don't think this is currently supported.

It also looks like the documentation is not correct. https://docs.rs/rp2040-hal/latest/rp2040_hal/dma/index.html contains the sentence: "This API tries to provide three types of buffers: Single buffers, double-buffered transfers where the user can specify the next buffer while the previous is being transferred, and automatic continuous ring buffers consisting of two aligned buffers being read or written alternatingly." I think this is wrong and the "continuous ring buffer" mode currently is not implemented. Or at least it's not obvious how to use the API in that mode.

Due to the way the hardware is implemented, even if you only want to send the contents of a single buffer continuously, you need two DMA channels. It is not possible to chain a channel to itself. And if the continuous mode needs two DMA channels anyway, it might be reasonable to extend double_buffer to support this use case. However from a usability and discoverability point of view, it might be better to have a separate mode.

jsgf commented 5 months ago

As a side note, I was surprised that the API doesn't let you set up a chain before starting the transfer. The current API is awkward if you know there's a chain but you're essentially racing with the transfer in flight.

jsgf commented 5 months ago

Also it doesn't have a way to group channels so you can start them simultaneously.

jannic commented 5 months ago

As a side note, I was surprised that the API doesn't let you set up a chain before starting the transfer. The current API is awkward if you know there's a chain but you're essentially racing with the transfer in flight.

The current API makes sense for the designated use case: In a common double-buffer setup, you want to have full access to the second buffer from your Rust code while the first buffer is used by DMA. To avoid data races (and therefore undefined behavior), the API can't setup the chaining to the second buffer before the caller cedes its reference to that buffer.

Of course, this focus on a particular use case is also limiting. If your buffers are small and the DMA speed is fast, buffer underruns become possible, and the API provides no way to compensate.

But it is difficult to design an API that is both safe and flexible. And for full flexibility, it's still possible to configure the DMA registers using the PAC.

(That doesn't mean that there's no room for improvement! And tickets like this one are useful and important to learn about actual use cases that could be better supported. I'm just explaining the status quo.)

Also it doesn't have a way to group channels so you can start them simultaneously.

Can you provide an example where this would be important?

jsgf commented 5 months ago

Of course, this focus on a particular use case is also limiting. If your buffers are small and the DMA speed is fast, buffer underruns become possible, and the API provides no way to compensate.

Right. I'm thinking about things like scatter/gather cases where you've already set things up and you just want to DMA them. But you probably want to do that with a data/control pair of channels rather than dedicate one channel per buffer.

But it is difficult to design an API that is both safe and flexible. And for full flexibility, it's still possible to configure the DMA registers using the PAC.

Yeah I'm thinking I'll fall back to that to explore the design space and then see if a reasonable safe API makes sense.

Also it doesn't have a way to group channels so you can start them simultaneously. Can you provide an example where this would be important?

My current project is trying to drive 3 SPI DACs together in lockstep so that the samples are emitted together. The DACs themselves have a PIO SM each which are fully synchronous, but they're triggered by when the first word to output is available.

There's one DMA channel per PIO SM / DAC. Right now manually starting the DMA transfers, there's a noticeable skew across the output steams.

I'd like to start all the DMA transfers simultaneously to minimize the skew. I'm hoping that the SM FIFO buffering will be enough to cover any delayed samples due to things like DMA bus contention (as the DMA is way faster than the PIO clocks).