fix(ledc): compute ESP32-H2 low-speed timer divisor from the selected source clock - #5941
fix(ledc): compute ESP32-H2 low-speed timer divisor from the selected source clock#5941yvf wants to merge 5 commits into
Conversation
…e 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 <noreply@anthropic.com>
playfulFence
left a comment
There was a problem hiding this comment.
IMO the fix makes sense and corresponds with what I see in IDF 👍🏼
I'm wondering if we can add some sane form of a HIL test here?
PS: here's something I came up with Claude after one prompt, thing is driving an LEDC LS channel at a known frequency, loops the output pin back into a PCNT unit, counts edges over a known time window, and asserts the measured frequency matches the requested one within a tolerance. Not saying it's perfect, just suggesting 🤷🏼
//! LEDC Tests
//% CHIP_FILTER: ledc_driver_supported && pcnt_driver_supported
//% FEATURES: unstable
#![no_std]
#![no_main]
use hil_test as _;
#[embedded_test::tests(default_timeout = 3)]
mod tests {
use esp_hal::{
delay::Delay,
gpio::{AnyPin, DriveMode, Input, InputConfig, Pin},
ledc::{
LSGlobalClkSource,
Ledc,
LowSpeed,
channel::{self, ChannelIFace},
timer::{self, TimerIFace},
},
pcnt::{Pcnt, channel::EdgeMode},
time::Rate,
};
struct Context<'d> {
ledc: Ledc<'d>,
pcnt: Pcnt<'d>,
pwm_pin: AnyPin<'d>,
sense_pin: AnyPin<'d>,
delay: Delay,
}
#[init]
fn init() -> Context<'static> {
let peripherals = esp_hal::init(esp_hal::Config::default());
// These two pins are wired together on the HIL rig (same jumper used
// by the gpio/pcnt loopback tests).
let (sense, pwm) = hil_test::common_test_pins!(peripherals);
Context {
ledc: Ledc::new(peripherals.LEDC),
pcnt: Pcnt::new(peripherals.PCNT),
pwm_pin: pwm.degrade(),
sense_pin: sense.degrade(),
delay: Delay::new(),
}
}
/// Configures LS Timer0 + Channel0 to `freq` (50% duty), loops the
/// output back into a PCNT unit, counts rising edges over
/// `window_ms`, and returns the measured frequency in Hz.
///
/// This is the kind of check that would have caught
/// https://github.com/esp-rs/esp-hal/pull/5941 (ESP32-H2 LEDC LS output
/// running at 1/3 of the requested frequency because the divisor was
/// computed from the wrong source clock).
fn measure_ledc_frequency(
mut ctx: Context<'static>,
freq: Rate,
duty_bits: timer::config::Duty,
window_ms: u32,
) -> u32 {
ctx.ledc.set_global_slow_clock(LSGlobalClkSource::APBClk);
let mut lstimer0 = ctx.ledc.timer::<LowSpeed>(timer::Number::Timer0);
lstimer0
.configure(timer::config::Config {
duty: duty_bits,
clock_source: timer::LSClockSource::APBClk,
frequency: freq,
})
.unwrap();
let mut channel0 = ctx.ledc.channel(channel::Number::Channel0, ctx.pwm_pin);
channel0
.configure(channel::config::Config {
timer: &lstimer0,
duty_pct: 50,
drive_mode: DriveMode::PushPull,
})
.unwrap();
let unit = ctx.pcnt.unit0;
unit.channel0
.set_edge_signal(Input::new(ctx.sense_pin, InputConfig::default()));
unit.channel0
.set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
unit.clear();
unit.resume();
ctx.delay.delay_millis(window_ms);
unit.pause();
let edges = unit.value().max(0) as u32;
edges * 1000 / window_ms
}
#[test]
fn ledc_ls_frequency_is_accurate_at_50hz(ctx: Context<'static>) {
// Mirrors the exact scenario from the PR #5941 report: LowSpeed
// Timer0, Duty14Bit, 50 Hz servo control signal.
let requested = Rate::from_hz(50);
let measured =
measure_ledc_frequency(ctx, requested, timer::config::Duty::Duty14Bit, 1000);
// Generous tolerance: this is about catching gross,
// wrong-clock-source-class bugs (2x, 3x off), not sub-percent
// accuracy.
let tolerance = requested.as_hz() / 10;
assert!(
measured.abs_diff(requested.as_hz()) <= tolerance,
"requested {} Hz, measured {} Hz",
requested.as_hz(),
measured
);
}
#[test]
fn ledc_ls_frequency_is_accurate_at_2khz(ctx: Context<'static>) {
let requested = Rate::from_hz(2_000);
let measured =
measure_ledc_frequency(ctx, requested, timer::config::Duty::Duty10Bit, 100);
let tolerance = requested.as_hz() / 10;
assert!(
measured.abs_diff(requested.as_hz()) <= tolerance,
"requested {} Hz, measured {} Hz",
requested.as_hz(),
measured
);
}
}
I guess having a way to check the generated clock frequency would also be interesting for other peripherals |
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 <noreply@anthropic.com>
|
Good idea @playfulFence - I (Claude) added a test. Verified locally passing with the fix, and failing without, on a esp32h2. Hopefully CI will validate others, as that's the only model I have right now. Let me know if something should be done differently. |
|
@yvf, Do you mind running |
|
@playfulFence Done. |
| // 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()) |
There was a problem hiding this comment.
Please adhere to the coding guidelines we have. In this case, that would be using cfg_select. There's also no need for any of that comment. The difference will be encoded in the per-device clock tree data in the future.
|
C2 and C6 HIL failures are real ones, they'll need to be figured out before this PR can be merged. |
Description
On ESP32-H2 hardware we observed that LEDC low-speed PWM output runs at 1/3 of the requested frequency. The cause appears to be a mismatch between the clock the driver selects and the frequency it uses for the divisor calculation:
set_global_slow_clock(LSGlobalClkSource::APBClk)writesledc_sclk_sel = 0,which on the H2 selects XTAL_CLK (32 MHz), whilels_freq_hwreportsapb_clk_frequency()(96 MHz on the default preset). This PR makesls_freq_hwreport the XTAL frequency on the H2; no other chip is affected.The fix is LLM generated (Fable), after having implemented local workarounds, and looking at other relevant / related PRs (#4966 and #5937). It is verified working on the hardware.
What we observed, for reference:
components/hal/esp32h2/include/hal/ledc_ll.h(ledc_ll_set_slow_clk_sel) confirms thatledc_sclk_sel = 0selects XTAL on this chip (0 = XTAL, 1 = RC_FAST, 2 = PLL_DIV). So the existing sel = 0 write looks correct and only the frequency used for the divisor seems wrong.LEDC_SCLK_SELas "0 (default): do not select anyone clock, 1: 80MHz, 2: FOSC, 3: XTAL", which does not match either the ESP-IDF mapping or our measurement, so the SVD text might deserve a look by someone with TRM access.Two things we deliberately did not touch, which might be worth a maintainer's opinion:
esp32c6/ledc_ll.h, the sel = 1 the driver writes selects PLL_F80M (80 MHz), whileapb_clk_frequency()computes 40 MHz from the clock tree - which would make C6 output 2x the requested frequency. We do not have C6 hardware to verify, so we left it unchanged.Testing
cargo build --target riscv32imac-unknown-none-elf --features esp32h2,unstableand--features esp32c6,unstableboth build cleanly.Changelog
esp-hal