[WIP} introduce overflow interrupt handing for led

[KashunCheng]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request. To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• [x] I have updated existing examples or added new ones (if applicable).
• [x] I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• [ ] My changes were added to the CHANGELOG.md in the proper section.
• [ ] I have added necessary changes to user code to the Migration Guide.
• [ ] My changes are in accordance to the esp-rs API guidelines

Extra:

• [ ] I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

This patch refactors the ledc (LED PWM Controller) module, simplifying the code by removing direct references to RegisterBlock. This change eliminates the need for a lifetime specifier in certain structs, making the code more flexible and easier to maintain.

Additionally, new functionality has been added to support overflow counter management and overflow interrupt handling for channels on specific chips like ESP32S3.

Key changes include:

• Added methods to enable and disable signal output for channels.
• Introduced overflow counter and interrupt handling mechanisms, including methods to enable/disable the counter, configure overflow thresholds, and manage interrupts.
• Extended the timer interface with additional methods like pause, resume, and reset for better control over timers.

An example (ledc_pulse_counting.rs) has been added, showcasing how to generate a PWM signal with a predefined number of pulses and handle the overflow interrupt.

Testing

Currently, ledc_pulse_counting.rs cannot compile due to thread safe issues.

error[E0277]: `(dyn TimerIFace<LowSpeed> + 'static)` cannot be shared between threads safely
   --> src/bin/ledc_pulse_counting.rs:34:18
    |
34  | static CHANNEL0: Mutex<RefCell<Option<Channel<'_, LowSpeed, GpioPin<0>>>>> =
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn TimerIFace<LowSpeed> + 'static)` cannot be shared between threads safely
    |
    = help: the trait `core::marker::Sync` is not implemented for `(dyn TimerIFace<LowSpeed> + 'static)`, which is required by `critical_section::Mutex<RefCell<Option<esp_hal::ledc::channel::Channel<'static, LowSpeed, GpioPin<0>>>>>: core::marker::Sync`
    = note: required for `&'static (dyn TimerIFace<LowSpeed> + 'static)` to implement `Send`

I tested the code after applying a workaround in ledc_pulse_counting.rs. It works fine on my esp32s3.

[KashunCheng]

@bugadani I’m currently following along with the pcnt_encoder example to write a similar implementation for led in ledc_pulse_counting.rs, but I’m encountering compilation issues. Could you provide guidance on how to address these issues with the Channel API?

Additionally, while reviewing the pcnt module, I noticed that some operations are wrapped in a critical section when writing to registers. I'm a bit unclear on when exactly I should wrap register operations in a critical section block for my newly-added ledc code. Could you clarify the situations or conditions that necessitate the use of a critical section for such operations?

Thank you for your time and assistance.

[bugadani]

I’m currently following along with the pcnt_encoder example to write a similar implementation for led in ledc_pulse_counting.rs, but I’m encountering compilation issues. Could you provide guidance on how to address these issues with the Channel API?

Can you clarify, please?

[bugadani]

Technically, you need to use a critical section (or better, esp_hal::lock) if it is possible to modify a register from multiple places. Essentially in every case where a type is Sync, takes &self, and is public. I don't think PCNT is correct here, and I'd say you can ignore this problem for now.

[KashunCheng]

I’m currently following along with the pcnt_encoder example to write a similar implementation for led in ledc_pulse_counting.rs, but I’m encountering compilation issues. Could you provide guidance on how to address these issues with the Channel API?

Can you clarify, please?

Thank you for your response. To clarify my previous message, I followed the structure of the pcnt_encoder example to write a similar implementation for LED control in ledc_pulse_counting.rs. Both examples share commonalities, such as:

1. Using global variables to store the peripheral structs (e.g., static CHANNEL0: Mutex<RefCell<WORKAROUND>> in ledc_pulse_counting and static UNIT0: Mutex<RefCell<Option<unit::Unit<'static, 1>>>> in pcnt).
2. Defining an interrupt handler to manage interrupts.

However, the key difference between the two is that pcnt explicitly marks its unit as Send:

https://github.com/esp-rs/esp-hal/blob/45806dfba02056a14dd11775a8fdd23879c06212/esp-hal/src/pcnt/unit.rs#L310-L311

This allows pcnt to compile successfully, while ledc_pulse_counting does not. In ledc, the Channel takes a reference to the Timer, which is not marked as Send, thus causing the compilation issue. To work around this, I used a structure like WORKAROUND.

My question is: Is there any other way to solve this problem more elegantly?

[bugadani]

I don't see a good solution here short of rewriting the whole driver. Which we will need to do, it doesn't look like the best designed driver to me...

Regardless, let's accept that the situation is not ideal. You can require TimerSpeed::ClockSourceType to be Send + Sync, that way hopefully Timer can be automatically Sync and we can place it in a static variable. (if not, you can unsafe impl it)

This gets us to a place where a timer can be configured and be made accessible.

Next, I don't think Channel should hold on to the timer reference. It mostly needs duty and the timer's number and the implementation can just copy that out of the timer when initialising.

With these two changes, I think the example has a chance of working, with a structure like this:

• Create a LEDC instance and put it in a static_cell::StaticCell.
• Take and configure a Timer
• Use the timer to set up a Channel
• Store the timer in a global Mutex
• Do stuff with the channel.

It's incorrect (you can reconfigure the timer while you have active channels) but that's the best I can come up with, given what we have.

[bjoernQ]

... rewriting the whole driver ...

Yes - this driver is known to be the ugly duckling

[jessebraham]

I guess this is blocked for now, so added the label; if this is incorrect please feel free to remove it.

[MabezDev]

Thanks for the PR @KashunCheng. I think the consensus here is that LEDC is not fit for purpose as it currently lives. We should probably spend our resources on a fresh design, if you have any insights it would be great to hear them in #2286.

Closing this for now.