Print array fields separate instead of as array in Debug and defmt::Format impls.

[de-vri-es]

The first implementation of Debug and defmt::Format printed array fields as actual arrays. However, that is rather hard to read. This PR changes them to print as numbered fields (which is also how they are named in the datasheets, so that's nice).

For example, it generates this:

    impl core::fmt::Debug for Af11chCmp {
        fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
            f.debug_struct("Af11chCmp")
                .field("bkine", &self.bkine())
                .field("bkcmpe0", &self.bkcmpe(0usize))
                .field("bkcmpe1", &self.bkcmpe(1usize))
                .field("bkcmpe2", &self.bkcmpe(2usize))
                .field("bkcmpe3", &self.bkcmpe(3usize))
                .field("bkcmpe4", &self.bkcmpe(4usize))
                .field("bkcmpe5", &self.bkcmpe(5usize))
                .field("bkcmpe6", &self.bkcmpe(6usize))
                .field("bkcmpe7", &self.bkcmpe(7usize))
                .field("bkinp", &self.bkinp())
                .field("bkcmpp0", &self.bkcmpp(0usize))
                .field("bkcmpp1", &self.bkcmpp(1usize))
                .field("bkcmpp2", &self.bkcmpp(2usize))
                .field("bkcmpp3", &self.bkcmpp(3usize))
                .finish()
        }
    }
    #[cfg(feature = "defmt")]
    impl defmt::Format for Af11chCmp {
        fn format(&self, f: defmt::Formatter) {
            #[derive(defmt :: Format)]
            struct Af11chCmp {
                bkine: bool,
                bkcmpe0: bool,
                bkcmpe1: bool,
                bkcmpe2: bool,
                bkcmpe3: bool,
                bkcmpe4: bool,
                bkcmpe5: bool,
                bkcmpe6: bool,
                bkcmpe7: bool,
                bkinp: super::vals::Bkinp,
                bkcmpp0: super::vals::Bkinp,
                bkcmpp1: super::vals::Bkinp,
                bkcmpp2: super::vals::Bkinp,
                bkcmpp3: super::vals::Bkinp,
            }
            let proxy = Af11chCmp {
                bkine: self.bkine(),
                bkcmpe0: self.bkcmpe(0usize),
                bkcmpe1: self.bkcmpe(1usize),
                bkcmpe2: self.bkcmpe(2usize),
                bkcmpe3: self.bkcmpe(3usize),
                bkcmpe4: self.bkcmpe(4usize),
                bkcmpe5: self.bkcmpe(5usize),
                bkcmpe6: self.bkcmpe(6usize),
                bkcmpe7: self.bkcmpe(7usize),
                bkinp: self.bkinp(),
                bkcmpp0: self.bkcmpp(0usize),
                bkcmpp1: self.bkcmpp(1usize),
                bkcmpp2: self.bkcmpp(2usize),
                bkcmpp3: self.bkcmpp(3usize),
            };
            defmt::write!(f, "{}", proxy)
        }
    }

instead of this:

    impl core::fmt::Debug for Af11chCmp {
        fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
            f.debug_struct("Af11chCmp")
                .field("bkine", &self.bkine())
                .field(
                    "bkcmpe",
                    &[
                        self.bkcmpe(0usize),
                        self.bkcmpe(1usize),
                        self.bkcmpe(2usize),
                        self.bkcmpe(3usize),
                        self.bkcmpe(4usize),
                        self.bkcmpe(5usize),
                        self.bkcmpe(6usize),
                        self.bkcmpe(7usize),
                    ],
                )
                .field("bkinp", &self.bkinp())
                .field(
                    "bkcmpp",
                    &[
                        self.bkcmpp(0usize),
                        self.bkcmpp(1usize),
                        self.bkcmpp(2usize),
                        self.bkcmpp(3usize),
                    ],
                )
                .finish()
        }
    }
    #[cfg(feature = "defmt")]
    impl defmt::Format for Af11chCmp {
        fn format(&self, f: defmt::Formatter) {
            #[derive(defmt :: Format)]
            struct Af11chCmp {
                bkine: bool,
                bkcmpe: [bool; 8usize],
                bkinp: super::vals::Bkinp,
                bkcmpp: [super::vals::Bkinp; 4usize],
            }
            let proxy = Af11chCmp {
                bkine: self.bkine(),
                bkcmpe: [
                    self.bkcmpe(0usize),
                    self.bkcmpe(1usize),
                    self.bkcmpe(2usize),
                    self.bkcmpe(3usize),
                    self.bkcmpe(4usize),
                    self.bkcmpe(5usize),
                    self.bkcmpe(6usize),
                    self.bkcmpe(7usize),
                ],
                bkinp: self.bkinp(),
                bkcmpp: [
                    self.bkcmpp(0usize),
                    self.bkcmpp(1usize),
                    self.bkcmpp(2usize),
                    self.bkcmpp(3usize),
                ],
            };
            defmt::write!(f, "{}", proxy)
        }
    }

[de-vri-es]

Hmm, once concern I have: can we be sure the start index from the datasheet is actually 0? Maybe sometimes it will be 1 or even something else?

For example, the STM32F405xx ADCs have the registers SQR{1,2,3}, which have the fields SQ1..SQ16 spread over them. But the debug impl for all of them will just start counting at SQ0 (which doesn't even exist).

Maybe ir::Array should preserve the original field names so we can use them again in the debug impls?

[de-vri-es]

Adjusted the implementation to print the original field names, by storing them in ir::Field.

This does still pose a problem for the stm32 crates though, because the generated YAML files are checked in to the repo. So re-running the MakeFieldArray transform on them seems challenging.

[Dirbaio]

About field names: I'm not sure what's best. Printing indices starting with 0 will be inconsistent with datasheets/manuals, but printing indices starting with 1 will be inconsistent with the code the user is writing.

because the generated YAML files are checked in to the repo. So re-running the MakeFieldArray transform on them seems challenging.

They're checked in because they're maintained by hand. Only at the beginning they're extracted from the SVDs. The SVDs are garbage quality so they need fixing, improving, deduplicating across families manually, which is done by hand. So yes there's no easy way to get the "original" field names other than filling them in by hand.

[de-vri-es]

About field names: I'm not sure what's best. Printing indices starting with 0 will be inconsistent with datasheets/manuals, but printing indices starting with 1 will be inconsistent with the code the user is writing.

I understand. I think this is also a matter of how it's printed. For example, for a field called SQ5 which happens to be the first SQ field in a particular fieldset: sq[0] vs sq0 vs sq5.

Personally I think sq5 is the best. If you're working with the PAC and printing things for debugging, I think you're keeping a datasheet next to the debug output. So eliminating the mental acrobatics needed to go from actual fields to array indices is nice.

I would say sq[0] is still acceptable because it clearly shows the 0 is an array index and not part of the field name. But sq0 is just wrong, I think.

because the generated YAML files are checked in to the repo. So re-running the MakeFieldArray transform on them seems challenging.

They're checked in because they're maintained by hand. Only at the beginning they're extracted from the SVDs. The SVDs are garbage quality so they need fixing, improving, deduplicating across families manually, which is done by hand. So yes there's no easy way to get the "original" field names other than filling them in by hand.

I understand the choice. But it does complicate what I want to do now :p Maybe I can use the available data to recover field names automatically though.

I have a proposal on how to proceed: I can change the Debug/Format impls to use the original field name when available, but use array indexing syntax when not available: so either sq5: ... or sq[0]: ....

Additionally, I will have to remove the #[derive(defmt::Format)] proxy structs in order to be able to use array indexing syntax. This will probably also improve compile time, as it removes a duplicate struct for each fieldset and a call to the defmt::Format derive macro (instead it will be a single call to defmt::write!().

If you really don't want to use real field names in order to avoid confusion with the index, I can still do the second part of the proposal to switch to array indexing syntax and (probably) improve compile times.

/edit: Pushed a commit implementing the proposal.

For stm32 with unknown original field names it generates this:

    impl core::fmt::Debug for Sqr2 {
        fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
            f.debug_struct("Sqr2")
                .field("sq[0]", &self.sq(0usize))
                .field("sq[1]", &self.sq(1usize))
                .field("sq[2]", &self.sq(2usize))
                .field("sq[3]", &self.sq(3usize))
                .field("sq[4]", &self.sq(4usize))
                .finish()
        }
    }
    #[cfg(feature = "defmt")]
    impl defmt::Format for Sqr2 {
        fn format(&self, f: defmt::Formatter) {
            defmt::write!(
                f,
                "Sqr2 {{ sq[0]: {=u8:?}, sq[1]: {=u8:?}, sq[2]: {=u8:?}, sq[3]: {=u8:?}, sq[4]: {=u8:?} }}",
                self.sq(0usize),
                self.sq(1usize),
                self.sq(2usize),
                self.sq(3usize),
                self.sq(4usize)
            )
        }
    }

[de-vri-es]

What do you think of the proposal? I also think that recording original field names is a good idea for the future. It could also be used to improve generated documentation.

[Dirbaio]

I think adding array_names to the IR is a step too far, it complicates the IR for little benefit, plus the likelihood that someone will go through all existing yamls in stm32-data to fill them to match the manuals is zero.

I like the sq[0] syntax though. It makes it obvious that it's an array index instead of a field name. Could you keep that but remove array_names? Then it'll be good to merge.

[de-vri-es]

I think adding array_names to the IR is a step too far, it complicates the IR for little benefit, plus the likelihood that someone will go through all existing yamls in stm32-data to fill them to match the manuals is zero.

I think the main benefit will come from the ability to generate better documentation. I was quite confused for a while about what value to use for the index parameter of array field accessors in the PAC. If the original field names are known, the documentation could explain better how the index maps to actual fields.

This PR itself doesn't do that yet, but I was thinking it could be another use for the field names.

For the STM32 yaml files, I have the faint hope that some hacky scripting can restore the original field names from the SVD files.

I like the sq[0] syntax though. It makes it obvious that it's an array index instead of a field name. Could you keep that but remove array_names? Then it'll be good to merge.

I also think it's a big improvement already. At-least you don't have to count array elements to find the right one anymore :p I removed the original field names from the PR.

Maybe if I find myself with enough time on my hands I will look at a proof of concept for better array field documentation based on original field names and lobby to get it back in :p And then I can propose to print actual field names again ^_^

[Dirbaio]

thank you!