Clustering DMA channels into an array has lost CTRL reset values

[cbiffle]

I had some code that resembled this:

        for smnum in 0..STRAND_COUNT {
            // irrelevant stuff omitted
            p.DMA.ch[smnum].ch_al1_ctrl.write(|w| unsafe {
                w.read_error().set_bit() // to clear
                    .write_error().set_bit() // to clear
                    .bswap().set_bit()
                    .treq_sel().bits(0 + smnum as u8) // DREQ_PIO0_TX0..3 are contiguous
                    .incr_write().clear_bit()
                    .incr_read().set_bit()
                    .data_size().size_word()
                    .high_priority().set_bit()
                    .en().set_bit()
            });
        }

It causes rather elaborate misbehavior on DMA channel 0.

Can you see why?

Unfortunately, you can't see why, as the critical piece of information does not actually appear in the code. svd2rust defaults any register bitfields that are not explicitly set to its idea of the reset value (a design flaw, imo, but hey). The rp2040 SVD from the RaspPi folx have all the DMA channel registers modeled separately, which is good, because they have different reset values.

Why? Because for the CHAIN_TO field to be a no-op, it has to be set to the ID of the channel being configured. So for channel 0 it's 0, for channel 1, it's 1, etc.

The PAC loses this information from the SVD by throwing away the types of channels 1-7 and using the channel 0 type for everything. Of course, you have to do that to make an array. (svd2rust also makes types distinct just so they can have different reset values, preventing them from being in an array, which, well, you can guess my opinion on that.)

The net effect of this is that every channel other than 0 implicitly triggers channel 0 when it completes. Every time.

I'm not actually sure how to fix this. Moving away from the array would be terribly unfortunate and cause me to basically hack an array back in so that I can loop over channels (god forbid you wanted to allocate channels dynamically, with them all becoming different types!). However, the current situation is a rather elaborate footgun for DMA. I don't believe svd2rust has any notion of a field that must be set explicitly when writing a register, which otherwise would be a reasonable thing to reach for here.

[thejpster]

Oh no! Good spot. We should fix this in the docs in the first instance. One option may be to use modify instead of write, as that reads the current value instead of starting with the reset value.

Also rustfmt does a better job if every closure line starts w. and ends ; (just remember to return w at the end).

Also I’m curious if you’ve joined the legions going “bah, no ST in stock, but Raspberry Pi seem to have millions of their chip in stock” or if this is just like a fun side thing. But feel free to decline to answer!

[cbiffle]

Oh no! Good spot. We should fix this in the docs in the first instance. One option may be to use modify instead of write, as that reads the current value instead of starting with the reset value.

That's true, though it does so at a cost of at least three additional cycles (edit: ...four, in practice), and my application is rather performance-sensitive. Ah, the joys of porting from M4 to M0+.

SVD itself has concepts for indicating that a register must only be altered using an equivalent to modify, but I believe svd2rust discards this information. (To be fair, the CMSIS C API generator does too, and I get the sense svd2rust was developed with reference to CMSIS rather than the SVD spec.) Otherwise that might be an option.

Also rustfmt does a better job if every closure line starts w. and ends ; (just remember to return w at the end).

rustfmt and svd2rust are locked in a forever-battle and I'm not getting between them. :-)

Also I’m curious if you’ve joined the legions going “bah, no ST in stock, but Raspberry Pi seem to have millions of their chip in stock” or if this is just like a fun side thing.

Yeah, I've got an art project that I designed around the STM32L4, which ST does not appear to be able to make fast enough. So I'm having to redesign it, and I happened to already have some rp2040 eval hardware from when the chip came out. So far I'm really happy with the chip. ...until it, too, vanishes from the market because 2022. :-\

For robustness I'm also porting to the ATSAMD51, though their PAC has had much more serious issues and I've set it aside for now.

[thejpster]

If it helps almost all RP2040 silicon can run at 250 MHz. I think I’d rather have two M0+ at 250 MHz than one M4 at 80 MHz (or even 160 MHz), especially given the RP2’s wide bus, although YMMV.

And Eben says he has a lot of product on hand, so I’m not expecting shortages. Although nothing is impossible given the state of things.

https://twitter.com/ebenupton/status/1514691280894402564?s=21&t=p9-xeepcv6ujSszYl_93hA

https://twitter.com/ebenupton/status/1455647401268682757?s=21&t=p9-xeepcv6ujSszYl_93hA

Might be worth a ticket on svd2rust about disallowing write. I’ll do it later if I remember, when I’m not on my phone.

[9names]

jannic mentioned that this has been discussed in the past on matrix, it's just that no-one created an issue so I completely forgot about it... :/

TLDR for those not wanting to follow the link:

ithinuel: A solution could be not to use write/reset_value. I think this could be enforced by a patch to the svd file to remove the reset value and which should remove the impl for crate::Resettable. If a type does not impl resettable, Reg won't have write/reset_value defined.

I have tested - removing reset values also removes register.write(), so that is a viable strategy for removing that footgun. We'll still want to have sufficient documentation around this, and the HAL driver for DMA will have to iron over this risky behaviour.

[jannic]

If we do remove the reset values, is there still some way to write the register without reading it first? Otherwise, I'm not 100% sure what's the right thing to do: On the one hand, the existing behavior is obviously undesirable because it can easily cause subtle, difficult to debug mistakes. On the other hand, IMHO the PAC's main job is to provide low-level access to all hardware features, without much abstraction. User-friendliness can be added at a higher level, e.g. in the HAL layer. The best solution would combine both, easy access to all features without additional overhead, and reasonable usability without unnecessary foot guns. One way could be a kind of register.write() which requires manually setting all bits, without fallback to the (wrong) reset value. But I have no idea if such a thing is possible with the current svd2rust tooling.

[9names]

Yes, write_with_zero() still works without reset values, but that is slightly less misleading than the existing API. It's still not going to enforce that you to set all values.

[thejpster]

Should we just patch the SVD to add docs in the first instance?

[9names]

Yep. Patch the docs, bump patch number, release.

[jannic]

Can we consider this solved by #57 and close the ticket?