Merge pull request #78 from hyg-taylor/fix-negative-factor

Fix bug with unsigned types and negative factors

Author: Pascal Hertleif

.github/workflows/ci.yml

@@ -58,7 +58,7 @@ jobs:
       - name: Install Rust targets we use for embedded
         run: rustup target install thumbv7em-none-eabihf
       - name: Build for embedded
-        run: cargo build -p can-embedded --target=thumbv7em-none-eabihf
+        run: cargo build -p can-embedded --target=thumbv7em-none-eabihf --no-default-features
 
       - name: Install clippy
         run: rustup component add clippy

Cargo.lock

@@ -30,6 +30,7 @@ use anyhow::{anyhow, ensure, Context, Result};
 use can_dbc::{Message, MultiplexIndicator, Signal, ValDescription, ValueDescription, DBC};
 use heck::{ToPascalCase, ToSnakeCase};
 use pad::PadAdapter;
+use std::cmp::{max, min};
 use std::{
     collections::{BTreeMap, BTreeSet},
     fmt::Display,
@@ -261,7 +262,7 @@ fn render_message(mut w: impl Write, config: &Config<'_>, msg: &Message, dbc: &D
         let mut w = PadAdapter::wrap(&mut w);
         config
             .impl_serde
-            .fmt_attr(&mut w, "serde(with = \"serde_bytes\")");
+            .fmt_attr(&mut w, "serde(with = \"serde_bytes\")")?;
         writeln!(w, "raw: [u8; {}],", msg.message_size())?;
     }
     writeln!(w, "}}")?;
@@ -1033,59 +1034,127 @@ fn write_enum(
 ///
 /// NOTE: Factor and offset must be whole integers.
 fn scaled_signal_to_rust_int(signal: &Signal) -> String {
-    let sign = match signal.value_type() {
-        can_dbc::ValueType::Signed => "i",
-        can_dbc::ValueType::Unsigned => "u",
-    };
-
     assert!(
         signal.factor.fract().abs() <= f64::EPSILON,
         "Signal Factor ({}) should be an integer",
         signal.factor,
     );
     assert!(
         signal.offset.fract().abs() <= f64::EPSILON,
-        "Signal Factor ({}) should be an integer",
+        "Signal Offset ({}) should be an integer",
         signal.offset,
     );
 
-    // calculate the maximum possible signal value, accounting for factor and offset
-
-    if signal.min >= 0.0 {
-        let factor = signal.factor as u64;
-        let offset = signal.offset as u64;
-        let max_value = 1u64
-            .checked_shl(*signal.signal_size() as u32)
-            .map(|n| n.saturating_sub(1))
-            .and_then(|n| n.checked_mul(factor))
-            .and_then(|n| n.checked_add(offset))
-            .unwrap_or(u64::MAX);
-
-        let size = match max_value {
-            n if n <= u8::MAX.into() => "8",
-            n if n <= u16::MAX.into() => "16",
-            n if n <= u32::MAX.into() => "32",
-            _ => "64",
+    let err = format!(
+        "Signal {} could not be represented as a Rust integer",
+        &signal.name()
+    );
+    signal_params_to_rust_int(
+        *signal.value_type(),
+        signal.signal_size as u32,
+        signal.factor as i64,
+        signal.offset as i64,
+    )
+    .expect(&err)
+}
+
+/// Convert the relevant parameters of a `can_dbc::Signal` into a Rust type.
+fn signal_params_to_rust_int(
+    sign: can_dbc::ValueType,
+    signal_size: u32,
+    factor: i64,
+    offset: i64,
+) -> Option<String> {
+    if signal_size > 64 {
+        return None;
+    }
+    let range = get_range_of_values(sign, signal_size, factor, offset);
+    match range {
+        Some((low, high)) => Some(range_to_rust_int(low, high)),
+        _ => None,
+    }
+}
+
+/// Using the signal's parameters, find the range of values that it spans.
+fn get_range_of_values(
+    sign: can_dbc::ValueType,
+    signal_size: u32,
+    factor: i64,
+    offset: i64,
+) -> Option<(i128, i128)> {
+    if signal_size == 0 {
+        return None;
+    }
+    let low;
+    let high;
+    match sign {
+        can_dbc::ValueType::Signed => {
+            low = 1i128
+                .checked_shl(signal_size.saturating_sub(1))
+                .and_then(|n| n.checked_mul(-1));
+            high = 1i128
+                .checked_shl(signal_size.saturating_sub(1))
+                .and_then(|n| n.checked_sub(1));
+        }
+        can_dbc::ValueType::Unsigned => {
+            low = Some(0);
+            high = 1i128
+                .checked_shl(signal_size)
+                .and_then(|n| n.checked_sub(1));
+        }
+    }
+    let range1 = apply_factor_and_offset(low, factor, offset);
+    let range2 = apply_factor_and_offset(high, factor, offset);
+    match (range1, range2) {
+        (Some(a), Some(b)) => Some((min(a, b), max(a, b))),
+        _ => None,
+    }
+}
+
+fn apply_factor_and_offset(input: Option<i128>, factor: i64, offset: i64) -> Option<i128> {
+    input
+        .and_then(|n| n.checked_mul(factor.into()))
+        .and_then(|n| n.checked_add(offset.into()))
+}
+
+/// Determine the smallest Rust integer type that can fit the range of values
+/// Only values derived from 64 bit integers are supported, i.e. the range [-2^64-1, 2^64-1]
+fn range_to_rust_int(low: i128, high: i128) -> String {
+    let lower_bound: u8;
+    let upper_bound: u8;
+    let sign: &str;
+
+    if low < 0 {
+        // signed case
+        sign = "i";
+        lower_bound = match low {
+            n if n >= i8::MIN.into() => 8,
+            n if n >= i16::MIN.into() => 16,
+            n if n >= i32::MIN.into() => 32,
+            n if n >= i64::MIN.into() => 64,
+            _ => 128,
+        };
+        upper_bound = match high {
+            n if n <= i8::MAX.into() => 8,
+            n if n <= i16::MAX.into() => 16,
+            n if n <= i32::MAX.into() => 32,
+            n if n <= i64::MAX.into() => 64,
+            _ => 128,
         };
-        format!("{sign}{size}")
     } else {
-        let factor = signal.factor as i64;
-        let offset = signal.offset as i64;
-        let max_value = 1i64
-            .checked_shl(*signal.signal_size() as u32)
-            .map(|n| n.saturating_sub(1))
-            .and_then(|n| n.checked_mul(factor))
-            .and_then(|n| n.checked_add(offset))
-            .unwrap_or(i64::MAX);
-
-        let size = match max_value {
-            n if n <= i8::MAX.into() => "8",
-            n if n <= i16::MAX.into() => "16",
-            n if n <= i32::MAX.into() => "32",
-            _ => "64",
+        sign = "u";
+        lower_bound = 8;
+        upper_bound = match high {
+            n if n <= u8::MAX.into() => 8,
+            n if n <= u16::MAX.into() => 16,
+            n if n <= u32::MAX.into() => 32,
+            n if n <= u64::MAX.into() => 64,
+            _ => 128,
         };
-        format!("i{size}")
     }
+
+    let size = max(lower_bound, upper_bound);
+    format!("{sign}{size}")
 }
 
 /// Determine the smallest rust integer that can fit the raw signal values.
@@ -1507,3 +1576,59 @@ impl FeatureConfig<'_> {
         f(&mut w)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::{get_range_of_values, range_to_rust_int, signal_params_to_rust_int};
+    use can_dbc::ValueType::{Signed, Unsigned};
+
+    #[test]
+    fn test_range_of_values() {
+        assert_eq!(get_range_of_values(Unsigned, 4, 1, 0), Some((0, 15)));
+        assert_eq!(
+            get_range_of_values(Unsigned, 32, -1, 0),
+            Some((-(u32::MAX as i128), 0))
+        );
+        assert_eq!(
+            get_range_of_values(Unsigned, 12, 1, -1000),
+            Some((-1000, 3095))
+        );
+    }
+
+    #[test]
+    fn test_range_0_signal_size() {
+        assert_eq!(
+            get_range_of_values(Signed, 0, 1, 0),
+            None,
+            "0 bit signal should be invalid"
+        );
+    }
+
+    #[test]
+    fn test_range_to_rust_int() {
+        assert_eq!(range_to_rust_int(0, 255), "u8");
+        assert_eq!(range_to_rust_int(-1, 127), "i8");
+        assert_eq!(range_to_rust_int(-1, 128), "i16");
+        assert_eq!(range_to_rust_int(-1, 255), "i16");
+        assert_eq!(range_to_rust_int(-65535, 0), "i32");
+        assert_eq!(range_to_rust_int(-129, -127), "i16");
+        assert_eq!(range_to_rust_int(0, 1i128 << 65), "u128");
+        assert_eq!(range_to_rust_int(-(1i128 << 65), 0), "i128");
+    }
+
+    #[test]
+    fn test_convert_signal_params_to_rust_int() {
+        assert_eq!(signal_params_to_rust_int(Signed, 8, 1, 0).unwrap(), "i8");
+        assert_eq!(signal_params_to_rust_int(Signed, 8, 2, 0).unwrap(), "i16");
+        assert_eq!(signal_params_to_rust_int(Signed, 63, 1, 0).unwrap(), "i64");
+        assert_eq!(
+            signal_params_to_rust_int(Unsigned, 64, -1, 0).unwrap(),
+            "i128"
+        );
+        assert_eq!(
+            signal_params_to_rust_int(Unsigned, 65, 1, 0),
+            None,
+            "This shouldn't be valid in a DBC, it's more than 64 bits"
+        );
+    }
+}

src/lib.rs

@@ -4,5 +4,16 @@ version = "0.1.0"
 authors = ["Pascal Hertleif <[email protected]>"]
 edition = "2021"
 
+[features]
+default = ["build-messages"]
+build-messages = ["dep:can-messages"]
+
 [dependencies]
 bitvec = { version = "1.0", default-features = false }
+
+
+# This is optional and default so we can turn it off for the embedded target.
+# Then it doesn't pull in std.
+[dependencies.can-messages]
+path = "../can-messages"
+optional = true

testing/can-embedded/Cargo.toml

@@ -0,0 +1 @@
+// This stub will be replaced by testing/can-messages/build.rs

testing/can-embedded/src/messages.rs

@@ -12,6 +12,7 @@ fn main() -> Result<()> {
     let mut out = BufWriter::new(File::create(out_file)?);
     println!("cargo:rerun-if-changed=../dbc-examples/example.dbc");
     println!("cargo:rerun-if-changed=../../src");
+    println!("cargo:rerun-if-changed=../can-embedded/src");
 
     let config = Config::builder()
         .dbc_name("example.dbc")
@@ -34,5 +35,7 @@ fn main() -> Result<()> {
         .output()
         .expect("failed to execute rustfmt");
 
+    fs::copy("src/messages.rs", "../can-embedded/src/messages.rs")?;
+
     Ok(())
 }