ESP32C3: set_duty_hw() on ledc::Channel doesn't work

[IsaacDynamo]

After creation and configuration of a ledc::Channel, as done in the example, the duty-cycle is not be updated via set_duty() or set_duty_hw().

This is because modification of some register in the LEDC peripheral, are buffered and are only applied when LEDC_PARA_UP_CHn is set.

The HAL contains a macro that does this, update_channel!(), but it is currently not public.
Instead of making it public it could also called from within set_duty() and set_duty_hw(). I think this would a bit more user friendly, and less surprising.

But after setting LEDC_PARA_UP_CHn manually, the duty-cycle still didn't update. By referencing the esp-idf source code and reconstructing their register access, I found that setting the LEDC_DUTY_START_CHn is also required. Although this is as far as I could see in the technical reference manual, not mentioned to be needed for normal PWM mode. LEDC_DUTY_START_CHn is mentioned for the more advanced fading options. My guess is that the hardware implement regular PWM just as fading with step size of zero. An therefor still needs the LEDC_DUTY_START_CHn trigger to start the "fading" action, which updates the duty-cycle.

So currently the following extra steps need to be done to updated the duty-cycle:

channel0.set_duty(80);
let raw_ledc = unsafe { &*hal::pac::LEDC::ptr() };
raw_ledc.ch0_conf1.modify(|_, w| w.duty_start().set_bit());
raw_ledc.ch0_conf0.modify(|_, w| w.para_up().set_bit());

I can work on a PR that fixes the issue for the ESP32-C3 but I don't have the hardware to test the other ESP32 variants. Let me known if/how I can help.

Here are some other usability improvements suggestions for the LEDC:

• Allow set_duty(0) instead of returning Error::Duty. When precision is required set_duty_hw() should be used instead IMHO.
• The addition of a trait without generic type parameter for the channels, so something like this becomes easier:

impl<'a, R, G, B> Rgb<'a, R, G, B>
where
    R: AnyChannel,
    G: AnyChannel,
    B: AnyChannel
{
    pub fn new(r: &'a R, g: &'a G, b: &'a B) -> Self {
        Self { r, g, b }
    }
}

The embedded_hal::PwmPin trait might be a good option as well.

[bjoernQ]

Great find! For sure a PR would be very welcome. We also have plans to rewrite this driver since it's currently not really compile time safe - just didn't get to it

Also implementing embedded_hal::PwmPin is a great suggestion

No problem if you can't verify it on other chips than ESP32-C3 - we can (and will) test it on all supported chips. Would be nice if it at least compiles for all targets

[IsaacDynamo]

The embedded_hal::PwmPin documentation that I found was apparently outdated, and currently there is no standardized trait. See embedded-hal #357.