From 215804facc43623fd8bb9170c05032e7f8540025 Mon Sep 17 00:00:00 2001 From: Eric Seppanen Date: Sat, 13 Dec 2025 13:56:54 -0800 Subject: add unit test for calculate_pio_clock_divider The test will fail, because the current implementation doesn't calculate the fractional part. --- embassy-rp/src/pio_programs/clock_divider.rs | 30 +++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/embassy-rp/src/pio_programs/clock_divider.rs b/embassy-rp/src/pio_programs/clock_divider.rs index 02e353f53..687fd53ba 100644 --- a/embassy-rp/src/pio_programs/clock_divider.rs +++ b/embassy-rp/src/pio_programs/clock_divider.rs @@ -16,10 +16,38 @@ use crate::clocks::clk_sys_freq; /// A fixed-point divider value suitable for use in a PIO state machine configuration #[inline] pub fn calculate_pio_clock_divider(target_hz: u32) -> fixed::FixedU32 { + calculate_pio_clock_divider_inner(clk_sys_freq(), target_hz) +} + +#[inline] +fn calculate_pio_clock_divider_inner(sys_freq: u32, target_hz: u32) -> fixed::FixedU32 { // Requires a non-zero frequency assert!(target_hz > 0, "PIO clock frequency cannot be zero"); // Calculate the divider - let divider = (clk_sys_freq() + target_hz / 2) / target_hz; + let divider = (sys_freq + target_hz / 2) / target_hz; divider.to_fixed() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn clock_divider_math() { + // A simple divider that must have a fractional part. + let divider = calculate_pio_clock_divider_inner(125_000_000, 40_000_000); + let expected: fixed::FixedU32 = 3.125.to_fixed(); + assert_eq!(divider, expected); + + // A system clk so high it would overflow a u32 if shifted left. + let divider = calculate_pio_clock_divider_inner(2_000_000_000, 40_000); + let expected: fixed::FixedU32 = 50000.to_fixed(); + assert_eq!(divider, expected); + + // A divider that requires all 8 fractional bits. + let divider = calculate_pio_clock_divider_inner(134_283_264, 16_777_216); + let expected: fixed::FixedU32 = 8.00390625.to_fixed(); + assert_eq!(divider, expected); + } +} -- cgit From a47f745c2504805feba646c6971ed35266782403 Mon Sep 17 00:00:00 2001 From: Eric Seppanen Date: Sat, 13 Dec 2025 14:16:02 -0800 Subject: improve pio clock divider math Ensure that the fractional part of the clock divider is accurately calculated. This does additional u32 division/mod operations, which I think is better than using u64 for its extra precision. Also: - Add additional asserts to catch out-of-bounds results. - Make the version that accepts the system clock as a parameter const and public, so if anyone wants to hardcode the system frequency they can avoid any runtime computation. - Remove the `inline` attributes because the function has grown quite a bit. --- embassy-rp/src/pio_programs/clock_divider.rs | 43 +++++++++++++++++++++------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/embassy-rp/src/pio_programs/clock_divider.rs b/embassy-rp/src/pio_programs/clock_divider.rs index 687fd53ba..28c931ed2 100644 --- a/embassy-rp/src/pio_programs/clock_divider.rs +++ b/embassy-rp/src/pio_programs/clock_divider.rs @@ -1,6 +1,5 @@ //! Helper functions for calculating PIO clock dividers -use fixed::traits::ToFixed; use fixed::types::extra::U8; use crate::clocks::clk_sys_freq; @@ -14,39 +13,61 @@ use crate::clocks::clk_sys_freq; /// # Returns /// /// A fixed-point divider value suitable for use in a PIO state machine configuration -#[inline] pub fn calculate_pio_clock_divider(target_hz: u32) -> fixed::FixedU32 { - calculate_pio_clock_divider_inner(clk_sys_freq(), target_hz) + calculate_pio_clock_divider_value(clk_sys_freq(), target_hz) } -#[inline] -fn calculate_pio_clock_divider_inner(sys_freq: u32, target_hz: u32) -> fixed::FixedU32 { +/// Calculate a PIO clock divider value based on the desired target frequency. +/// +/// # Arguments +/// +/// * `sys_hz` - The system clock frequency in Hz +/// * `target_hz` - The desired PIO clock frequency in Hz +/// +/// # Returns +/// +/// A fixed-point divider value suitable for use in a PIO state machine configuration +pub const fn calculate_pio_clock_divider_value(sys_hz: u32, target_hz: u32) -> fixed::FixedU32 { // Requires a non-zero frequency assert!(target_hz > 0, "PIO clock frequency cannot be zero"); - // Calculate the divider - let divider = (sys_freq + target_hz / 2) / target_hz; - divider.to_fixed() + // Compute the integer and fractional part of the divider. + // Doing it this way allows us to avoid u64 division while + // maintaining precision. + let integer = sys_hz / target_hz; + let remainder = sys_hz % target_hz; + let frac = (remainder << 8) / target_hz; + + let result = integer << 8 | frac; + + // Ensure the result will fit in 16+8 bits. + assert!(result <= 0xffff_ff, "pio clock divider too big"); + // The clock divider can't be used to go faster than the system clock. + assert!(result >= 0x0001_00, "pio clock divider cannot be less than 1"); + + fixed::FixedU32::from_bits(result) } #[cfg(test)] mod tests { + use fixed::traits::ToFixed; + use super::*; #[test] fn clock_divider_math() { // A simple divider that must have a fractional part. - let divider = calculate_pio_clock_divider_inner(125_000_000, 40_000_000); + let divider = calculate_pio_clock_divider_value(125_000_000, 40_000_000); let expected: fixed::FixedU32 = 3.125.to_fixed(); assert_eq!(divider, expected); // A system clk so high it would overflow a u32 if shifted left. - let divider = calculate_pio_clock_divider_inner(2_000_000_000, 40_000); + let divider = calculate_pio_clock_divider_value(2_000_000_000, 40_000); let expected: fixed::FixedU32 = 50000.to_fixed(); assert_eq!(divider, expected); // A divider that requires all 8 fractional bits. - let divider = calculate_pio_clock_divider_inner(134_283_264, 16_777_216); + let divider = calculate_pio_clock_divider_value(134_283_264, 16_777_216); let expected: fixed::FixedU32 = 8.00390625.to_fixed(); assert_eq!(divider, expected); } -- cgit From 7227c7bde24b9ebd1e3fcc355f708690d1724469 Mon Sep 17 00:00:00 2001 From: Eric Seppanen Date: Sat, 13 Dec 2025 15:18:39 -0800 Subject: clock_divider: use core::assert in const fn defmt::assert gives a compile error in const context. --- embassy-rp/src/pio_programs/clock_divider.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/embassy-rp/src/pio_programs/clock_divider.rs b/embassy-rp/src/pio_programs/clock_divider.rs index 28c931ed2..87ba93008 100644 --- a/embassy-rp/src/pio_programs/clock_divider.rs +++ b/embassy-rp/src/pio_programs/clock_divider.rs @@ -29,7 +29,7 @@ pub fn calculate_pio_clock_divider(target_hz: u32) -> fixed::FixedU32 { /// A fixed-point divider value suitable for use in a PIO state machine configuration pub const fn calculate_pio_clock_divider_value(sys_hz: u32, target_hz: u32) -> fixed::FixedU32 { // Requires a non-zero frequency - assert!(target_hz > 0, "PIO clock frequency cannot be zero"); + core::assert!(target_hz > 0); // Compute the integer and fractional part of the divider. // Doing it this way allows us to avoid u64 division while @@ -41,9 +41,9 @@ pub const fn calculate_pio_clock_divider_value(sys_hz: u32, target_hz: u32) -> f let result = integer << 8 | frac; // Ensure the result will fit in 16+8 bits. - assert!(result <= 0xffff_ff, "pio clock divider too big"); + core::assert!(result <= 0xffff_ff); // The clock divider can't be used to go faster than the system clock. - assert!(result >= 0x0001_00, "pio clock divider cannot be less than 1"); + core::assert!(result >= 0x0001_00); fixed::FixedU32::from_bits(result) } -- cgit