Merge pull request #30 from lord-ne/rerewrite

Rewrite: Prevent overflow from public functions

Hopefully are the underlaying commits preserved.
Yes, I do fear that "github.com" does `git merge --squash`.

What "lord-ne" wrote:

This is the rewrite discussed in #17. Its main purpose is to ensure that public functions cannot overflow, by having them delegate to private functions that accept larger parameters. Some work was also put in to mitigating the effects of the delays not being inlined at compile time.

Additional a link to #22 are is being asked for good commit messages.

Author: stappersg

src/delay_impl.rs

@@ -0,0 +1,52 @@
+#[allow(unused_imports)]
+use core::arch::asm;
+
+/// Internal function to implement a variable busy-wait loop.
+/// # Arguments
+/// * 'count' - an u32, the number of times to cycle the loop (4 clock cycles per loop).
+#[inline(always)]
+pub fn delay_count_32(count: u32) {
+    let mut outer_count: u16 = (count >> 16) as u16;
+    let inner_count: u16 = count as u16;
+    if inner_count != 0 {
+        delay_loop_4_cycles(inner_count);
+    }
+    while outer_count != 0 {
+        delay_loop_4_cycles(0);
+        outer_count -= 1;
+    }
+}
+
+/// Internal function to implement a variable busy-wait loop.
+/// # Arguments
+/// * 'count' - an u64, the number of times to cycle the loop (4 clock cycles per loop). *The top 16 bits are ignored.*
+#[inline(always)]
+pub fn delay_count_48(count: u64) {
+    let mut outer_count: u32 = (count >> 16) as u32;
+    let inner_count: u16 = count as u16;
+    if inner_count != 0 {
+        delay_loop_4_cycles(inner_count);
+    }
+    while outer_count != 0 {
+        delay_loop_4_cycles(0);
+        outer_count -= 1;
+    }
+}
+
+/// Internal function to implement a 16-bit busy-wait loop in assembly.
+/// Delays for 4 cycles per iteration, not including setup overhead.
+/// Up to 2^16 iterations (the value 2^16 would have to be passed as 0).
+/// # Arguments
+/// * 'cycles' - an u16, the number of times to cycle the loop.
+#[inline(always)]
+#[allow(unused_variables, unused_mut, unused_assignments, dead_code)]
+pub fn delay_loop_4_cycles(mut cycles: u16) {
+    #[cfg(target_arch = "avr")]
+    unsafe {
+        asm!("1: sbiw {i}, 1",
+            "brne 1b",
+            i = inout(reg_iw) cycles => _,
+        )
+    }
+    // Allow compilation even on non-avr targets, for testing purposes
+}

src/lib.rs

@@ -2,7 +2,7 @@
 #![no_std]
 #![crate_name = "avr_delay"]
 
-use core::arch::asm;
+mod delay_impl;
 
 /// This library is intended to provide a busy-wait delay
 /// similar to the one provided by the arduino c++ utilities
@@ -12,60 +12,47 @@ use core::arch::asm;
 // This library does all of the busy-wait loop in rust.
 // We pack as much of the looping as possible into asm!
 // so that we can count cycles.
-//
-// Ignoring the overhead, which may be significant:
-// An arduino runs at 16MHZ. Each asm loop is 4 cycles.
-// so each loop is 0.25 us.
-//
-// the overhead of delay() seems to be about 13 cycles
-// initially, and then 11 cycles per outer loop. We ignore
-// all that.
 
-/// Internal function to implement a variable busy-wait loop.
+/// Internal function to implement a variable busy-wait loop. Even if count isn't
+/// known at compile time, this function shouldn't have too much overhead.
 /// # Arguments
-/// * 'count' - a u64, the number of times to cycle the loop.
+/// * 'count' - an u32, the number of times to cycle the loop.
 #[inline(always)]
-pub fn delay(count: u64) {
-    // Our asm busy-wait takes a 16 bit word as an argument,
-    // so the max number of loops is 2^16
-    let outer_count = count / 65536;
-    let last_count = ((count % 65536) + 1) as u16;
-    for _ in 0..outer_count {
-        // Each loop through should be 4 cycles.
-        let zero = 0u16;
-        unsafe {
-            asm!("1: sbiw {i}, 1",
-                 "brne 1b",
-                 i = inout(reg_iw) zero => _,
-            )
-        }
-    }
-    unsafe {
-        asm!("1: sbiw {i}, 1",
-             "brne 1b",
-             i = inout(reg_iw) last_count => _,
-        )
-    }
+pub fn delay(count: u32) {
+    delay_impl::delay_count_32(count);
 }
 
 ///delay for N milliseconds
 /// # Arguments
-/// * 'ms' - an u64, number of milliseconds to busy-wait
+/// * 'ms' - an u32, number of milliseconds to busy-wait. This should be known at
+/// compile time, otherwise the delay may be much longer than specified.
 #[inline(always)]
-pub fn delay_ms(ms: u64) {
-    // microseconds
-    let us = ms * 1000;
-    delay_us(us);
+pub fn delay_ms(ms: u32) {
+    const GCD: u32 = gcd(avr_config::CPU_FREQUENCY_HZ, 4_000);
+    const NUMERATOR: u32 = avr_config::CPU_FREQUENCY_HZ / GCD;
+    const DENOMINATOR: u32 = 4_000 / GCD;
+    let ticks: u64 = (u64::from(ms) * u64::from(NUMERATOR)) / u64::from(DENOMINATOR);
+    delay_impl::delay_count_48(ticks);
 }
 
 ///delay for N microseconds
 /// # Arguments
-/// * 'us' - an u64, number of microseconds to busy-wait
+/// * 'ms' - an u32, number of microseconds to busy-wait. This should be known at
+/// compile time, otherwise the delay may be much longer than specified.
 #[inline(always)]
-pub fn delay_us(us: u64) {
-    let us_in_loop = (avr_config::CPU_FREQUENCY_HZ / 1000000 / 4) as u64;
-    let loops = us * us_in_loop;
-    delay(loops);
+pub fn delay_us(us: u32) {
+    const GCD: u32 = gcd(avr_config::CPU_FREQUENCY_HZ, 4_000_000);
+    const NUMERATOR: u32 = avr_config::CPU_FREQUENCY_HZ / GCD;
+    const DENOMINATOR: u32 = 4_000_000 / GCD;
+    let ticks: u64 = (u64::from(us) * u64::from(NUMERATOR)) / u64::from(DENOMINATOR);
+    delay_impl::delay_count_48(ticks);
+}
+
+const fn gcd(mut a: u32, mut b: u32) -> u32 {
+    while b != 0 {
+        (a, b) = (b, a % b);
+    }
+    return a;
 }
 
 #[cfg(test)]