Improvement: Set LedcTimerDriver frequency

[TysonNoodles]

Hi there,

This proposed minor addition gives the ability to set the frequency of an LedcTimerDriver after it has been created by calling the esp-idf function ledc_set_freq. I've found it quite helpful for a quick and dirty PWM controller for motor drivers and such.

I wasn't sure what to do about the frequency of the TimerConfig object since the LedcTimerDriver seems to be where you control the timer but I am open to suggestions 😊. It has been tested on an ESP32-S3

Please let me know if you need anything else before reviewing the request.

[TysonNoodles]

Apologies I see the CIs failed for formatting issues because I didn't run cargo fmt. I have run it now and made a new commit.

[ivmarkov]

I think we can merge this as it is a very small extension, and changing the timer frequency while the driver is already configured might be a valid use case - I simply don't know as I don't use that driver myself.

As to what to do with the frequency configuration of the timer driver initially - perhaps leave it as it is, as there is no harm?

Do you plan to do other changes (the PR is in draft mode), or do you feel it is ready for merging?

[TysonNoodles]

No I had no plans to add other changes, I just wanted to make sure the maintainers were comfortable with the driver config conflict I mentioned initially. I wonder if it would be more correct declaring timer drivers and LEDC drivers like so:

    let peripherals = Peripherals::take().unwrap();
    let pins = peripherals.pins;

    let mut timer = LedcTimerDriver::new(
        peripherals.ledc.timer0,
        &TimerConfig::default().frequency(1.kHz().into()
    ))?;

    let mut driver = LedcDriver::new(peripherals.ledc.channel0, &timer, pins.gpio13)?;

    let max_duty = driver.get_duty();
    driver.set_duty(max_duty / 2)?;
    timer.set_frequency(2.kHz().into())?;

This way there is no TimerConfig floating around with the wrong frequency. Anyways that's a different discussion. I will mark the pull request as ready for review. Thanks for the feedback. Much appreciated.

[ivmarkov]

This way there is no TimerConfig floating around with the wrong frequency.

Sorry for maybe (over)iterating over this, but I'm really trying to understand your concern and I'm failing to. Where in the current code there is a TimerConfig floating around? In fact, I'm also struggling to understand the difference between your snippet from above, and how currently the timer driver and the ledc driver are configured. Can you elaborate further?

[TysonNoodles]

Doh! I'm very sorry, I've re-examined the example code and can find no discernible differences as you said. I could've sworn it was set up like:

    let timer_config = TimerConfig::default().frequency(1.kHz().into());
    let mut timer = LedcTimerDriver::new(
        peripherals.ledc.timer0,
        &timer_config,
    )?;

    let mut driver = LedcDriver::new(peripherals.ledc.channel0, &timer, pins.gpio13)?;

Which would leave a TimerConfig object floating. In any case consider my concerns withdrawn. Apologies once again for creating unnecessary confusion.

[ivmarkov]

I'm merging, yet another last iteration: both what you suggest and what currently is the status quo do take the config by reference rather than moving it into the driver. So yeah, once you change the config, it will have no effect on the driver post construction. If that's your concern, it kinda does exist, I'm just not sure if that's such a big deal?

[TysonNoodles]

I understand. I've seen examples that do it both ways, and I now believe you're right now that I've thought about it some more it really doesn't matter either way. Thanks for the explanation and thanks for accepting the merge. I'm glad to make a small contribution to the project and I hope I can make more in the future.