Skip to content

Ledc rewrite - #5937

Open
HungryMuttis wants to merge 4 commits into
esp-rs:mainfrom
HungryMuttis:ledc-rewrite
Open

Ledc rewrite#5937
HungryMuttis wants to merge 4 commits into
esp-rs:mainfrom
HungryMuttis:ledc-rewrite

Conversation

@HungryMuttis

@HungryMuttis HungryMuttis commented Jul 17, 2026

Copy link
Copy Markdown

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • I have added changelog entries and/or migration guide notes in the sections below, or I will ask a maintainer to add the skip-changelog or manual-changelog label as appropriate.
  • My changes are in accordance to the esp-rs developer guidelines

Extra:

Pull Request Details 📖

Description

This pull request refactors the LEDC driver to be in par with esp-hal developer guidelines.
Key changes:

  • Removed unsafe creation of timers and channels by moving to a creator pattern.
  • Changed percentage based duty cycles to absolute values to allow more precision.
  • Added a HIL test to ensure the reliability of PWM and fade functionalities.

Testing

  • Compilation: Verified all examples and drivers build successfully using cargo xtask lint-packages and cargo xtask fmt-packages.
  • Unit Testing: Verified API consistency and trait implementations.
  • HIL Testing: Added a dedicated LEDC test binary in hil-test to verify PWM signal generation, timer reconfiguration, and automated hardware duty cycle fading.
  • Verification: Manually verified the migration path with the provided example code to ensure the new API is functional and ergonomic.

Changelog

esp-hal

  • Changed: Refactored LEDC driver to use the creator pattern for timers and channels.
  • Changed: Replaced percentage-based LEDC duty cycle configuration with absolute duty values
  • Changed: Renamed LSGlobalClkSource to LowSpeedGlobalClockSource.
  • Changed: Renamed LSClockSource to ClockSource.

Migration guide

esp-hal/LEDC driver

LEDC API refactored to use creator pattern

The LEDC driver API has been restructured. Ledc::new() now returns an instance containing dedicated creator fields for each timer and channel (e.g., timer0, channel0) instead of using .timer() and .channel() methods.

LSGlobalClkSource has been renamed to LowSpeedGlobalClockSource

Replace all uses of LSGlobalClkSource with LowSpeedGlobalClockSource.

LSClockSource and HSClockSource were merged and renamed to ClockSource.

Replace all uses of LSClockSource and HSClockSource with ClockSource.

Timer and Channel creation has been moved to TimerCreator and ChannelCreator.

Change all usages of the .timer() and .channel() methods inside the Ledc driver to use the new creator fields: ledc.timer0.configure() or ledc.channel0.configure().

Output pin is assignment moved to with_pin() method.

Move all pin assignments from the Config struct to with_pin() method on the configured Channel.

Duty Cycles are now absolute u32 values.

Change all set_duty() method usages to set_duty_cycle() and make sure the argument is an absolute value. A simple way to do this would be to change percentage to channel.max_duty_cycle() / percentage * 100.

Example migration:

// Old
let mut ledc = Ledc::new(peripherals.LEDC);
ledc.set_global_slow_clock(ledcLSGlobalClkSource::APBClk);

let timer0 = ledc.timer::<ledc::LowSpeed>(timer::Number::Timer0);
timer0.configure(timer::config::Config {
    duty: timer::config::Duty::Duty8Bit,
    clock_source: timer::LSClockSource::APBClk,
    frequency: Rate::from_khz(2),
});

let mut channel0 = ledc.channel::<LowSpeed>(channel::Number::Channel0, peripherals.GPIO0);
channel0.configure(channel::config::Config {
    timer: &timer0,
    duty_pct: 0,
    drive_mode: DriveMode::PushPull,
})?;

channel0.start_duty_fade(50, 100, 1000)?;

// New
let ledc = Ledc::new(
    peripherals.LEDC,
    ledc::Config::default().with_clock_source(ledc::LowSpeedGlobalClockSource::APBClock),
)?;

let timer0 = ledc.timer0.configure(
    timer::Config::default()
        .with_clock_source(timer::ClockSource::APBClock)
        .with_duty(timer::Duty::Bit8)
        .with_frequency(Rate::from_khz(2)),
)?;

let mut chanel0 = ledc
    .channel0
    .configure(&timer0, channel::Config::default().with_duty(0))?
    .with_pin(peripherals.GPIO0);

let max_duty = chanel0.max_duty_cycle();
chanel0.start_duty_fade(max_duty / 2, max_duty, 1000)?;

@bugadani

bugadani commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

I'm not sure why we should accept this - a lot of change, but it doesn't solve the fundamental useability issues - timers and channels are tied in such a way that makes using LEDC in a real application rather difficult.

Sure, the driver itself is a little better than how it was - but if we're going to change it, we might as well spend the time to actually design the driver into a shape that we consider good enough for the long term.

I'm also not sure this was written by a human, and maintaining something generated by an LLM will be difficult if you won't be around to be able to answer questions about the code in the future.

@HungryMuttis

Copy link
Copy Markdown
Author

The start_duty_fade() method needs the timer frequency. To decouple timers and channels a static array with the timer frequencies could be used. Timers would write while channels only read. Then the channel struct would only need to store the number of the timer.

Regarding the AI comment: I used an LLM to research how other drivers are written. I wrote the code myself, only sometimes asking the LLM for small code snippets which I reviewed and edited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants