Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 13 additions & 29 deletions src/atdf/peripheral.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,52 +13,36 @@ pub fn parse_list(
let module_name = module.attr("name")?;

for instance in module.iter_children_with_name("instance", Some("module")) {
let mut registers = vec![];

// Find corresponding module
let module = modules.first_child_by_attr(Some("module"), "name", module_name)?;

// The register definitions can reference value-groups, that are stored on the same
// level as the register-groups, so we parse them in here first.
let value_groups = atdf::values::parse_value_groups(module)?;

for register_group in instance
.children
.iter()
.filter_map(|node| node.as_element().filter(|e| e.name == "register-group"))
{
let name = register_group.attr("name-in-module")?;
let offset = util::parse_int(register_group.attr("offset")?)?;

let group = module.first_child_by_attr(Some("register-group"), "name", name)?;

for register in group.iter_children_with_name("register", Some("register-group")) {
registers.push(atdf::register::parse(register, offset, &value_groups)?);
}
}

let registers = registers
.into_iter()
.map(|r| {
(
match r.mode {
Some(ref mode) => format!("{mode}_{}", r.name),
_ => r.name.clone(),
},
r,
)
})
.collect();
// An instance should always have one register-group
let instance_register_group = match instance.first_child("register-group") {
Ok(rg) => rg,
Err(_) => continue,
};
let name_in_module = instance_register_group.attr("name-in-module")?;
let offset = util::parse_int(instance_register_group.attr("offset")?)?;
let register_group_headers =
atdf::register::parse_register_group_headers(module, offset, &value_groups)?;
let main_register_group = register_group_headers.get(name_in_module).cloned().unwrap();
let registers = main_register_group.registers;

peripherals.push(chip::Peripheral {
name: instance.attr("name")?.clone(),
name_in_module: name_in_module.clone(),
description: instance
.attr("caption")
.or(module.attr("caption"))
.ok()
.cloned()
.and_then(|d| if !d.is_empty() { Some(d) } else { None }),
registers,
register_group_headers: register_group_headers.clone(),
})
}
}
Expand Down
147 changes: 147 additions & 0 deletions src/atdf/register.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,150 @@ pub fn parse(
fields,
})
}

pub fn parse_list(
register_group_header_el: &xmltree::Element,
offset: usize,
value_groups: &atdf::values::ValueGroups,
) -> crate::Result<BTreeMap<String, chip::Register>> {
let mut registers = vec![];

for register in
register_group_header_el.iter_children_with_name("register", Some("register-group"))
{
registers.push(atdf::register::parse(register, offset, value_groups)?);
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let registers: Vec<_> = register_group_header_el
    .iter_children_with_name("register", Some("register-group"))
    .map(|reg| atdf::register::parse(reg, offset, value_groups))
    .collect()?;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And you can actually drop the .collect() because you are going to just turn the Vec into an iterator again two lines below)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged the 2 statements into 1

Ok(registers
.into_iter()
.map(|r| {
(
match r.mode {
Some(ref mode) => format!("{mode}_{}", r.name),
_ => r.name.clone(),
},
r,
)
})
.collect())
}

pub fn parse_register_group_headers(
module_el: &xmltree::Element,
offset: usize,
value_groups: &atdf::values::ValueGroups,
) -> crate::Result<BTreeMap<String, chip::RegisterGroupHeader>> {
let mut register_group_headers = BTreeMap::new();
for register_group_header_el in module_el
.children
.iter()
.filter_map(|node| node.as_element().filter(|e| e.name == "register-group"))
{
let name = register_group_header_el.attr("name")?.clone();
let class = register_group_header_el
.attributes
.get("class")
.and_then(|d| if !d.is_empty() { Some(d) } else { None })
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed class from the register group struct

.cloned();
let description = register_group_header_el
.attributes
.get("caption")
.and_then(|d| if !d.is_empty() { Some(d) } else { None })
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied in register_group.rs

.cloned();

let size = register_group_header_el
.attributes
.get("size")
.and_then(|d| {
if !d.is_empty() {
util::parse_int(d).ok()
} else {
None
}
});
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about .ok() on the parse_int() result.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the size code


let register_group_items = parse_register_group_items(register_group_header_el)?;
let registers = parse_list(register_group_header_el, offset, value_groups)?;

register_group_headers.insert(
name.clone(),
chip::RegisterGroupHeader {
name,
class,
description,
size,
register_group_items,
registers,
},
);
}
Ok(register_group_headers)
}

pub fn parse_register_group_items(
register_group_header_el: &xmltree::Element,
) -> crate::Result<BTreeMap<String, chip::RegisterGroupItem>> {
let mut register_group_items = BTreeMap::new();

for register_group_item_el in register_group_header_el
.children
.iter()
.filter_map(|node| node.as_element().filter(|e| e.name == "register-group"))
{
let name = register_group_item_el.attr("name")?.clone();
let description = register_group_item_el
.attributes
.get("caption")
.and_then(|d| if !d.is_empty() { Some(d) } else { None })
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@hammerlink hammerlink May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed description from the reference in register_group.rs

.cloned();

let name_in_module = register_group_item_el
.attributes
.get("name-in-module")
.and_then(|d| if !d.is_empty() { Some(d) } else { None })
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied in register_group.rs reference struct

.cloned();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the size code was removed

} else {
None
}
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied in register_group.rs reference


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And same error silencing problem here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the count code was removed


register_group_items.insert(
name.clone(),
chip::RegisterGroupItem {
name,
name_in_module,
description,
size,
offset,
count,
},
);
}

Ok(register_group_items)
}
69 changes: 69 additions & 0 deletions src/chip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,57 @@ pub struct Chip {
#[derive(Debug, Clone)]
pub struct Peripheral {
pub name: String,
pub name_in_module: String,
pub description: Option<String>,

pub registers: BTreeMap<String, Register>,
pub register_group_headers: BTreeMap<String, RegisterGroupHeader>,
}

impl Peripheral {
pub fn base_address(&self) -> Option<usize> {
if self.is_union() {
return self
.get_union_register_group_headers()
.iter()
.flat_map(|(h, _)| h.registers.values())
.map(|r| r.address)
.min();
}
self.registers.values().map(|r| r.address).min()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this an if {} else {} construct rather than if { return } return

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code was removed

}

pub fn module_register_group_header(&self) -> Option<&RegisterGroupHeader> {
self.register_group_headers.get(&self.name_in_module)
}

pub fn is_union(&self) -> bool {
let module_register_group_header = self.module_register_group_header();
match module_register_group_header {
Some(header) => header.is_union(),
None => false,
}
}

pub fn get_union_register_group_headers(
&self,
) -> Vec<(&RegisterGroupHeader, &RegisterGroupItem)> {
match self.module_register_group_header() {
Some(module_header) if module_header.is_union() => module_header
.register_group_items
.values()
.filter_map(|group_item| {
group_item.name_in_module.as_ref().and_then(|header_name| {
self.register_group_headers
.iter()
.find(|(_, header)| &header.name == header_name)
.map(|(_, header)| (header, group_item))
})
})
.collect(),
_ => vec![],
}
}
}

#[derive(Debug, Clone)]
Expand All @@ -52,6 +94,33 @@ pub enum ValueRestriction {
Enumerated(BTreeMap<String, EnumeratedValue>),
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"/>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub description: Option<String>,
pub size: Option<usize>,

pub register_group_items: BTreeMap<String, RegisterGroupItem>,
pub registers: BTreeMap<String, Register>,
}

impl RegisterGroupHeader {
pub fn is_union(&self) -> bool {
self.class.as_deref() == Some("union")
}
}

#[derive(Debug, Clone)]
pub struct RegisterGroupItem {
pub name: String,
pub name_in_module: Option<String>,
pub description: Option<String>,
pub size: Option<usize>,
pub offset: Option<usize>,
pub count: Option<usize>,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

#[derive(Debug, Clone)]
pub struct Register {
pub name: String,
Expand Down
6 changes: 6 additions & 0 deletions src/svd/chip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ pub fn generate(c: &chip::Chip) -> crate::Result<svd_rs::Device> {
}

fn has_registers(peripheral: &&chip::Peripheral) -> bool {
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);
Expand Down
Loading