Generate enum-typed setters and constructors for signals with value descriptions (#178)

- Updates the generated `new()` constructors and `set_<signal>()`
setters to accept enums for signals that have an associated enum type
(value description in the DBC). This mirrors the existing enum-typed
getters, making the getter and setter APIs symmetrical.
- Cleans up the formatting of some generated getter/setter function
comments for readability and consistency.

This is a breaking change for DBCs that have signals with value
descriptions, but should make these signals much cleaner to handle. The
best approach is to directly pass the enum-typed value into
`set_<signal>()` and `new()`. Raw or undescribed values can still be set
through the enum's `_Other` variant, e.g.
`set_<signal>(SignalEnum::_Other(value))`.

This PR should be stacked on top of
#176.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: felixvanoost

fuzz/fuzz_targets/fuzz_target_1.rs

@@ -8,7 +8,14 @@ use cantools_messages::{
 };
 
 fuzz_target!(|dbc_codegen_bar: can_messages::Bar| {
-    let dbc_codegen_bar = can_messages::Bar::new(3, 2.0, 4, 5, false).unwrap();
+    let dbc_codegen_bar = can_messages::Bar::new(
+        3,
+        2.0,
+        can_messages::BarThree::_Other(4),
+        can_messages::BarFour::_Other(5),
+        can_messages::BarType::X0off,
+    )
+    .unwrap();
 
     println!(
         "{} {} {} {} {}",

src/lib.rs

@@ -372,14 +372,14 @@ impl Config<'_> {
 
             self.render_attribute_structs(&mut w, msg, dbc)?;
 
-            writeln!(w, "/// Construct new {} from values", msg.name)?;
+            writeln!(w, "/// Construct new '{}' from values", msg.name)?;
             let args = msg
                 .signals
                 .iter()
                 .filter_map(|signal| {
                     if matches!(signal.multiplexer_indicator, Plain | Multiplexor) {
                         let field = signal.field_name();
-                        let typ = ValType::from_signal(signal);
+                        let typ = signal_pub_type(dbc, msg, signal);
                         Some(format!("{field}: {typ}"))
                     } else {
                         None
@@ -426,7 +426,7 @@ impl Config<'_> {
                         .render_signal(&mut w, signal, dbc, msg)
                         .with_context(|| format!("write signal impl `{}`", signal.name))?,
                     Multiplexor => {
-                        self.render_multiplexor_signal(&mut w, signal, msg)?;
+                        self.render_multiplexor_signal(&mut w, signal, dbc, msg)?;
                     }
                     MultiplexedSignal(_) | MultiplexorAndMultiplexedSignal(_) => {}
                 }
@@ -470,7 +470,7 @@ impl Config<'_> {
         self.impl_defmt
             .fmt_cfg(&mut *w, |w| render_defmt_impl(w, msg))?;
         self.impl_arbitrary
-            .fmt_cfg(&mut *w, |w| self.render_arbitrary(w, msg))?;
+            .fmt_cfg(&mut *w, |w| self.render_arbitrary(w, dbc, msg))?;
 
         let enums_for_this_message = dbc.value_descriptions.iter().filter_map(|x| {
             if let ValueDescription::Signal {
@@ -685,7 +685,7 @@ impl Config<'_> {
         dbc: &Dbc,
         msg: &Message,
     ) -> Result<()> {
-        writeln!(w, "/// {}", signal.name)?;
+        writeln!(w, "/// Get value of '{}'", signal.name)?;
         if let Some(comment) = dbc.signal_comment(msg.id, &signal.name) {
             writeln!(w, "///")?;
             for line in comment.trim().lines() {
@@ -756,7 +756,7 @@ impl Config<'_> {
             writeln!(w)?;
         }
 
-        writeln!(w, "/// Get raw value of {}", signal.name)?;
+        writeln!(w, "/// Get raw value of '{}'", signal.name)?;
         writeln!(w, "///")?;
         writeln!(w, "/// - Start bit: {}", signal.start_bit)?;
         writeln!(w, "/// - Signal size: {} bits", signal.size)?;
@@ -774,15 +774,18 @@ impl Config<'_> {
         writeln!(w, "}}")?;
         writeln!(w)?;
 
-        self.render_set_signal(w, signal, msg)?;
+        self.render_set_signal(w, signal, dbc, msg)?;
 
         Ok(())
     }
 
-    fn render_set_signal(&self, w: &mut impl Write, signal: &Signal, msg: &Message) -> Result<()> {
-        writeln!(w, "/// Set value of {}", signal.name)?;
-        writeln!(w, "#[inline(always)]")?;
-
+    fn render_set_signal(
+        &self,
+        w: &mut impl Write,
+        signal: &Signal,
+        dbc: &Dbc,
+        msg: &Message,
+    ) -> Result<()> {
         // To avoid accidentally changing the multiplexor value without changing
         // the signals accordingly this fn is kept private for multiplexors.
         let visibility = if signal.multiplexer_indicator == Multiplexor {
@@ -793,42 +796,60 @@ impl Config<'_> {
 
         let field = signal.field_name();
         let typ = ValType::from_signal(signal);
+        let param_type = signal_pub_type(dbc, msg, signal);
+        let is_enum_backed = param_type != typ.to_string();
+
+        writeln!(w, "/// Set value of '{}'", signal.name)?;
+        writeln!(w, "#[inline(always)]")?;
         writeln!(
             w,
-            "{visibility}fn set_{field}(&mut self, value: {typ}) -> Result<(), CanError> {{",
+            "{visibility}fn set_{field}(&mut self, value: {param_type}) -> Result<(), CanError> {{",
         )?;
-
         {
             let mut w = PadAdapter::wrap(w);
+            // Enum-backed signals accept the value-description enum; convert it to
+            // the raw primitive before range checks and packing.
+            if is_enum_backed {
+                writeln!(w, "let value = {typ}::from(value);")?;
+            }
+            self.render_set_signal_body(&mut w, signal, msg)?;
+        }
+        writeln!(w, "}}")?;
+        writeln!(w)?;
 
-            if signal.size != 1 {
-                if let FeatureConfig::Gated(gate) = self.check_ranges {
-                    writeln!(w, r"#[cfg(feature = {gate:?})]")?;
-                }
+        Ok(())
+    }
 
-                if let FeatureConfig::Gated(..) | FeatureConfig::Always = self.check_ranges {
-                    let typ = ValType::from_signal(signal);
-                    let min = signal.min;
-                    let max = signal.max;
-                    writeln!(w, r"if value < {min}_{typ} || {max}_{typ} < value {{")?;
+    fn render_set_signal_body(
+        &self,
+        w: &mut impl Write,
+        signal: &Signal,
+        msg: &Message,
+    ) -> Result<()> {
+        if signal.size != 1 {
+            if let FeatureConfig::Gated(gate) = self.check_ranges {
+                writeln!(w, r"#[cfg(feature = {gate:?})]")?;
+            }
 
-                    {
-                        let mut w = PadAdapter::wrap(&mut w);
-                        let typ = msg.type_name();
-                        writeln!(
-                            w,
-                            r"return Err(CanError::ParameterOutOfRange {{ message_id: {typ}::MESSAGE_ID }});",
-                        )?;
-                    }
+            if let FeatureConfig::Gated(..) | FeatureConfig::Always = self.check_ranges {
+                let typ = ValType::from_signal(signal);
+                let min = signal.min;
+                let max = signal.max;
+                writeln!(w, r"if value < {min}_{typ} || {max}_{typ} < value {{")?;
 
-                    writeln!(w, r"}}")?;
+                {
+                    let mut w = PadAdapter::wrap(&mut *w);
+                    let typ = msg.type_name();
+                    writeln!(
+                        w,
+                        r"return Err(CanError::ParameterOutOfRange {{ message_id: {typ}::MESSAGE_ID }});",
+                    )?;
                 }
+
+                writeln!(w, r"}}")?;
             }
-            signal_to_payload(&mut w, signal, msg).context("signal to payload")?;
         }
-
-        writeln!(w, "}}")?;
-        writeln!(w)?;
+        signal_to_payload(&mut *w, signal, msg).context("signal to payload")?;
 
         Ok(())
     }
@@ -837,9 +858,10 @@ impl Config<'_> {
         &self,
         w: &mut impl Write,
         signal: &Signal,
+        dbc: &Dbc,
         msg: &Message,
     ) -> Result<()> {
-        writeln!(w, "/// Get raw value of {}", signal.name)?;
+        writeln!(w, "/// Get raw value of '{}'", signal.name)?;
         writeln!(w, "///")?;
         writeln!(w, "/// - Start bit: {}", signal.start_bit)?;
         writeln!(w, "/// - Signal size: {} bits", signal.size)?;
@@ -906,7 +928,7 @@ impl Config<'_> {
         }
         writeln!(w, "}}")?;
 
-        self.render_set_signal(w, signal, msg)?;
+        self.render_set_signal(w, signal, dbc, msg)?;
 
         for switch_index in multiplexer_indexes {
             render_set_signal_multiplexer(w, signal, msg, switch_index)?;
@@ -999,7 +1021,7 @@ fn render_set_signal_multiplexer(
     msg: &Message,
     switch_index: u64,
 ) -> Result<()> {
-    writeln!(w, "/// Set value of {}", multiplexor.name)?;
+    writeln!(w, "/// Set value of '{}'", multiplexor.name)?;
     writeln!(w, "#[inline(always)]")?;
     writeln!(
         w,
@@ -1070,6 +1092,21 @@ fn le_start_end_bit(signal: &Signal, msg: &Message) -> Result<(u64, u64)> {
     Ok((start_bit, end_bit))
 }
 
+/// Public type of a signal as seen by `new()` and `set_*`. This is the
+/// enum when the signal has one (and isn't the multiplexor selector), otherwise
+/// it is the raw primitive type.
+fn signal_pub_type(dbc: &Dbc, msg: &Message, signal: &Signal) -> String {
+    if signal.multiplexer_indicator != Multiplexor
+        && dbc
+            .value_descriptions_for_signal(msg.id, &signal.name)
+            .is_some()
+    {
+        enum_name(msg, signal)
+    } else {
+        ValType::from_signal(signal).to_string()
+    }
+}
+
 fn signal_from_payload(w: &mut impl Write, signal: &Signal, msg: &Message) -> Result<()> {
     writeln!(w, r"let signal = {};", read_fn(signal, msg)?)?;
     writeln!(w)?;
@@ -1450,7 +1487,7 @@ impl Config<'_> {
         Ok(())
     }
 
-    fn render_arbitrary(&self, w: &mut impl Write, msg: &Message) -> Result<()> {
+    fn render_arbitrary(&self, w: &mut impl Write, dbc: &Dbc, msg: &Message) -> Result<()> {
         writeln!(w, "{ALLOW_LINTS}")?;
         self.write_allow_dead_code(w)?;
         let typ = msg.type_name();
@@ -1481,7 +1518,16 @@ impl Config<'_> {
 
                 let args: Vec<String> = filtered_signals
                     .iter()
-                    .map(|signal| signal.field_name())
+                    .map(|signal| {
+                        let field = signal.field_name();
+                        let is_enum_backed = signal_pub_type(dbc, msg, signal)
+                            != ValType::from_signal(signal).to_string();
+                        if is_enum_backed {
+                            format!("{}::_Other({field})", enum_name(msg, signal))
+                        } else {
+                            field
+                        }
+                    })
                     .collect();
 
                 writeln!(

testing/can-messages/tests/all.rs

@@ -5,15 +5,15 @@
 )]
 
 use can_messages::{
-    Amet, Bar, BarThree, CanError, Foo, LargerIntsWithOffsets, MsgExtendedId, MultiplexTest,
-    MultiplexTestMultiplexorIndex, MultiplexTestMultiplexorM0, NegativeFactorTest,
+    Amet, Bar, BarFour, BarThree, BarType, CanError, Foo, LargerIntsWithOffsets, MsgExtendedId,
+    MultiplexTest, MultiplexTestMultiplexorIndex, MultiplexTestMultiplexorM0, NegativeFactorTest,
     TruncatedBeSignal, TruncatedLeSignal,
 };
 use embedded_can::{ExtendedId, Id, StandardId};
 
 #[test]
 fn check_range_value_error() {
-    let result = Bar::new(1, 2.0, 3, 4, true);
+    let result = Bar::new(1, 2.0, BarThree::Onest, BarFour::_Other(4), BarType::X1on);
     assert_eq!(
         result.unwrap_err(),
         CanError::ParameterOutOfRange {
@@ -24,7 +24,7 @@ fn check_range_value_error() {
 
 #[test]
 fn check_range_value_valid() {
-    let result = Bar::new(1, 2.0, 3, 3, true);
+    let result = Bar::new(1, 2.0, BarThree::Onest, BarFour::Onest, BarType::X1on);
     assert!(result.is_ok());
 }
 
@@ -141,14 +141,14 @@ fn offset_integers() {
 
 #[test]
 fn debug_impl() {
-    let result = Bar::new(1, 2.0, 3, 3, true).unwrap();
+    let result = Bar::new(1, 2.0, BarThree::Onest, BarFour::Onest, BarType::X1on).unwrap();
     let dbg = format!("{result:?}");
     assert_eq!(&dbg, "Bar([5, 94, 0, 64, 0, 0, 0, 0])");
 }
 
 #[test]
 fn debug_alternative_impl() {
-    let result = Bar::new(1, 2.0, 3, 3, true).unwrap();
+    let result = Bar::new(1, 2.0, BarThree::Onest, BarFour::Onest, BarType::X1on).unwrap();
     let dbg = format!("{result:#?}");
     assert_eq!(
         &dbg,
@@ -162,6 +162,27 @@ fn from_enum_into_raw() {
     assert_eq!(raw, 3);
 }
 
+#[test]
+fn enum_setters_and_getters_are_symmetrical() {
+    let mut bar = Bar::new(1, 2.0, BarThree::Onest, BarFour::Onest, BarType::X1on).unwrap();
+
+    // The setter accepts the enum directly.
+    bar.set_three(BarThree::Oner).unwrap();
+    assert_eq!(bar.three(), BarThree::Oner);
+
+    // Raw / undescribed values remain settable via the `_Other` variant.
+    bar.set_three(BarThree::_Other(3)).unwrap();
+    assert_eq!(bar.three(), BarThree::Onest);
+
+    // Out-of-range raw values still produce an error.
+    assert_eq!(
+        bar.set_three(BarThree::_Other(8)),
+        Err(CanError::ParameterOutOfRange {
+            message_id: Id::Standard(StandardId::new(512).unwrap())
+        })
+    );
+}
+
 #[test]
 fn negative_factor() {
     assert_eq!(

testing/cantools-messages/src/lib.rs

@@ -16,7 +16,14 @@ use msg_bindings::*;
 
 #[test]
 fn pack_message() {
-    let dbc_codegen_bar = can_messages::Bar::new(3, 2.0, 4, 2, false).unwrap();
+    let dbc_codegen_bar = can_messages::Bar::new(
+        3,
+        2.0,
+        can_messages::BarThree::_Other(4),
+        can_messages::BarFour::Oner,
+        can_messages::BarType::X0off,
+    )
+    .unwrap();
     let one = unsafe { example_bar_one_encode(3.0) };
     let two = unsafe { example_bar_two_encode(2.0) };
     let three = unsafe { example_bar_three_encode(4.0) };
@@ -61,7 +68,8 @@ fn pack_message_signed_positive() {
 
 #[test]
 fn pack_big_endian_signal_with_start_bit_zero() {
-    let dbc_codegen_bar = can_messages::Dolor::new(0.5).unwrap();
+    let dbc_codegen_bar =
+        can_messages::Dolor::new(can_messages::DolorOneFloat::_Other(0.5)).unwrap();
     let one_float = unsafe { example_dolor_one_float_encode(0.5) };
 
     let dolor = example_dolor_t { one_float };