embassy-rp::dma::Channel: Allow use of custom irq handler

[dobrowolski-lukasz]

Follow up to #5338.
Make it possible to use Channel with custom irq handler.

This change makes some type annotations necessary. Do let me know if there is a better way.

Misc: Make Channel number method pub to help with custom DMA handling code.

[Dirbaio]

the way to do this I had in mind is using the same pattern as the Blocking/Async modes in other drivers.

fn new(ch, irq) -> Channel<'a, Async>;
fn new_blocking(ch) -> Channel<'a, Blocking>;

and then make Transfer only awaitable in Async mode, since it'll hang if the irq handler is not bound.

[dobrowolski-lukasz]

I feel we are talking about two different things. Can You elaborate?

I'll explain my use case. It should make things clearer.
I'm using DMA to feed I2S interface implemented with PIO. In current form I observe DMA underflows. I use two buffers that I switch. One is given to DMA to feed I2S and in the meantime the other is filled with data by another part of the application(which unfortunately has blocking parts that are hard to get rid of).
I believe the underflows are due to the time between DMA being done and next transfer being started.

The solution I'm trying to implement is to start next transfer in IRQ handler.
I had to reimplement Transfer type as it's tied to embassy-rp::dma::InterruptHandler, so I don't think there is any sense in making it more accessible from outside of embassy-rp::dma module.
Channel type is however very useful and I don't think it's a good idea to reimplement it in my app. The problems I've run into is getting DMA channel number(convenience) and binding a custom IRQ handler(blocker). This PR addresses those two.

The only negative I can see is the annotations. In my own app I'm passing Irqs to a function that passes it to dma::Channel::new, this way I don't need extra annotations. I believe that examples that I needed to modify are atypical in this regard. So they make the "problem" seem bigger than it actually is.

[dobrowolski-lukasz]

Ahh. Did You mean adding another method to Channel type? Something like Channel::new_with_custom_irq_handler?

If so then I'm still unsure if it's sensible. As I've written those annotations seem to be needed only in weird situation when global Irqs is passed to Channel::new directly. I feel this will only happen in examples or poorly structured code.

I could change the examples so that the annotations aren't needed but I feel that It's better to leave them as is, so that when somebody struggles they will have a solution shown in examples.

[Dirbaio]

Awaiting a transfer needs the interrupt handler to wake the waker. If the user forgets to bind the interrupt handler, await would simply hang.

This is why the typelevel interrupt binding stuff exists. It forces the user to "prove" to embassy-rp that they've bound the right interrupt handler. This way it either works, or it fails to compile. There's no way to make it compile but hang.

This is all embassy-rp should care about. You bound embassy-rp's irq handler? you can await transfers. You didn't? then you can't.

It shouldn't care whether you've bound a custom irq handler or you've bound nothing at all. It's not going to do anything different based on that. This is why making new generic on the irq handler, or adding new_with_custom_irq_handler doesn't make sense. You shouldn't need to pass in your custom irq handler.

[dobrowolski-lukasz]

It shouldn't care whether you've bound a custom irq handler or you've bound nothing at all. It's not going to do anything different based on that. This is why making new generic on the irq handler, or adding new_with_custom_irq_handler doesn't make sense. You shouldn't need to pass in your custom irq handler.

I feel that there is a misunderstanding here. Code before this PR hardcodes that embassy-rp::dma::InterruptHandler as the only handler accepted by Channel::new. This PR relaxes that to "any handler that binds the DMA IRQ".

I've discovered this when I've bound my custom IRQ handler and the code failed to compile because Channel::new didn't accept Irqs because it didn't bound embassy-rp::dma::InterruptHandler specifically.

[dobrowolski-lukasz]

fn new(ch, irq) -> Channel<'a, Async>;
fn new_blocking(ch) -> Channel<'a, Blocking>;

I don't understand "blocking dma" concept. I have guesses but those make little sense:

1. Pooling busy loop. Possible to implement but no gains.
2. "Enclosed async/await". embassy-rp::dma would have to bind the IRQ on behalf of the user but what if the user manually binds IRQ handlers for other channels? Would this blocking dma be usable in blocking code? Built in mini-executor. I guess it would be possible to implement this but it seems like a lot of work and complication for little gain.

[orukusaki]

I would be surprised if this really fixes the problem you have. When a transfer completes, the IRQ handler wakes the channels waker, so your loop should continue promptly. Are you running in an InterruptExecutor? Using that, with a correctly set priority should help.

[dobrowolski-lukasz]

I would be surprised if this really fixes the problem you have. When a transfer completes, the IRQ handler wakes the channels waker, so your loop should continue promptly. Are you running in an InterruptExecutor? Using that, with a correctly set priority should help.

In theory, yes. There are two problems in practice:

1. I don't think I can use InterruptExecutor: InterruptExecutor doesn't work in rp2040 multicore. #1634. The task handling handling DMA (and the second one that generates audio) are running on core 1 and embassy handles all DMA IRQs on core 0.
2. The second task that generates audio blocks for a long time and it would be difficult to me to make it async.

Maybe I could shuffle my app around to make it work but that would be a workaround and I have other constraints too.

[Dirbaio]

Code before this PR hardcodes that embassy-rp::dma::InterruptHandler as the only handler accepted by Channel::new.

this is correct, it's needed to ensure transfer.await doesn't hang.

This PR relaxes that to "any handler that binds the DMA IRQ".

this is wrong. with this the user can pass a random handler, and write code that still uses transfer.await which will compile fine but hang. This is what we want to prevent.

I don't understand "blocking dma" concept.

"blocking" is perhaps not the best name. It basically means "not async, ie you can't transfer.await". A channel in this mode wouldn't be able to await transfers, but would still be able to

• blocking busy-wait for transfers
• non-blocking ask "is the transfer done?" to poll it from some main loop
• handle transfer completion in a custom irq handler.

which I guess is what you want?

The point of having this new "mode" represented as a generic param is to prevent the user from doing transfer.await at compile time if they didn't bind embassy-rp's standard irq handler.

[dobrowolski-lukasz]

This PR relaxes that to "any handler that binds the DMA IRQ".

this is wrong. with this the user can pass a random handler, and write code that still uses transfer.await which will compile fine but hang. This is what we want to prevent.

I see. This indeed is a problem. By making Channel reusable we loose correctness grantees.
How about splitting Channel type into two: ChannellInner(name to be changed) that has reusable code and accepts any irq handler and ChannellOuter(name to be changed) that accepts only embassy-rp::dma::InterruptHandler and implements all the methods that return Transfer?

"blockig dma" concept: I understand it now. It does make sense. Through it's not what I need and it's something different then this PR.

[orukusaki]

Maybe I could shuffle my app around to make it work but that would be a workaround and I have other constraints too.

I have an app with an OLED screen. Core1 renders the next frame, and Core0 does everything else.
A task running in an InterruptExecutor on Core0 sends one frame to the screen over SPI while Core1 is drawing the next frame, then they swap buffers over a zerocopy channel and do the same again.
The timing requirements are not as tight as for audio, but it works really well, so maybe something like that could work for you.

[Dirbaio]

How about splitting Channel type into two: ChannellInner(name to be changed)

Channel<Async> and Channel<Blocking> is this split.

[dobrowolski-lukasz]

I've gone with Auto/Manual naming scheme. Another I've came up with is Managed/Manual. Let me know what You feel is better.

I've made new_manual function unsafe through I'm unsure if it's correct use this keyword.