Support field arrays

[japaric]

Start here

[cwoodall]

I propose the following:

1. This optimization occurs for the trivial case, as above.
2. This optimization occurs if the registers share names as <name>_X where X is in the range 0..N, where X has no gaps. These registers can occur at arbitrary places in peripheral definition.

If there are gaps then 1 of 2 things can happen:

1. No optimization
2. You generate an array of Option<PinCnf>'s instead. This creates some overhead and is likely not ideal.

[whitequark]

Doesn't SVD natively support this kind of arrays? I would argue this is a definition bug (or at least should be handled by an SVD-to-SVD transformation...)

[japaric]

@whitequark It does but, IIRC, the problem was that sometimes the memory layout is 'A B C A B C A B C' but instead of having a "A B C repeats itself N times at address 0x0" in the SVD, you have "C repeats N times at address 0x8 with offset pattern of 0xC", "A repeats N times at address 0x4 with offset pattern of 0xC", "B1 is at address 0x4", "B2 is at address 0x10", etc. so I punted on it because it wasn't trivial to merge those bits of information and just "unrolled" all the array information in what you see in the issue description.

[archaelus]

I ran into what I think is a similar problem recently while trying to come up with generic GPIO pin code for the stm32f41x - using the standard svd file and svd2rust I end up with a huge number of differently typed/named pin control functions, and that in turn forces me to write big match statements to convert a port and pin number to a pin control function.

I tried to modify the svd file I was using to use svd dim arrays for GPIO ports and pins, and I think svd2rust supports that at the register level, but not at the field level.

Am I trying to do a thing (simplify GPIO pin control code by making the pin register interface more uniform and array based) that is not intended? Am I missing an alternative to large match blocks that I could use instead? Alternately, if other people have the same problems, would it be helpful to implement SVD arrays for fields?

[whitequark]

Alternately, if other people have the same problems, would it be helpful to implement SVD arrays for fields?

This sounds like the right path here to me.

[japaric]

Am I missing an alternative to large match blocks that I could use instead?

Depending on what exactly you are doing you could simply use the bits method and deal with the bit shifts manually. Like I have done here.

would it be helpful to implement SVD arrays for fields?

It actually seems that SVD does support field arrays but I have never encountered one in a SVD file provided by a vendor.

Also, I don't know how that would map to Rust code. Register arrays make sense because a register maps to a Rust integer type in size (u8, u16, u32) so a field with type [Register; 4] is doable, but bitfields can be 2-bits big in size, which is a problem because [u2; 16] doesn't exist in Rust. It sounds like you want to be able to index a register (or its proxy) to return you a proxy with an index (e.g. Proxy { reg: &mut W, i: u8 }) that indicates which bitfield is supposed to modify, but I don't think that's compatible with how the Index trait works: you are supposed to return a reference and you can't return &Proxy with different values of i.

[archaelus]

The f3 library is a good example I think. In there, you're making an Led abstraction from the svd2rust translation, but need to bitshift and mask yourself as the svd and/or the translation didn't provide a more ergonomic alternative - (*GPIOE.get()).bsrr.write(|w| w.bits(1 << self.i)) for turning a LED on instead of something like (*GPIOE.get()).bsrr.write(|w| w.bs(self.i).set()). F3 is also using calling individual functions per pin to initialize the Leds which is the kind of thing I'd like to avoid - https://github.com/japaric/f3/blob/6c2bd803801c0cbf785ee1bce87d320dce9ba01b/src/led.rs#L42-L63 whereas I would love to be able to write instead:

gpioe
    .moder
    .modify( |_, w| 8..15.fold(w, |wp, pin| wp.moder(pin).output() ) );

I suppose it is possible to use bits() again, but I fear writing code like this:

gpioe.moder.modify( |_, w| 8..15.fold(w, |wp, pin| {
    let mask = 0b11 << pin*2; // 2 bit wide mask
    wp.bits( (wp.bits() & !mask) | (0b01 << pin*2) ) // set 0b01 (Output) as the mode at pin*2..pin*2+1 in the raw field without disturbing anything else
}))

My opinion here is that I'd like to move all the bitwise arithmetic into generated code so that the bugs can be fixed once and won't occur in code layered on top of svd2rust translations. As you're the driving force in the rust embedded ecosystem @japaric, I would love to know what your plan is on this stuff.

(I would also like (where the hardware actually does all behave the same over all the instances of it) for that Led code to be generic over port and pin number so that you could say wire additional leds to unused pins on a board and not need to modify any code other than the array of Leds)

[japaric]

Thanks for the examples, @archaelus.

I think the w.field(i).variant() API you have shown makes sense. I think such API should be generated iff the SVD makes uses of arrays at the field level. IOW, I don't think we should attempt to infer that arrays could be used for this or that set of fields.

We should start by finding some official vendor SVD that makes uses of arrays for fields. Or read carefully the SVD spec and add arrays to an existing SVD fiel, and probably run the XML linter on it as well.

Then there is at least one unresolved question about your API. What should be the behavior when one calls w.bs(16) or w.bs(17) but the array of fields only has 16 elements. Should those calls panic? Or should the implementation "wrap around" / mask the input i.e. n % 16 the input?

I would also like (where the hardware actually does all behave the same over all the instances of it) for that Led code to be generic over port and pin number

You can already do that. All the GPIO instances that have the same register block layout are actually newtypes over a common register block type and implement Deref<Target=RegisterBlock> so you could define Led like this:

struct Led<'a, P>
where
    P: Deref<Target = gpioa::RegisterBlock>
{
    port: &'a P,
    pin: u8,
}

let pa0 = Led { port: gpioa, pin: 0 };
let pb1 = Led { port: gpiob, pin: 1 };

Or you could maybe turn the port into a phantom type to drop the port field and the lifetime paramater. That may need an extra trait to get the address from the port type without requiring self.

[japaric]

related issue: #90 (non contiguous register arrays)

[roysmeding]

People seem to agree that this issue is more about supporting field arrays, so maybe it's a good idea to rename it to reflect this? Or maybe make a new issue about field array support and refer to it here?

I noticed the lack of support for this myself and only found that it's discussed here by obsessively reading issues :)

[japaric]

People seem to agree that this issue is more about supporting field arrays, so maybe it's a good idea to rename it to reflect this?

Yeah, that was my fault. I mistitled the issue about being about field arrays when the issue description was talking about register arrays. I'll ... just create another issue about the original issue.

[cs2dsb]

I think this would help/resolve one of the things I've bumped into in my code. I'll explain it here in case it's not quite the same.

I want to have a nice api over the ADCs that will allow channels to be turned on with a call like:
configure_adc_channel(adc1, AdcChannel:C5, Rank:R10, Cycles:C480, blah blah)

I've implemented this with a bunch of nested macros making match statements which results in 34000 lines of code. This is because there are 3 adc * 18 channels * 16 ranks * 8 sample time cycle counts = 6912 unique combinations of register configuration.

I believe the compiler will get rid of all the branches that aren't used and inline the actually used bits but I've not checked this at all - for now I'm just happy if it works.

This is what 1 branch of the huge match statement looks like for reference:

match channel {
            Adc1GpioA::C0 => {
                //Configure the gpio attached to this channel for analog
                gpio.moder.modify(|_, w| w.moder0().analog());
                gpio.afrl.modify(|_, w| w.afrl0().af0());
                gpio.pupdr.modify(|_, w| w.pupdr0().none());
                gpio.ospeedr.modify(|_, w| w.ospeedr0().low());
                gpio.otyper.modify(|_, w| w.ot0().open_drain());
                match rank {
                    AdcRank::R1 => {
                        match samp_cycles {
                            AdcSampCycles::C3 => {
                                //Configure the adc for the right sample time
                                adc.smpr2.modify(|_, w| w.smp0()._3cycles());
                                //Configure the given rank to sample the given channel
                                adc.sqr3.modify(|_, w| w.sq1().channel0());
                            }
...

[Dirbaio]

This is done in #400, released in v0.18, right?

[burrbull]

Yes, in #400 and #503