drivers: pwm_led: esp32: Driver update

[raffarost]

Update LEDC driver in order to address GH reported issues and improve clock management.

Following behaviors are ensured:

• inverted PWM
• default signal level after driver init, as configured
• PWM is now stopped when 0% or 100% duty is selected, to allow using max timer resolution
• transitions when frequency is updated follows timer overflow (synced transition)
• transition for 0% or 100% duty is immediate
• timer can only be shared by two or more channels if frequency is the same. If new value for frequency is requested, it will be rejected to avoid cross-channel interference.

Note: changes were kept in several commits to facilitate review, but can be squashed if requested.

[braiden]

Thanks for this, I think it can supersede my PR #82177? I'll merge and test.

There's an open comment on my PR that still worth consideration here: I proposed either supporting only one clock source or, moving the clock source to the device tree.

The rationale: All low speed channels share the same clock source. On a device such as esp32 that has multiple clock choices: suppose there are two distinct PWM low-speed channels each using a distinct timer. Then call set_cycles to configure ch0 with a small period selecting APB clock. Then call set_cycles for ch1 with a very long period so it picks REF_TICK clock. ch1 cannot modify the clock w/o squishing or stretching the PWM output already running on ch0. Even though the channel and timer are distinct, clocks are shared (for low speed).

Thoughts? Initialize an immutable clock source in driver init, rather than reconfiguration on-the-fly?

In any-case thanks for PR: LGTM. My issue's not a new bug and exists before and after this PR.

[braiden]

I tested on esp32c3 working-as-intended but, one suggestion:

Do you think it makes sense to reinitialize/reset the timer only when the clock selection changes, rather than every time the frequency changes? As-is here's what it looks like when I change the freq:

If the timer had not been reset, the trigger point would be updated on the next overflow resulting in a smooth transition to the new frequency.

[raffarost]

hi @braiden
your comments on clock management are right, there's some good improvement to be done in the driver
I'll try to take into account all the comments and work on it in the next weeks
thanks for your input

[raffarost]

@epc-ake @braiden can you take a look to see if your concerns were addressed?
if you are okay with the changes I'll open the PR for reviewing

[raffarost]

Thanks for this, I think it can supersede my PR #82177? I'll merge and test.

There's an open comment on my PR that still worth consideration here: I proposed either supporting only one clock source or, moving the clock source to the device tree.

The rationale: All low speed channels share the same clock source. On a device such as esp32 that has multiple clock choices: suppose there are two distinct PWM low-speed channels each using a distinct timer. Then call set_cycles to configure ch0 with a small period selecting APB clock. Then call set_cycles for ch1 with a very long period so it picks REF_TICK clock. ch1 cannot modify the clock w/o squishing or stretching the PWM output already running on ch0. Even though the channel and timer are distinct, clocks are shared (for low speed).

Thoughts? Initialize an immutable clock source in driver init, rather than reconfiguration on-the-fly?

In any-case thanks for PR: LGTM. My issue's not a new bug and exists before and after this PR.

I attempted the testcase you mention above and here's what I get:

START - test_pwm_nsec
[PWM]: 0, [period]: 1000000000, [pulse]: 500000000
D: channel_num=0, speed_mode=1, timer_num=0, clock_src=11, prescaler=488, resolution=19
[PWM]: 1, [period]: 100, [pulse]: 50
D: channel_num=1, speed_mode=1, timer_num=1, clock_src=4, prescaler=256, resolution=3
 PASS - test_pwm_nsec in 0.024 seconds

Signals are fine on the oscilloscope. The condition you mention makes sense when the same timer is used instead, and for this situation a check is now made on pwm_set to avoid using the same timer while setting different frequencies. Timers can still be shared using the same frequency but setting different duty cycles on the channels.

[epc-ake]

Hi @raffarost
I tried out this PR.
I tried it using the led_shell and pwm_shell (CONFIG_LED_SHELL and CONFIG_PWM_SHELL enabled. )

First I had some trouble with the led_shell and setting led brightness.
I had set the pwm frequency to a low 100 HZ. That caused the following errors.

<err> pwm_ledc_esp32: Error finding timer config for channel 0 for requested frequency
<err> pwm_ledc_esp32: Error finding timer config for channel 0 for requested frequency

I got the same errors when trying to set a 100Hz frequency with the pwm_shell:
pwm usec ledc@60019000 0 1000 900

Increasing it to 1000Hz fixed it though.

Is there a reason why low frequencies are not supported?
e.g. low frequencies could be nice to let an LED blink.

[raffarost]

Hi @raffarost I tried out this PR. I tried it using the led_shell and pwm_shell (CONFIG_LED_SHELL and CONFIG_PWM_SHELL enabled. )

First I had some trouble with the led_shell and setting led brightness. I had set the pwm frequency to a low 100 HZ. That caused the following errors.

<err> pwm_ledc_esp32: Error finding timer config for channel 0 for requested frequency
<err> pwm_ledc_esp32: Error finding timer config for channel 0 for requested frequency

I got the same errors when trying to set a 100Hz frequency with the pwm_shell: pwm usec ledc@60019000 0 1000 900

Increasing it to 1000Hz fixed it though.

Is there a reason why low frequencies are not supported? e.g. low frequencies could be nice to let an LED blink.

hello @epc-ake
which device are you using? S3?
As the clock source is automatically selected, this can happen indeed, while the configuration (possibly HAL) needs further checks. With ESP32 I tested from 1 Hz to 10 Mhz, but it looks like I need to test more on other devices :)
Thanks for the input, I'll have a look.

[epc-ake]

which device are you using? S3?

Yes I'm using the esp32s3.

[raffarost]

I included the fix that is already in place in the driver, so now it reaches 10 Hz on S3 and preserves the original functionality.

[epc-ake]

I included the fix that is already in place in the driver, so now it reaches 10 Hz on S3 and preserves the original functionality.

confirmed 👍

[marekmatej]

just a nits, LTGM

[raffarost]

Hi @kartben
Is this PR missing something we can do (to have it merged)?

[kartben]

Hi @kartben Is this PR missing something we can do (to have it merged)?

It was missing approval from the assignee, PWM maintainer @anangl
Given this really only touched esp32 specific code, I've taken the liberty to merge.