Refactor Register (#13)

`Register`:

* Reduce field visibility
* Remove unused fields

keelhaul/src/codegen.rs

@@ -307,7 +307,7 @@ impl TestCases {
             let test_fn = test_gen.gen_test_fn().to_string();
             let test_def = test_gen.gen_test_def().to_string();
             test_fns_and_defs_by_periph
-                .entry(register.path.periph().name.clone())
+                .entry(register.top_container_name())
                 .or_default()
                 .push((test_fn, test_def));
         }

keelhaul/src/codegen/test_register.rs

@@ -16,13 +16,13 @@ pub trait TestRegister<P> {
 
     /// A human-readable unique identifier for the register, usually the the path that is used to
     /// access the register.
-    fn uid(&self) -> String {
+    fn binding_id(&self) -> String {
         self.path().join("_")
     }
 
-    /// Name of the top-level element containing this register, usually the first element of the
-    /// `path`
-    fn periph_name(&self) -> String {
+    /// Name of the top-level element containing this register, usually the peripheral or subsystem
+    /// depending on system architecture
+    fn top_container_name(&self) -> String {
         self.path().first().unwrap().to_owned()
     }
 
@@ -31,7 +31,7 @@ pub trait TestRegister<P> {
         self.path().last().unwrap().to_owned()
     }
 
-    /// The address of the register
+    /// Get the absolute memory address of the register
     fn addr(&self) -> P;
 
     /// The size of the register in bits
@@ -188,7 +188,7 @@ impl<'r, 'c, P: ArchPtr + quote::IdentFragment> RegTestGenerator<'r, 'c, P> {
                 reg.reset_value()
                     .map(|value_on_reset| {
                         gen_read_is_reset_val_test(
-                            reg.uid(),
+                            reg.binding_id(),
                             reg.addr(),
                             reg.size(),
                             &value_on_reset,
@@ -225,10 +225,11 @@ impl<'r, 'c, P: ArchPtr + quote::IdentFragment> RegTestGenerator<'r, 'c, P> {
     /// ```
     pub fn gen_test_def(&self) -> TokenStream {
         let fn_name = self.gen_test_fn_ident();
-        let periph_name_lc: TokenStream = self.0.periph_name().to_lowercase().parse().unwrap();
+        let periph_name_lc: TokenStream =
+            self.0.top_container_name().to_lowercase().parse().unwrap();
         let func = quote!(#periph_name_lc::#fn_name);
         let addr_hex: TokenStream = format!("{:#x}", self.0.addr()).parse().unwrap();
-        let uid = self.0.uid();
+        let uid = self.0.binding_id();
 
         quote! {
             TestCase {

keelhaul/src/frontend/svd_legacy.rs

@@ -8,10 +8,8 @@ use std::{
 use crate::{
     bit_count_to_rust_uint_type_str,
     error::{self, Error, PositionalError, SvdParseError},
-    model::ArchPtr,
-    model::PtrSize,
-    model::{self, AddrRepr, RegPath, RegValue, Register, Registers, ResetValue},
-    util, Filters, IsAllowedOrBlocked, ItemFilter,
+    model::{self, AddrRepr, ArchPtr, PtrSize, RegPath, RegValue, Register, Registers, ResetValue},
+    util, Filters, IsAllowedOrBlocked, ItemFilter, TestRegister,
 };
 use itertools::Itertools;
 use log::{info, warn};
@@ -193,8 +191,6 @@ struct RegPropGroupBuilder {
     pub size: Option<u32>,
     /// Register access rights.
     pub access: Option<svd::Access>,
-    /// Register access privileges.
-    pub protection: Option<svd::Protection>,
     /// Register value after reset.
     /// Actual reset value is calculated using reset value and reset mask.
     pub(crate) reset_value: Option<RegValue>,
@@ -285,12 +281,6 @@ impl RegPropGroupBuilder {
         })? {
             self.access = Some(access);
         };
-        if let Some(protection) = process_prop_from_node_if_present("protection", node, |s| {
-            svd::Protection::parse_str(s)
-                .ok_or_else(|| error::SvdParseError::InvalidProtectionType(s.to_owned()))
-        })? {
-            self.protection = Some(protection);
-        };
         if let Some(reset_value) = process_prop_from_node_if_present("resetValue", node, |s| {
             parse_nonneg_int(s).map(RegValue::U64)
         })? {
@@ -319,7 +309,6 @@ impl RegPropGroupBuilder {
             warn!("property 'access' is not defined for register '{reg_path}' or any of its parents, assuming access = read-write");
             svd::Access::ReadWrite
         });
-        let protection = self.protection;
         let reset_value = {
             // Unwrap: `size` was validated on construction using `PtrSize::is_valid_bit_count`
             let size = PtrSize::from_bit_count(size).unwrap();
@@ -334,12 +323,7 @@ impl RegPropGroupBuilder {
             ResetValue::with_mask(reset_value, reset_mask)?
         };
 
-        Ok(RegisterPropertiesGroup::new(
-            size,
-            access,
-            protection,
-            reset_value,
-        ))
+        Ok(RegisterPropertiesGroup::new(size, access, reset_value))
     }
 }
 
@@ -349,8 +333,6 @@ pub struct RegisterPropertiesGroup {
     pub size: u32,
     /// Register access rights.
     pub access: svd::Access,
-    /// Register access privileges.
-    pub protection: Option<svd::Protection>,
     /// Expected register value after reset based on source format
     ///
     /// Checking for the value may require special considerations in registers
@@ -360,16 +342,10 @@ pub struct RegisterPropertiesGroup {
 }
 
 impl RegisterPropertiesGroup {
-    pub(crate) const fn new(
-        size: u32,
-        access: svd::Access,
-        protection: Option<svd::Protection>,
-        reset_value: ResetValue,
-    ) -> Self {
+    pub(crate) const fn new(size: u32, access: svd::Access, reset_value: ResetValue) -> Self {
         Self {
             size,
             access,
-            protection,
             reset_value,
         }
     }
@@ -665,15 +641,13 @@ where
                     .build(&reg_path, Some(P::ptr_size().bit_count()))
                     .map_err(|e| err_with_pos(e, &register_node))?
             };
-            let register = Register {
+            let register = Register::new(
                 path,
                 addr,
-                dimensions: Some(dimensions.clone()),
-                size: properties.size,
-                access: properties.access,
-                protection: properties.protection,
-                reset_value: properties.reset_value,
-            };
+                properties.size,
+                properties.access,
+                properties.reset_value,
+            );
             registers.push(register);
         }
     } else {
@@ -705,15 +679,13 @@ where
                 .build(&reg_path, Some(P::ptr_size().bit_count()))
                 .map_err(|e| err_with_pos(e, &register_node))?
         };
-        let register = Register {
+        let register = Register::new(
             path,
             addr,
-            dimensions,
-            size: properties.size,
-            access: properties.access,
-            protection: properties.protection,
-            reset_value: properties.reset_value,
-        };
+            properties.size,
+            properties.access,
+            properties.reset_value,
+        );
         registers.push(register);
     }
     Ok(Some(registers))
@@ -856,20 +828,20 @@ where
     <P as TryFrom<u64>>::Error: std::fmt::Debug,
 {
     let root = parsed.root().into_xml_node();
-    let registers = process_root(root, filters)?;
+    let registers = process_root::<P>(root, filters)?;
     let mut peripherals = HashSet::new();
     let mut addresses = HashMap::new();
     for register in &registers {
-        peripherals.insert(register.path.periph().name.clone());
-        let addr: P = register.full_addr().unwrap();
+        peripherals.insert(register.top_container_name());
+        let addr: P = register.addr();
         if let Entry::Vacant(entry) = addresses.entry(addr) {
-            entry.insert(register.path.join("-"));
+            entry.insert(register.path().join("-"));
         } else {
             let reg2 = addresses
                 .get(&addr)
                 .expect("failed to find register name by key");
             warn!("Address for register {reg}@{addr:#x?} is already registered for another register {reg2}. {reg} is ignored.",
-                reg = register.path.join("-"));
+                reg = register.path().join("-"));
         }
     }
 

keelhaul/src/model/register.rs

@@ -20,46 +20,50 @@ pub struct Register<P: num::CheckedAdd, S: RefSchema> {
     /// Hierarchical path to this register, e.g. `PERIPH-CLUSTER-REG` in CMSIS-SVD 1.2 and prior
     ///
     /// Used for generating unique identifiers and symbol names in test cases
-    pub path: RegPath<S>,
-    /// Physical address of the register
-    pub addr: AddrRepr<P, S>,
+    path: RegPath<S>,
+    /// Address of the register
+    addr: AddrRepr<P, S>,
     /// Bit-width of register
-    pub size: u32,
+    size: u32,
     /// Software access rights
     ///
     /// Used for determining what types of tests can be generated.
-    pub access: svd::Access,
-    /// Security privilege required to access register
-    pub protection: Option<svd::Protection>,
+    access: svd::Access,
     /// Expected register value after reset based on source format
     ///
     /// Checking for the value may require special considerations in registers
     /// with read-only or write-only fields. These considerations are encoded in
     /// [ResetValue].
-    pub(crate) reset_value: ResetValue,
-    pub dimensions: Option<svd::DimElement>,
+    reset_value: ResetValue,
+    // TODO: consider support for array-like registers in input
+    //dimensions: Option<svd::DimElement>,
 }
 
 impl<P, S> Register<P, S>
 where
     P: num::CheckedAdd + Copy,
     S: RefSchema,
 {
-    /// Get register's absolute memory address
-    ///
-    /// # Errors
-    ///
-    /// Address overflows
-    pub fn full_addr(&self) -> Result<P, error::AddrOverflowError<P, S>> {
-        self.addr
-            .full()
-            .ok_or_else(|| error::AddrOverflowError::new(self.path.join("-"), self.addr.clone()))
+    pub(crate) fn new(
+        path: RegPath<S>,
+        addr: AddrRepr<P, S>,
+        size: u32,
+        access: svd::Access,
+        reset_value: ResetValue,
+    ) -> Self {
+        Self {
+            path,
+            addr,
+            size,
+            access,
+            reset_value,
+        }
     }
 
-    /// Get register's unique identifier
+    /// Get register's unique path identifier
     ///
     /// Constructed from the hierarchical path, e.g., PERIPH-CLUSTER-REG.
-    pub fn uid(&self) -> String {
+    pub fn path_id(&self) -> String {
         self.path.join("-")
     }
 
@@ -99,8 +103,15 @@ where
             .collect_vec()
     }
 
+    /// Get the absolute memory address of the register
+    ///
+    /// # Panics
+    ///
+    /// * address overflows
     fn addr(&self) -> P {
-        self.full_addr()
+        self.addr
+            .full()
+            .ok_or_else(|| error::AddrOverflowError::new(self.path.join("-"), self.addr.clone()))
             .unwrap_or_else(|_| panic!("could not resolve full address for register: {:?}", &self))
     }
 
@@ -160,20 +171,6 @@ impl RegPath<RefSchemaSvdV1_2> {
         v.push(RegPathSegment { name: reg });
         Self::new(v)
     }
-
-    pub fn periph(&self) -> &RegPathSegment {
-        unsafe { self.0.get_unchecked(0) }
-    }
-
-    pub fn reg(&self) -> &RegPathSegment {
-        if self.0.len() == 2 {
-            unsafe { self.0.get_unchecked(1) }
-        } else if self.0.len() == 3 {
-            unsafe { self.0.get_unchecked(2) }
-        } else {
-            panic!("register in the CMSIS-SVD 1.2 schema must comprise of at least two elements (periph + offset)")
-        }
-    }
 }
 
 /// Address representation