DMA `ch` array prevents move of single channels

[jounathaen]

Please forgive me, if this is stupid, but I get a little confused by the nitty-gritty details of the move and borrow semantics of the SVD HALs.

The 12 DMA channels can be found in the DMA.ch array, but if I'm not wrong, this prevents a certain peripheral from taking ownership of a single channel:

let mut pac = pac::Peripherals::take().unwrap();
let mut chan0 = pac.DMA.ch[0];

error[E0507]: cannot move out of dereference of `DMA`
   --> src/main.rs:143:21
    |
143 |     let mut chan0 = pac.DMA.ch[0];
    |                     ^^^^^^^^^^^^^
    |                     |
    |                     move occurs because value has type `rp2040_hal::rp2040_pac::dma::CH
`, which does not implement the `Copy` trait
    |                     help: consider borrowing here: `&pac.DMA.ch[0]`

One could pass the whole DMA struct to please the borrow checker, but that obviously prevents the independent use of channels. I'd suggest to cluster the 12 channels in separate modules.

[9names]

Wouldn't you still be unable to move the DMA channels out due to needing the DMA singleton even if they weren't clustered? PAC code doesn't really allow you to move anything out of it for safety reasons.

As it is, you can:

• find a way to borrow the DMA singleton where you need to access the registers.
• At a HAL level, add 12 separate channel singletons you can hand to users. Under the hood the HAL code will still need to steal access but at least you know that it's safe for users.
• use pac::Peripherals::steal to get access to the peripherals struct again (and ensure only one user of each reference)

[9names]

Mattias made a PR for supporting DMA in the HAL, perhaps referencing that will help? https://github.com/rp-rs/rp-hal/pull/209/commits/1a9b8066a4d6df4dd68dd59e6396dc96e1789ad2#diff-7e6cf5be0b26ed108f5586c99d9af70d0b1d85ff199002e7dad2c408feb5fab3R54

[jounathaen]

Ok, I overlooked your first comment, so I deleted my initial response. Thank you for the good suggestions. However, I'm not convinced (yet), so if I may add my few cents:

As it is, you can: find a way to borrow the DMA singleton where you need to access the registers.

Well, from my understanding, the channels are rather independent on this chip. So, a borrow of all of them at once seems to make code unnecessarily harder. Especially when you consider using the DMA in separate threads/tasks.

At a HAL level, add 12 separate channel singletons you can hand to users. Under the hood the HAL code will still need to steal access but at least you know that it's safe for users.

This is how existing HALs seem to approach this. But the code for this is hard to understand (at least for me with intermediate Rust knowledge). There might be a simpler way to archive this, but I struggled hard to do so. So, let me turn this around: What is the advantage of the combination in an array? I mean, you can iterate over the channels, but I can't really think of a scenario where this makes sense for DMA.

use pac::Peripherals::steal to get access to the peripherals struct again (and ensure only one user of each reference)

Well, yeah, that would probably work, but honestly speaking: I think that's just an ugly workaround.

[9names]

I think that's just an ugly workaround.

That's true, but Rust really does not like you accessing potentially shared mutable state, and all memory-mapped IO just looks like a pub static slice. Things that aren't ugly workarounds (at a PAC level) don't really exist as far as I know.

What is the advantage of the combination in an array

Having an array means you can reuse the same code for interacting with each channel without resorting to macros (as stm32f1xx does) or more complicated generics (as stm32f4xx does). So it's easier to understand, even if it's a little bit more loose with unsafe for register access. A user can also get a DMA channel without having to manually name/number it, which can make for a nicer API - but we haven't written it yet, and it would exist at the HAL level, not the PAC level.

hard to understand (at least for me with intermediate Rust knowledge)

It's hard for me too.

Doing it this way is the tradeoff you make when using svd2rust based PACs - they're limited by what you can describe in an SVD, and the singletons they provide mean you get very coarse mutual exclusion which helps keep things safe in the simpler case but there are always places you end up working around it. Patching around this in the PAC is very time intensive so not many folks do that - it's faster to deal with at the HAL level. Accessing the hardware through raw pointers is no worse than the typical C APIs here, and folks are already using those.

There are other approaches in this space that use a thinner abstraction, like stm32ral or chiptool. chiptool is most relevant here since supports RP2040 (it's used to generate the embassy rp2040 hal). But if you don't like the workaround here you probably won't like chiptool PACs either - they assume all register access is unsafe and rely on you to build all the higher level abstractions yourself (in a HAL, or in your program). It is at least honest and consistent, which is a nice change.

[jannic]

Is there anything left to do for this ticket? As I understand it, while the API is a little bit ugly, there's nothing we can do in rp2040-pac as it's just the way svd2rust works. Can we close this ticket?