Implement union peripherals with clustering (TCA single mode / split mode on attiny's) #79

hammerlink · 2025-04-29T08:59:22Z

This PR addresses part of issue #4, specifically adding support for union types.

The TCA0 modes were already implemented at some point, but the functionality was lost in the commit history. I reintroduced them to peripherals with both modes clustered.

I noticed that the svd2rust crate allows peripherals to use clustered registers instead of direct registers. This feature allows a good handling for the TCA Peripheral as it has 2 different modes, Single or Split.

Example solution

        dp.tca0.tca_single().ctrla().write(|w| w.clksel().div1());
        dp.tca0.tca_split().ctrla().write(|w| w.enable().set_bit());

Clustering example in svd

    <peripheral>
      <name>TCA0</name>
      <description>16-bit Timer/Counter Type A</description>
      <baseAddress>0x00000A00</baseAddress>
      <registers>
        <!-- default registers come here <register>...</register> -->
        
        <!--clusterered registers -->
        <cluster>
          <name>TCA_SPLIT</name>
          <description>16-bit Timer/Counter Type A - Split Mode</description>
          <addressOffset>0x0</addressOffset>

          <!-- clustered registers <register>...</register> -->
        </cluster>
      </registers>
    </peripheral>

Example ATDF:

        <!-- Sub register group referenced by the union, contains the actual registers  -->
        <register-group caption="16-bit Timer/Counter Type A - Single Mode"
                         name="TCA_SINGLE"
                         size="0x40">
            <register caption="Compare 0" name="CMP0" offset="0x28" rw="RW" size="2"/>
            <!-- multiple <register>...</register> -->
         </register-group>

        <!-- Module union register group, targets the sub register groups -->
        <register-group caption="16-bit Timer/Counter Type A" class="union" name="TCA" size="0x40" union-tag="TCA.SINGLE.CTRLD.SPLITM">
            <register-group name="SINGLE" name-in-module="TCA_SINGLE" offset="0" union-tag-value="0"/>
            <register-group name="SPLIT" name-in-module="TCA_SPLIT" offset="0" union-tag-value="1"/>
        </register-group>

Testing: Tests passed locally.

Please review and let me know if additional changes or tests are needed!

notes

This only partially fixes the #4, as there are sub register-groups without the module register group being a union class (f.i. avr64du28 - "USB" peripheral). This was out of scope for my changes. This would need further analyses of how to include the registers of these sub groups. I already prepared the atdf peripherals to include these references, but without further implementation. It looks like these sub register-groups can be recursive as well.

…mode on attiny's)

Rahix

Hi, thanks a lot for working on this! I think the approach looks very reasonable. Although I could not find notes about using SVD clusters for overlapping regions, if svd2rust can handle it, it shall be fine for us. The resulting API from svd2rust makes sense to me at least.

I left some detailed review comments below, but generally I think there is still a bit of chaos in the data-structures representing the register groupings. Specifically, I do not like the way RegisterGroupHeader is structured, for a few reasons:

The name Header does not really make sense when the actual registers are stored inside as well.
You allow storing a lot of "unused" data, in various ways. This is a recipe for bugs because it means some data from the Chip structure is silently ignored during SVD generation.
- The Peripheral contains a BTreeMap of RegisterGroupHeader but you only ever access one of them directly via module_register_group_header(). The other "headers" are then referenced indirectly via get_union_register_group_headers(). To me, these should be subelements then.
- You have a lot of code gated on if p.is_union() { ... } else { ... } which is a pointer to me that there should really be an enum separating these two options instead. Right now, you can store registers alongside register groups but only one of them will be used.
- Related, only the union class is currently dealt with. I don't think we should allow storing other classes then, when they cannot be handled properly.
- There is a field for ungrouped registers inside RegisterGroupHeader, which seems to me like it could just be the registers from Peripheral instead?

I feel, it would be more reasonable to do it as follows. This should only represent data that's actually used and prevents bugs from accessing things that aren't actually in use.

pub struct Peripheral {
    // ...

    // this is just the top-level group we find in the peripheral instance. From
    // my findings, there should only ever be one `<register-group /> inside each
    // peripheral instance.  I think we should definitely error out if we ever
    // find more, because this will need to be fixed then.
    pub registers: RegisterGroup,
}

pub struct RegisterGroup {
    pub name: String,
    pub description: Option<String>,
    pub offset: usize,

    pub registers: BTreeMap<String, Register>,
    pub subgroups: Vec<RegisterSubgroup>,
}

pub enum RegisterSubgroup {
    Union(RegisterGroupUnion),
    // this would be for the remainder of issue #4, subgroups for creating
    // structure.  We should properly detect arrays so svd2rust can built the
    // correct accessors.  But this should be added in a later PR I think.
    Single(RegisterGroup),
    Array(RegisterGroupArray),
}

pub struct RegisterGroupUnion {
    pub name: String,
    pub description: Option<String>,
    pub offset: usize,

    pub variants: BTreeMap<String, RegisterGroup>,
}

The parsing then needs to be adjusted accordingly. I think it makes sense to create a new src/atdf/register_group.rs module module for the register group parsing.

I pushed a change to main to add the ATxmega128A1 as another regression testcase. Please rebase to pull it in for your changeset as well.

Rahix · 2025-04-29T11:59:17Z

src/atdf/register.rs

+
+        let size = register_group_item_el.attributes.get("size").and_then(|d| {
+            if !d.is_empty() {
+                util::parse_int(d).ok()


This is dangerous, you silently place None into the size when parsing the number fails. You should propagate the error outwards, e.g. using

Suggested change

util::parse_int(d).ok()

Some(util::parse_int(d)?)

the size code was removed

Rahix · 2025-04-29T12:07:48Z

src/atdf/register.rs

+        let offset = register_group_item_el
+            .attributes
+            .get("offset")
+            .and_then(|d| {
+                if !d.is_empty() {
+                    util::parse_int(d).ok()
+                } else {
+                    None
+                }
+            });


Same here, don't .ok() the parse_int() result. Propagating the error gets a little more difficult, though. I think I would do it like this:

let offset = register_group_item_el .attributes .get("offset") .filter(|d| !d.is_empty()) .map(|d| d.parse::<usize>()) .transpose()?;

applied in register_group.rs reference

Rahix · 2025-04-29T12:08:20Z

src/atdf/register.rs

+        let count = register_group_item_el
+            .attributes
+            .get("count")
+            .and_then(|d| {
+                if !d.is_empty() {
+                    util::parse_int(d).ok()
+                } else {
+                    None
+                }
+            });


And same error silencing problem here.

the count code was removed

Rahix · 2025-04-29T12:08:57Z

src/atdf/register.rs

+        let class = register_group_header_el
+            .attributes
+            .get("class")
+            .and_then(|d| if !d.is_empty() { Some(d) } else { None })


Suggested change

.and_then(|d| if !d.is_empty() { Some(d) } else { None })

.filter(|d| !d.is_empty())

removed class from the register group struct

Rahix · 2025-04-29T12:09:06Z

src/atdf/register.rs

+        let description = register_group_header_el
+            .attributes
+            .get("caption")
+            .and_then(|d| if !d.is_empty() { Some(d) } else { None })


Suggested change

.and_then(|d| if !d.is_empty() { Some(d) } else { None })

.filter(|d| !d.is_empty())

applied in register_group.rs

Rahix · 2025-04-29T12:20:19Z

src/svd/peripheral.rs

+    let registers: Vec<svd_rs::RegisterCluster> = match p.is_union() {
+        true => {


Don't match on a bool value, use if {} else {} instead.

removed the code, but I learned that you can do an inline if {} else {}. Thanks!

Rahix · 2025-04-29T12:27:58Z

src/chip.rs

+    pub size: Option<usize>,
+    pub offset: Option<usize>,
+    pub count: Option<usize>,


Is it really sensible to have these as optionals? At least for offset, you seem to encode a default value somewhere in the svd-generation code below, so I think it would be better to store the proper values here already. If the ATDF doesn't give input, default values should be set in the ATDF-parsing code already.

For size and count I couldn't find a usage site at all? Did I miss it or are the values never used?

Good point, I have removed the option from offset, and defaulted it to 0.

size can be useful for defining address blocks in the svd peripheral, but removed it for now

count I am not entirely sure about, I have removed it

Rahix · 2025-04-29T12:29:49Z

src/chip.rs

+#[derive(Debug, Clone)]
+pub struct RegisterGroupHeader {
+    pub name: String,
+    pub class: Option<String>,


Out of curiosity, do we ever see classes other than union?

There are supposed to be others like (struct, bitfield, array).

But I did not encounter any in the current .atdf files. I only encountered class attributes (always union) in the ATDF files coming from avr-mcu.

The newer/current ATDF files coming from the Atmel site seem to use another format. They seem to work with modes instead of nested register groups. I will probably make a PR for this one later on.

<register-group caption="16-bit Timer/Counter Type A" name="TCA" size="0x40"> <mode caption="Single Mode" name="SINGLE" qualifier="TCA.SINGLE.CTRLD.SPLITM" value="0"/> <mode caption="Split Mode" name="SPLIT" qualifier="TCA.SPLIT.CTRLD.SPLITM" value="1"/>

There are supposed to be others like (struct, bitfield, array).

Out of curiosity, where did you get this info?

The newer/current ATDF files coming from the Atmel site seem to use another format.

Are all ATDFs updated for this newer schema? Or just some newer MCUs?

In avr-device the following list would be updated because of the unions

Rahix · 2025-04-29T12:33:42Z

src/svd/chip.rs

+        return peripheral
+            .get_union_register_group_headers()
+            .iter()
+            .any(|(header, _)| !header.registers.values().len() > 0)


Isn't this just a convoluted way to say header.registers.is_empty()? Also I think this is actually inverted? The return should be true when the list is not empty?

code has been removed

Rahix · 2025-04-29T12:36:08Z

src/svd/chip.rs

+    if peripheral.is_union() {
+        return peripheral
+            .get_union_register_group_headers()
+            .iter()
+            .any(|(header, _)| !header.registers.values().len() > 0)
+    }
    let regs = !peripheral.registers.is_empty();
    if !regs {
        log::warn!("No registers found for peripheral {}", peripheral.name);


You are again using an if { return } return pattern here and this time it actually introduced a bug: You are skipping the empty peripheral warning further down in the function with your early return. Use if {} else {} instead, in this case I'd do it like this:

let regs = if peripheral.is_union() { // ... } else { !peripheral.registers.is_empty() }; if !regs { log::warn!("No registers found for peripheral {}", peripheral.name); } regs

code has been removed

* WIP - improve the register group structure and update the parsing accordingly * fix the dynamic address calculation * remove unused base_address fn * rename parse_list args & run cargo fmt

hammerlink · 2025-05-01T06:00:52Z

Thanks @Rahix for the thorough review, it is much appreciated!

I've rebased the branch to include the ATxmega128A1 regression test as requested.

Implementation Updates

I've restructured the register group handling with your suggestions in mind, which provides a much cleaner representation of the data and eliminates the potential for unused/ignored data issues.
While implementing these changes, I encountered cases in actual ATDF files (such as avr64du28.atdf) that demonstrate nested register groups requiring recursive handling:

<register-group caption="USB Device Controller EP_TABLE" name="USB_EP_TABLE" size="0x122">
   <!-- containing multiple registers -->
   <register-group caption="USB Device Controller EP"
                   count="0x10"
                   name="EP"
                   name-in-module="USB_EP_PAIR"
                   offset="0x20"
                   size="0x100"/>
</register-group>
<register-group caption="USB Device Controller EP_PAIR" name="USB_EP_PAIR" size="0x10">
   <register-group caption="USB Device Controller OUT"
                   name="OUT"
                   name-in-module="USB_EP"
                   offset="0x00"
                   size="0x08"/>
   <register-group caption="USB Device Controller IN"
                   name="IN"
                   name-in-module="USB_EP"
                   offset="0x08"
                   size="0x08"/>
</register-group>
<register-group caption="USB Device Controller EP" name="USB_EP" size="0x8">
   <!-- containing multiple registers -->
</register-group>

This led to a full recursive implementation that handles both the union and non-union cases, unintentionally implementing issue Fix parsing of nested register groups #4 completely.
But to keep the changes simple I did use the is_union bool. So for the moment it only works for the class=union.
Though refactoring this boolean will fully enable the nested register group support for all atdf. There are actually quite a few impacted mcus.

Address & Offset Handling

I noticed some complexities with the current address/offset calculation approach:

The original implementation stored absolute addresses on registers and calculated base addresses afterward
This approach becomes problematic with nested register groups
The calculated base addresses often differ from what's defined in the ATDF instance

I've modified this to more closely align with how ATDF and SVD both use relative offsets between parents and children, which simplifies the nested register-group handling.

* fix invalid address block regressions * re-implement baselining the start address & fix address block issue * apply PR comments * run format

hammerlink · 2025-05-12T19:51:24Z

Hi @Rahix , just checking in to see if you’ve had a chance to take a look at the updated PR. Let me know if there’s anything else I can do to move this forward.
Thanks!

Rahix · 2025-05-13T10:34:42Z

Didn't get to it yet, but your PR is on my list :) Will ping you once I have reviewed your updated changeset!

hammerlink added 2 commits April 29, 2025 10:17

implement union peripherals with clustering (TCA single mode / split …

f4b2dc0

…mode on attiny's)

run cargo fmt

f9b07a5

Rahix requested changes Apr 29, 2025

View reviewed changes

hammerlink added 3 commits April 30, 2025 20:42

Merge branch 'Rahix:main' into main

041c66e

Merge branch 'Rahix:main' into main

d2428ed

Implement register group restructure + recursive implementation (#1)

035344c

* WIP - improve the register group structure and update the parsing accordingly * fix the dynamic address calculation * remove unused base_address fn * rename parse_list args & run cargo fmt

hammerlink added 3 commits May 1, 2025 23:27

Fix regression test issues (#2)

2b09f9d

* fix invalid address block regressions * re-implement baselining the start address & fix address block issue * apply PR comments * run format

remove register group item

c74b75e

fix register-group offset docs & update

b35963a

hammerlink requested a review from Rahix May 5, 2025 09:42

Rahix mentioned this pull request Jan 10, 2026

Does not handle attiny412 TCA0 possibly due to split mode #30

Closed

	.and_then(\|d\| if !d.is_empty() { Some(d) } else { None })
	.filter(\|d\| !d.is_empty())

		let registers: Vec<svd_rs::RegisterCluster> = match p.is_union() {
		true => {

Implement union peripherals with clustering (TCA single mode / split mode on attiny's) #79

Are you sure you want to change the base?

Implement union peripherals with clustering (TCA single mode / split mode on attiny's) #79

Uh oh!

Conversation

hammerlink commented Apr 29, 2025

notes

Uh oh!

Rahix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hammerlink commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation Updates

Address & Offset Handling

Uh oh!

hammerlink commented May 12, 2025

Uh oh!

Rahix commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hammerlink commented May 1, 2025 •

edited

Loading