From b0ae142196aa4d3b5ed6853982538ea85fc31c66 Mon Sep 17 00:00:00 2001 From: Yan Fitterer Date: Sat, 18 Jul 2026 17:37:53 -0400 Subject: [PATCH 1/3] fix(ledc): compute ESP32-H2 low-speed divisor from the selected source clock On the ESP32-H2, set_global_slow_clock(APBClk) writes ledc_sclk_sel = 0, which selects XTAL_CLK (32 MHz) per ESP-IDF's esp32h2 ledc_ll.h, but ls_freq_hw reported apb_clk_frequency() (96 MHz on the default preset), so the timer divisor came out 3x too large and the output frequency was 1/3 of the requested value. Verified on ESP32-H2 hardware driving an RC servo at 50 Hz. Report the XTAL frequency on the H2 instead; all other chips are unchanged. Co-Authored-By: Claude Fable 5 --- esp-hal/src/ledc/low_level/v3.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/esp-hal/src/ledc/low_level/v3.rs b/esp-hal/src/ledc/low_level/v3.rs index 50917742858..03f2af40896 100644 --- a/esp-hal/src/ledc/low_level/v3.rs +++ b/esp-hal/src/ledc/low_level/v3.rs @@ -22,6 +22,14 @@ pub(super) fn set_global_slow_clock(ledc: &RegisterBlock, clock_source: LSGlobal } pub(super) fn ls_freq_hw(_clock_source: LSClockSource) -> Rate { + // On the ESP32-H2, `set_global_slow_clock` selects `ledc_sclk_sel = 0`, which is + // XTAL_CLK (see `ledc_ll_set_slow_clk_sel` in ESP-IDF's + // `components/hal/esp32h2/include/hal/ledc_ll.h`), so the divisor must be computed + // from the XTAL frequency. Using `apb_clk_frequency()` (96 MHz on the default + // preset) produced output at 1/3 of the requested frequency. + #[cfg(esp32h2)] + return Rate::from_hz(clocks::xtal_clk_frequency()); + #[cfg(not(esp32h2))] Rate::from_hz(clocks::apb_clk_frequency()) } From 71299e432dd554fdb5f16e4526eb070bb3b4515c Mon Sep 17 00:00:00 2001 From: Yan Fitterer Date: Tue, 21 Jul 2026 20:50:27 -0400 Subject: [PATCH 2/3] test(ledc): HIL test asserting output frequency matches configuration Adds hil-test/src/bin/ledc.rs, gated on ledc_driver_supported so it runs on every LEDC-capable chip. It splits one common test pin into an input/output pair (internal loopback, no wiring), drives PWM on the output half, and measures the real signal period on the input half via the hardware timer. This is the check the divisor fix needs: the frequency() getter returns the *requested* value, so only measuring the achieved output frequency can catch a wrong-source-clock divisor. On the unfixed ESP32-H2 the output ran at 1/3 the configured rate (32 MHz XTAL source vs 96 MHz APB divisor), which blows the 20% period tolerance by ~10x. Two frequencies (2 kHz and 500 Hz) also catch any regression where the output stops tracking the configured rate. Builds for esp32h2/esp32c6/esp32c3. intended for the esp30h2-usb runner. Co-Authored-By: Claude Fable 5 --- hil-test/Cargo.toml | 4 + hil-test/src/bin/ledc.rs | 172 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 hil-test/src/bin/ledc.rs diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml index 122bc62a9f0..8a765bf1b13 100644 --- a/hil-test/Cargo.toml +++ b/hil-test/Cargo.toml @@ -41,6 +41,10 @@ harness = false name = "i2s" harness = false +[[bin]] +name = "ledc" +harness = false + [[bin]] name = "interrupt" harness = false diff --git a/hil-test/src/bin/ledc.rs b/hil-test/src/bin/ledc.rs new file mode 100644 index 00000000000..00d188c43ac --- /dev/null +++ b/hil-test/src/bin/ledc.rs @@ -0,0 +1,172 @@ +//! LEDC output-frequency HIL test. +//! +//! Verifies that a LowSpeed LEDC timer produces PWM at the frequency it was +//! configured for, by measuring the real signal period on hardware. +//! +//! This guards the timer-divisor computation, which derives the divisor from the +//! *source clock* the LEDC slow-clock mux actually selects. If the driver models +//! the wrong source frequency, the divisor is miscalculated and the output comes +//! out at a fixed ratio of the requested rate - a mistake that a purely +//! register-level or `frequency()`-getter check cannot catch, because the getter +//! returns the *requested* value, not the achieved one. Concretely, on the +//! ESP32-H2 `set_global_slow_clock(APBClk)` selects the 32 MHz XTAL while the +//! divisor was briefly computed from the 96 MHz "APB" figure, so every timer ran +//! at 1/3 of its configured frequency (see PR #5941). +//! +//! No external wiring: one `common_test_pins!` pin is split into an input and an +//! output driver, LEDC drives the output half and the input half samples it. + +//% CHIP_FILTER: ledc_driver_supported +//% FEATURES: unstable + +#![no_std] +#![no_main] + +use esp_hal::{ + delay::Delay, + gpio::{AnyPin, DriveMode, Flex, Input, Pin}, + ledc::{ + channel::{self, ChannelIFace}, + timer::{self, TimerIFace}, + LSGlobalClkSource, Ledc, LowSpeed, + }, + peripherals::LEDC, + time::{Instant, Rate}, +}; +#[allow(unused_imports)] +use hil_test::{assert, assert_eq}; + +/// Rising edges to average the period over. More edges tighten the estimate; 64 +/// edges is ~32 ms at 2 kHz, comfortably inside the default test timeout. +const PERIOD_SAMPLE_EDGES: usize = 64; + +/// Allowed deviation of the measured period from the configured one. GPIO-polled +/// edge timing carries some jitter, so the bound is generous; the bug this test +/// exists for shifts the frequency by ~3x (a ~200% error), an order of magnitude +/// past this tolerance. +const PERIOD_TOLERANCE_PERCENT: u32 = 20; + +/// Measure the average PWM period (in microseconds) over `edges` rising edges of +/// `input`, using the hardware timer for timestamps (not a loop counter, so it is +/// insensitive to polling-loop speed). +/// +/// A stuck (never-toggling) output never reaches `edges` and surfaces as a test +/// timeout rather than a wrong value - still a failure, which is the point. +fn average_period_us(input: &Input<'_>, edges: usize) -> u32 { + assert!(edges > 1); + + let mut prev = input.is_high(); + + // Align to the first rising edge so the first measured interval is a full period. + loop { + let now = input.is_high(); + if !prev && now { + break; + } + prev = now; + core::hint::spin_loop(); + } + + let first_edge = Instant::now(); + let mut last_edge = first_edge; + let mut seen_edges = 1usize; + prev = true; + + while seen_edges < edges { + let now = input.is_high(); + if !prev && now { + seen_edges += 1; + last_edge = Instant::now(); + } + prev = now; + core::hint::spin_loop(); + } + + let total_us = (last_edge - first_edge).as_micros() as u32; + total_us / (edges as u32 - 1) +} + +/// Configure a LowSpeed timer+channel at `frequency` (50% duty) on `test_pin` and +/// assert the measured output period matches, within `PERIOD_TOLERANCE_PERCENT`. +fn assert_output_frequency(ledc: LEDC<'static>, test_pin: AnyPin<'static>, delay: Delay, frequency: Rate) { + let pin = Flex::new(test_pin); + // SAFETY: the output half is driven only by LEDC and the input half is only sampled. + let (input, output) = unsafe { pin.split_into_drivers() }; + + let mut ledc = Ledc::new(ledc); + ledc.set_global_slow_clock(LSGlobalClkSource::APBClk); + + let mut timer0 = ledc.timer::(timer::Number::Timer0); + timer0 + .configure(timer::config::Config { + duty: timer::config::Duty::Duty8Bit, + clock_source: timer::LSClockSource::APBClk, + frequency, + }) + .unwrap(); + + let mut channel0 = ledc.channel(channel::Number::Channel0, output); + channel0 + .configure(channel::config::Config { + timer: &timer0, + duty_pct: 50, + drive_mode: DriveMode::PushPull, + }) + .unwrap(); + + // Let the output settle before sampling. + delay.delay_millis(4); + + let measured_us = average_period_us(&input, PERIOD_SAMPLE_EDGES); + + let expected_us = 1_000_000u32 / frequency.as_hz(); + let min_us = expected_us * (100 - PERIOD_TOLERANCE_PERCENT) / 100; + let max_us = expected_us * (100 + PERIOD_TOLERANCE_PERCENT) / 100; + + assert!( + measured_us >= min_us && measured_us <= max_us, + "measured period {} us at {} Hz, expected {} us (+/- {}%)", + measured_us, + frequency.as_hz(), + expected_us, + PERIOD_TOLERANCE_PERCENT + ); +} + +struct Context { + ledc: LEDC<'static>, + test_pin: AnyPin<'static>, + delay: Delay, +} + +#[embedded_test::tests(default_timeout = 3, executor = hil_test::Executor::new())] +mod tests { + use super::*; + + #[init] + fn init() -> Context { + let peripherals = esp_hal::init(esp_hal::Config::default()); + let (_, test_pin) = hil_test::common_test_pins!(peripherals); + + Context { + ledc: peripherals.LEDC, + test_pin: test_pin.degrade(), + delay: Delay::new(), + } + } + + /// The core regression guard: a mid-range frequency must appear at the output. + /// Off-by-source-clock divisor bugs make this a fixed ratio too high or low. + #[test] + fn output_frequency_matches_configuration_2khz(ctx: Context) { + assert_output_frequency(ctx.ledc, ctx.test_pin, ctx.delay, Rate::from_khz(2)); + } + + /// A second, 4x-lower frequency: since a wrong source clock scales *every* + /// frequency by the same factor, this both re-confirms the fix and catches a + /// regression where the output ignores the configured rate altogether. + #[test] + fn output_frequency_matches_configuration_500hz(ctx: Context) { + assert_output_frequency(ctx.ledc, ctx.test_pin, ctx.delay, Rate::from_hz(500)); + } +} From a087444984a82266e7e02869a5af3ddbf89157ef Mon Sep 17 00:00:00 2001 From: Yan Fitterer Date: Thu, 23 Jul 2026 00:38:16 -0400 Subject: [PATCH 3/3] cargo fmt changes --- hil-test/src/bin/ledc.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/hil-test/src/bin/ledc.rs b/hil-test/src/bin/ledc.rs index 00d188c43ac..3bd27bf6b69 100644 --- a/hil-test/src/bin/ledc.rs +++ b/hil-test/src/bin/ledc.rs @@ -26,9 +26,11 @@ use esp_hal::{ delay::Delay, gpio::{AnyPin, DriveMode, Flex, Input, Pin}, ledc::{ + LSGlobalClkSource, + Ledc, + LowSpeed, channel::{self, ChannelIFace}, timer::{self, TimerIFace}, - LSGlobalClkSource, Ledc, LowSpeed, }, peripherals::LEDC, time::{Instant, Rate}, @@ -88,7 +90,12 @@ fn average_period_us(input: &Input<'_>, edges: usize) -> u32 { /// Configure a LowSpeed timer+channel at `frequency` (50% duty) on `test_pin` and /// assert the measured output period matches, within `PERIOD_TOLERANCE_PERCENT`. -fn assert_output_frequency(ledc: LEDC<'static>, test_pin: AnyPin<'static>, delay: Delay, frequency: Rate) { +fn assert_output_frequency( + ledc: LEDC<'static>, + test_pin: AnyPin<'static>, + delay: Delay, + frequency: Rate, +) { let pin = Flex::new(test_pin); // SAFETY: the output half is driven only by LEDC and the input half is only sampled. let (input, output) = unsafe { pin.split_into_drivers() };