[WIP} introduce overflow interrupt handing for led #2155

KashunCheng · 2024-09-13T12:54:58Z

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 📝

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.
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 · 2024-09-13T13:00:57Z

@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

Thanks for bearing with me, I can at least understand this PR :) However, I still have some concerns, one of which is related to the PR, the other one isn't necessarily.

bugadani · 2024-09-13T14:36:33Z

esp-hal/src/ledc/mod.rs

            .timer(0)
            .conf()
            .modify(|_, w| w.para_up().set_bit());
    }

    /// Return a new timer
-    pub fn get_timer<S: TimerSpeed>(&self, number: timer::Number) -> Timer<'d, S> {
-        Timer::new(self.ledc, number)
+    pub fn get_timer<S: TimerSpeed>(&self, number: timer::Number) -> Timer<S> {


So, I guess this isn't new, but I can get any number of references to the same timer, then I can configure that timer in different ways. If that is intentional, creating a Timer shouldn't even need the Ledc object. If that is not intentional, this will somehow need to be prevented.

And if you're touching this code, please remove the get_ prefix from the function name.

To address the issue of "getting any number of references to the same timer," would it make sense to take an approach similar to what is used in Rmt? The following code is taken from rmt.rs:

pub struct Rmt<'d, M> where M: crate::Mode, { _peripheral: PeripheralRef<'d, crate::peripherals::RMT>, /// RMT Channel 0. pub channel0: ChannelCreator<M, 0>, /// RMT Channel 1. pub channel1: ChannelCreator<M, 1>, /// RMT Channel 2. pub channel2: ChannelCreator<M, 2>, /// RMT Channel 3. pub channel3: ChannelCreator<M, 3>, phantom: PhantomData<M>, }

In this approach, we construct the channels and timers in advance and ensure they are moved. However, this would require a significant refactoring, so it might be better to address this in a separate PR.

Regarding the removal of the get_ prefix from the function name, as I reintroduced the lifetime, so I the function signature remains the same in this PR, which is why I haven't removed the prefix here.

bugadani · 2024-09-13T14:41:23Z

esp-hal/src/ledc/timer.rs

 }

 /// Timer struct
-pub struct Timer<'a, S: TimerSpeed> {
-    ledc: &'a crate::peripherals::ledc::RegisterBlock,
+pub struct Timer<S: TimerSpeed> {


I am not sure but I think we don't want the Timers to potentially survive our use of LEDC. If the user takes a Timer, drops Ledc, reinitializes it and grabs the same Timer, we'll end up in a bad spot.

Thank you for pointing this out. I have reintroduced the lifetime parameter for the timer.

bugadani · 2024-09-13T14:45:08Z

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 · 2024-09-13T14:50:22Z

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.

… configuration

KashunCheng · 2024-09-17T00:17:38Z

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:

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).
Defining an interrupt handler to manage interrupts.

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

esp-hal/esp-hal/src/pcnt/unit.rs

Lines 310 to 311 in 45806df

    
           // The entire Unit is Send but the individual channels are not. 
        
           unsafe impl<'d, const NUM: usize> Send for Unit<'d, NUM> {}

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 · 2024-09-17T12:02:57Z

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 · 2024-09-18T06:59:55Z

... rewriting the whole driver ...

Yes - this driver is known to be the ugly duckling

jessebraham · 2024-10-21T12:45:42Z

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

KashunCheng added 3 commits September 13, 2024 20:13

[feat] refactor ledc and introduce overflow interrupt handing

9fdc739

[feat] clean up with Ledc::register_block()

4800856

[fix] temporary workaround for the example

14af10c

KashunCheng marked this pull request as draft September 13, 2024 12:55

[feat] reset counter

da5fd96

bugadani reviewed Sep 13, 2024

View reviewed changes

[feat] rollback lifetime changes for timer and add support for hpoint…

ccab4eb

… configuration

MabezDev mentioned this pull request Oct 7, 2024

LEDC driver improvements #2286

Open

jessebraham added the status:blocked Unable to progress - dependent on another task label Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP} introduce overflow interrupt handing for led #2155

[WIP} introduce overflow interrupt handing for led #2155

KashunCheng commented Sep 13, 2024 •

edited

Loading

KashunCheng commented Sep 13, 2024

bugadani left a comment

bugadani Sep 13, 2024

KashunCheng Sep 17, 2024

bugadani Sep 13, 2024

KashunCheng Sep 17, 2024

bugadani commented Sep 13, 2024

bugadani commented Sep 13, 2024

KashunCheng commented Sep 17, 2024

bugadani commented Sep 17, 2024

bjoernQ commented Sep 18, 2024

jessebraham commented Oct 21, 2024

[WIP} introduce overflow interrupt handing for led #2155

Are you sure you want to change the base?

[WIP} introduce overflow interrupt handing for led #2155

Conversation

KashunCheng commented Sep 13, 2024 • edited Loading

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

KashunCheng commented Sep 13, 2024

bugadani left a comment

Choose a reason for hiding this comment

bugadani Sep 13, 2024

Choose a reason for hiding this comment

KashunCheng Sep 17, 2024

Choose a reason for hiding this comment

bugadani Sep 13, 2024

Choose a reason for hiding this comment

KashunCheng Sep 17, 2024

Choose a reason for hiding this comment

bugadani commented Sep 13, 2024

bugadani commented Sep 13, 2024

KashunCheng commented Sep 17, 2024

bugadani commented Sep 17, 2024

bjoernQ commented Sep 18, 2024

jessebraham commented Oct 21, 2024

KashunCheng commented Sep 13, 2024 •

edited

Loading