Runtime DMA interrupt binding

[bjoernQ]

This is ready for review now.

Basically, the user-facing change is there is that configure configures the DMA channel for blocking mode while configure_for_async configures the channel for async operation. The async interrupt handler for that channel is only installed when configure_for_async is used.

It's possible to register an interrupt handler for a blocking channel but it's not really useful, yet. However, that wasn't really possible before and is something for a future enhancement. (And would need some new/additional API probably)

[bjoernQ]

blocked by
#1294
#1299

needs
esp-rs/esp-pacs#209

somewhat blocked by #1310

[jessebraham]

All blockers have been resolved!

[JurajSadel]

embassy_i2s_read is not working on C6, H2 and S3. For me, it prints only:

Init!
Start

main works fine, though.

[bjoernQ]

Blocked again since it will need the proc-macro-changes from #1341

[jessebraham]

Unblocked again! 😁

[jessebraham]

Lots of changes here but otherwise LGTM, thanks for taking care of this.

One little nitpick, plus still need to address whatever remaining comments from @MabezDev I suppose, then we should probably be good to merge.

[bjoernQ]

I think I addressed everything? Or did I miss something?

[jessebraham]

I think that's it, we will need @MabezDev to approve this to merge however.

[MabezDev]

Noticed a couple of other bits, but I have a bigger question (and I'm sorry in advance)

Do we ever plan Dma Channels to be used publically? In my head I feel like they're a an implementation detail for other drivers. We currently only have impls like impl<'d, C> Channel<'d, C, crate::Blocking>, would we ever have impl<'d, C> Channel<'d, C, crate::Async>? I feel like if we were, for example, async memcpy, we still wouldn't use the DMA channels directly but we'd have a AsyncMemcpy driver which took in the channel.

My point is, I think we can maybe avoid a whole bunch of Dma type state and complexity by:

• Dropping the Mode param for Channel
• Making set_interrupt_handler public always on Channel
• Add a new method on Channel called install_async_handler (choose whatever name you like, it will be a pub(crate) function) which the peripheral driver will call to bind the async interrupt (basically what we do now on the configure_async)

It's then up to the driver to install the async handler, and the rest of the API stays as it were now. For instance, SpiDma would get new method(s) with_dma_async, but dma configuration step would stay the same. The result is a SpiDma<_, _, _, crate::Async>, but the dma channel doesn't care, and IMO it shouldn't. The DMA is dumb peripheral that copies bytes from one place to another.

Maybe I'm missing something, please feel free to correct me! and sorry again for delaying this, I just want to make sure we're going down the right path before we commit to converting all the drivers.

[bjoernQ]

I think it might be somewhat useful to have control over DMA interrupts as a user. Unfortunately most peripherals need more than them (those have more checks in their "wait" implementation).

Another option would be to have the blocking drivers in charge of that

In any case the current transaction API isn't suitable for that and we would need another set of APIs

[bjoernQ]

But for sure: if we agree DMA is "internal" to the HAL that makes things definitely easier. Channels wouldn't need public functions then at all

[MabezDev]

I think it might be somewhat useful to have control over DMA interrupts as a user. Unfortunately most peripherals need more than them (those have more checks in their "wait" implementation).

I do agree here, and I think we can still allow that. For instance it's possible to bind the DMA interrupt before calling with_dma, and because with_dma doesn't rebind the interrupt it will still work. This might get a bit hairy though, particularly if a user expected their interrupt to run, but they use with_dma_async which rebinds the interrupt. In that case the current code works better.

I think we need to figure out that use case for DMA interrupts. Perhaps it becomes part of the peripheral driver to register the interrupt? I don't know how it will work with all peripherals, but SpiDma, could have method to configure the DMA interrupt - not sure how I feel about this currently.

[MabezDev]

Let's proceed with this for now I think, unless anyone feels like we shouldn't. I think this is a great approach, so thanks for putting in the time to get it working!

I think my previous comments might have been trying to optimize prematurely; we don't currently know all the DMA use cases and I think your solution here is the most flexible going-forward - we can always change our path as we refine our APIs.

[MabezDev]

Removing from the queue just to address the two other comments then this is good to go from my perspective!

[bjoernQ]

Let's proceed with this for now I think, unless anyone feels like we shouldn't. I think this is a great approach, so thanks for putting in the time to get it working!

I think my previous comments might have been trying to optimize prematurely; we don't currently know all the DMA use cases and I think your solution here is the most flexible going-forward - we can always change our path as we refine our APIs.

Sounds good to me! Definitely removing the type-state etc. later is much less work than doing it now and then realizing we need it for something and re-adding it 👍

Will address the remaining two comments and rebase this