Merge pull request #829 from rust-embedded/write_enum

add IsEnum & rm FieldWriterSafe, add set

Author: burrbull

CHANGELOG.md

@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/).
 
 ## [Unreleased]
 
+- Add `IsEnum` constraint for `FieldWriter`s (fix `variant` safety)
+- Make field writer `bits` always `unsafe` add `set` for safe writing
 - Fix bit writer type for `ModifiedWriteValues::ZeroToSet`
 
 ## [v0.32.0] - 2024-02-26

src/generate/generic.rs

@@ -56,6 +56,9 @@ pub trait FieldSpec: Sized {
     type Ux: Copy + PartialEq + From<Self>;
 }
 
+/// Marker for fields with fixed values
+pub trait IsEnum: FieldSpec {}
+
 /// Trait implemented by readable registers to enable the `read` method.
 ///
 /// Registers marked with `Writable` can be also be `modify`'ed.
@@ -474,16 +477,13 @@ pub struct Safe;
 /// You should check that value is allowed to pass to register/field writer marked with this
 pub struct Unsafe;
 
-/// Write field Proxy with unsafe `bits`
-pub type FieldWriter<'a, REG, const WI: u8, FI = u8> = raw::FieldWriter<'a, REG, WI, FI, Unsafe>;
-/// Write field Proxy with safe `bits`
-pub type FieldWriterSafe<'a, REG, const WI: u8, FI = u8> = raw::FieldWriter<'a, REG, WI, FI, Safe>;
+/// Write field Proxy
+pub type FieldWriter<'a, REG, const WI: u8, FI = u8, Safety = Unsafe> = raw::FieldWriter<'a, REG, WI, FI, Safety>;
 
-impl<'a, REG, const WI: u8, FI> FieldWriter<'a, REG, WI, FI>
+impl<'a, REG, const WI: u8, FI, Safety> FieldWriter<'a, REG, WI, FI, Safety>
 where
     REG: Writable + RegisterSpec,
     FI: FieldSpec,
-    REG::Ux: From<FI::Ux>,
 {
     /// Field width
     pub const WIDTH: u8 = WI;
@@ -499,7 +499,14 @@ where
     pub const fn offset(&self) -> u8 {
         self.o
     }
+}
 
+impl<'a, REG, const WI: u8, FI, Safety> FieldWriter<'a, REG, WI, FI, Safety>
+where
+    REG: Writable + RegisterSpec,
+    FI: FieldSpec,
+    REG::Ux: From<FI::Ux>,
+{
     /// Writes raw bits to the field
     ///
     /// # Safety
@@ -511,45 +518,31 @@ where
         self.w.bits |= (REG::Ux::from(value) & REG::Ux::mask::<WI>()) << self.o;
         self.w
     }
-    /// Writes `variant` to the field
-    #[inline(always)]
-    pub fn variant(self, variant: FI) -> &'a mut W<REG> {
-        unsafe { self.bits(FI::Ux::from(variant)) }
-    }
 }
 
-impl<'a, REG, const WI: u8, FI> FieldWriterSafe<'a, REG, WI, FI>
+impl<'a, REG, const WI: u8, FI> FieldWriter<'a, REG, WI, FI, Safe>
 where
     REG: Writable + RegisterSpec,
     FI: FieldSpec,
     REG::Ux: From<FI::Ux>,
 {
-    /// Field width
-    pub const WIDTH: u8 = WI;
-
-    /// Field width
-    #[inline(always)]
-    pub const fn width(&self) -> u8 {
-        WI
-    }
-
-    /// Field offset
-    #[inline(always)]
-    pub const fn offset(&self) -> u8 {
-        self.o
-    }
-
     /// Writes raw bits to the field
     #[inline(always)]
-    pub fn bits(self, value: FI::Ux) -> &'a mut W<REG> {
-        self.w.bits &= !(REG::Ux::mask::<WI>() << self.o);
-        self.w.bits |= (REG::Ux::from(value) & REG::Ux::mask::<WI>()) << self.o;
-        self.w
+    pub fn set(self, value: FI::Ux) -> &'a mut W<REG> {
+        unsafe { self.bits(value) }
     }
+}
+
+impl<'a, REG, const WI: u8, FI, Safety> FieldWriter<'a, REG, WI, FI, Safety>
+where
+    REG: Writable + RegisterSpec,
+    FI: IsEnum,
+    REG::Ux: From<FI::Ux>,
+{
     /// Writes `variant` to the field
     #[inline(always)]
     pub fn variant(self, variant: FI) -> &'a mut W<REG> {
-        self.bits(FI::Ux::from(variant))
+        unsafe { self.bits(FI::Ux::from(variant)) }
     }
 }
 

src/generate/register.rs

@@ -393,17 +393,22 @@ pub fn render_register_mod(
         // * there is a single field that covers the entire register
         // * that field can represent all values
         // * the write constraints of the register allow full range of values
-        let can_write_safe = !unsafety(
+        let safe_ty = if let Safety::Safe = Safety::get(
             register
                 .fields
                 .as_ref()
                 .and_then(|fields| fields.first())
                 .and_then(|field| field.write_constraint)
                 .as_ref(),
             rsize,
-        ) || !unsafety(register.write_constraint.as_ref(), rsize);
-        let safe_ty = if can_write_safe { "Safe" } else { "Unsafe" };
-        let safe_ty = Ident::new(safe_ty, span);
+        ) {
+            Safety::Safe
+        } else if let Safety::Safe = Safety::get(register.write_constraint.as_ref(), rsize) {
+            Safety::Safe
+        } else {
+            Safety::Unsafe
+        };
+        let safe_ty = safe_ty.ident(span);
 
         let doc = format!("`write(|w| ..)` method takes [`{mod_ty}::W`](W) writer structure",);
 
@@ -1097,7 +1102,7 @@ pub fn fields(
         // the read value, or else we reuse read value.
         if can_write {
             let mut proxy_items = TokenStream::new();
-            let mut unsafety = unsafety(f.write_constraint.as_ref(), width);
+            let mut safety = Safety::get(f.write_constraint.as_ref(), width);
 
             // if we writes to enumeratedValues, generate its structure if it differs from read structure.
             let value_write_ty = if let Some(ev) = rwenum.write_enum() {
@@ -1136,12 +1141,12 @@ pub fn fields(
                         if variants.len() == 1 << width {
                         } else if let Some(def) = def.take() {
                             variants.push(def);
-                            unsafety = false;
+                            safety = Safety::Safe;
                         }
 
                         // if the write structure is finite, it can be safely written.
                         if variants.len() == 1 << width {
-                            unsafety = false;
+                            safety = Safety::Safe;
                         }
 
                         // generate write value structure and From conversation if we can't reuse read value structure.
@@ -1235,19 +1240,15 @@ pub fn fields(
                             quote! { crate::#wproxy<'a, REG, #value_write_ty> }
                         }
                     } else {
-                        let wproxy = Ident::new(
-                            if unsafety {
-                                "FieldWriter"
-                            } else {
-                                "FieldWriterSafe"
-                            },
-                            span,
-                        );
+                        let wproxy = Ident::new("FieldWriter", span);
                         let width = &unsuffixed(width);
-                        if value_write_ty == "u8" {
+                        if value_write_ty == "u8" && safety != Safety::Safe {
                             quote! { crate::#wproxy<'a, REG, #width> }
-                        } else {
+                        } else if safety != Safety::Safe {
                             quote! { crate::#wproxy<'a, REG, #width, #value_write_ty> }
+                        } else {
+                            let safe_ty = safety.ident(span);
+                            quote! { crate::#wproxy<'a, REG, #width, #value_write_ty, crate::#safe_ty> }
                         }
                     };
                     mod_items.extend(quote! {
@@ -1370,22 +1371,40 @@ pub fn fields(
     ))
 }
 
-fn unsafety(write_constraint: Option<&WriteConstraint>, width: u32) -> bool {
-    match &write_constraint {
-        Some(&WriteConstraint::Range(range))
-            if range.min == 0 && range.max == u64::MAX >> (64 - width) =>
-        {
-            // the SVD has acknowledged that it's safe to write
-            // any value that can fit in the field
-            false
-        }
-        None if width == 1 => {
-            // the field is one bit wide, so we assume it's legal to write
-            // either value into it or it wouldn't exist; despite that
-            // if a writeConstraint exists then respect it
-            false
+#[derive(Clone, Debug, PartialEq, Eq)]
+enum Safety {
+    Unsafe,
+    Safe,
+}
+
+impl Safety {
+    fn get(write_constraint: Option<&WriteConstraint>, width: u32) -> Self {
+        match &write_constraint {
+            Some(&WriteConstraint::Range(range))
+                if range.min == 0 && range.max == u64::MAX >> (64 - width) =>
+            {
+                // the SVD has acknowledged that it's safe to write
+                // any value that can fit in the field
+                Self::Safe
+            }
+            None if width == 1 => {
+                // the field is one bit wide, so we assume it's legal to write
+                // either value into it or it wouldn't exist; despite that
+                // if a writeConstraint exists then respect it
+                Self::Safe
+            }
+            _ => Self::Unsafe,
         }
-        _ => true,
+    }
+    fn ident(&self, span: Span) -> Ident {
+        Ident::new(
+            if let Self::Safe = self {
+                "Safe"
+            } else {
+                "Unsafe"
+            },
+            span,
+        )
     }
 }
 
@@ -1425,7 +1444,7 @@ impl Variant {
             span,
         );
         let sc = case.sanitize(&ev.name);
-        const INTERNALS: [&str; 4] = ["set_bit", "clear_bit", "bit", "bits"];
+        const INTERNALS: [&str; 6] = ["bit", "bits", "clear_bit", "set", "set_bit", "variant"];
         let sc = Ident::new(
             &(if INTERNALS.contains(&sc.as_ref()) {
                 sc + "_"
@@ -1552,6 +1571,7 @@ fn add_from_variants<'a>(
             impl crate::FieldSpec for #pc {
                 type Ux = #fty;
             }
+            impl crate::IsEnum for #pc {}
         });
     }
 }