Merge pull request #291 from jannic/avoid-64bit-division

Avoid 64bit division

.github/workflows/build_and_test.yml

@@ -48,3 +48,7 @@ jobs:
         with:
           command: test
           args: --doc --target x86_64-unknown-linux-gnu --features chrono
+      - uses: actions-rs/cargo@v1
+        with:
+          command: test
+          args: --tests --target x86_64-unknown-linux-gnu

rp2040-hal/CHANGELOG.md

@@ -13,7 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
-- Updated embedded-hal alpha support to version 1.0.0-alpha.7
+- Update embedded-hal alpha support to version 1.0.0-alpha.7
+- Avoid 64-bit division in clock calculations
 
 ## [0.3.0] - 2021-12-19
 

rp2040-hal/src/clocks/macros.rs

@@ -180,19 +180,13 @@ macro_rules! clock {
 
                 #[doc = "Configure `"$name"`"]
                 fn configure_clock<S: ValidSrc<$name>>(&mut self, src: &S, freq: Hertz) -> Result<(), ClockError>{
-                    let src_freq: Hertz<u64> = src.get_freq().into();
+                    let src_freq: Hertz<u32> = src.get_freq().into();
 
                     if freq.gt(&src_freq){
                         return Err(ClockError::CantIncreaseFreq);
                     }
 
-                    // Div register is 24.8) int.frac divider so multiply by 2^8 (left shift by 8)
+                    let div = fractional_div(src_freq.integer(), freq.integer()).ok_or(ClockError::FrequencyTooLow)?;
-                    let shifted_src_freq = src_freq * (1 << 8);
-                    let div = if freq.eq(&src_freq) {
-                        1 << 8
-                    } else {
-                        (shifted_src_freq / freq.integer() as u64).integer() as u32
-                    };
 
                     // If increasing divisor, set divisor before source. Otherwise set source
                     // before divisor. This avoids a momentary overspeed when e.g. switching
@@ -223,8 +217,7 @@ macro_rules! clock {
                     self.set_div(div);
 
                     // Store the configured frequency
-                    // div contains both the integer part and the fractional part so we need to shift the src_freq equally
+                    self.frequency = fractional_div(src_freq.integer(), div).ok_or(ClockError::FrequencyTooHigh)?.Hz();
-                    self.frequency = (shifted_src_freq / div as u64).try_into().map_err(|_| ClockError::FrequencyToHigh)?;
 
                     Ok(())
                 }
@@ -337,19 +330,13 @@ macro_rules! stoppable_clock {
 
                 #[doc = "Configure `"$name"`"]
                 fn configure_clock<S: ValidSrc<$name>>(&mut self, src: &S, freq: Hertz) -> Result<(), ClockError>{
-                    let src_freq: Hertz<u64> = src.get_freq().into();
+                    let src_freq: Hertz<u32> = src.get_freq().into();
 
                     if freq.gt(&src_freq){
                         return Err(ClockError::CantIncreaseFreq);
                     }
 
-                    // Div register is 24.8) int.frac divider so multiply by 2^8 (left shift by 8)
+                    let div = fractional_div(src_freq.integer(), freq.integer()).ok_or(ClockError::FrequencyTooLow)?;
-                    let shifted_src_freq = src_freq * (1 << 8);
-                    let div = if freq.eq(&src_freq) {
-                        1 << 8
-                    } else {
-                        (shifted_src_freq / freq.integer() as u64).integer() as u32
-                    };
 
                     // If increasing divisor, set divisor before source. Otherwise set source
                     // before divisor. This avoids a momentary overspeed when e.g. switching
@@ -386,7 +373,7 @@ macro_rules! stoppable_clock {
                     self.set_div(div);
 
                     // Store the configured frequency
-                    self.frequency = (shifted_src_freq / div as u64).try_into().map_err(|_| ClockError::FrequencyToHigh)?;
+                    self.frequency = fractional_div(src_freq.integer(), div).ok_or(ClockError::FrequencyTooHigh)?.Hz();
 
                     Ok(())
                 }

rp2040-hal/src/clocks/mod.rs

@@ -70,10 +70,7 @@ use crate::{
     watchdog::Watchdog,
     xosc::{setup_xosc_blocking, CrystalOscillator, Error as XoscError, Stable},
 };
-use core::{
+use core::{convert::Infallible, marker::PhantomData};
-    convert::{Infallible, TryInto},
-    marker::PhantomData,
-};
 use embedded_time::rate::*;
 use pac::{CLOCKS, PLL_SYS, PLL_USB, RESETS, XOSC};
 
@@ -102,11 +99,15 @@ impl ShareableClocks {
 }
 
 /// Something when wrong setting up the clock
+#[non_exhaustive]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 pub enum ClockError {
     /// The frequency desired is higher than the source frequency
     CantIncreaseFreq,
     /// The desired frequency is to high (would overflow an u32)
-    FrequencyToHigh,
+    FrequencyTooHigh,
+    /// The desired frequency is too low (divider can't reach the desired value)
+    FrequencyTooLow,
 }
 
 /// For clocks
@@ -354,3 +355,70 @@ pub fn init_clocks_and_plls(
         .map_err(InitError::ClockError)?;
     Ok(clocks)
 }
+
+// Calculates (numerator<<8)/denominator, avoiding 64bit division
+// Returns None if the result would not fit in 32 bit.
+fn fractional_div(numerator: u32, denominator: u32) -> Option<u32> {
+    if denominator.eq(&numerator) {
+        return Some(1 << 8);
+    }
+
+    let div_int = numerator / denominator;
+    if div_int >= 1 << 24 {
+        return None;
+    }
+
+    let div_rem = numerator - (div_int * denominator);
+
+    let div_frac = if div_rem < 1 << 24 {
+        // div_rem is small enough to shift it by 8 bits without overflow
+        (div_rem << 8) / denominator
+    } else {
+        // div_rem is too large. Shift denominator right, instead.
+        // As 1<<24 < div_rem < denominator, relative error caused by the
+        // lost lower 8 bits of denominator is smaller than 2^-16
+        (div_rem) / (denominator >> 8)
+    };
+
+    Some((div_int << 8) + div_frac)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_fractional_div() {
+        // easy values
+        assert_eq!(fractional_div(1, 1), Some(1 << 8));
+
+        // typical values
+        assert_eq!(fractional_div(125_000_000, 48_000_000), Some(666));
+        assert_eq!(fractional_div(48_000_000, 46875), Some(1024 << 8));
+
+        // resulting frequencies
+        assert_eq!(
+            fractional_div(
+                125_000_000,
+                fractional_div(125_000_000, 48_000_000).unwrap()
+            ),
+            Some(48_048_048)
+        );
+        assert_eq!(
+            fractional_div(48_000_000, fractional_div(48_000_000, 46875).unwrap()),
+            Some(46875)
+        );
+
+        // not allowed in src/clocks/mod.rs, but should still deliver correct results
+        assert_eq!(fractional_div(1, 2), Some(128));
+        assert_eq!(fractional_div(1, 256), Some(1));
+        assert_eq!(fractional_div(1, 257), Some(0));
+
+        // borderline cases
+        assert_eq!(fractional_div((1 << 24) - 1, 1), Some(((1 << 24) - 1) << 8));
+        assert_eq!(fractional_div(1 << 24, 1), None);
+        assert_eq!(fractional_div(1 << 24, 2), Some(1 << (23 + 8)));
+        assert_eq!(fractional_div(1 << 24, (1 << 24) + 1), Some(1 << 8));
+        assert_eq!(fractional_div(u32::MAX, u32::MAX), Some(1 << 8));
+    }
+}