pwm_led_esp32: Move perihperal setup to driver init. #82177
Conversation
zephyrbot
added
platform: ESP32
zephyrbot
requested review from
anangl,
henrikbrixandersen,
marekmatej,
raffarost,
sylvioalves,
uLipe and
wmrsouza
|
Hello @braiden, and thank you very much for your first pull request to the Zephyr project! |
| for (int i = 0; i < config->channel_len; i++) { | ||
| struct pwm_ledc_esp32_channel_config *channel = &config->channel_config[i]; | ||
| ledc_hal_init(&data->hal, channel->speed_mode); | ||
| ledc_hal_timer_rst(&data->hal, channel->timer_num); |
There was a problem hiding this comment.
While calling ledc_hal_timer_rst appears to work, I'm a little uncertain on when this method actually needs to be called. The TRM doesn't state. I know it needs to be called, simply removing ledc_hal_timer_rst results in no output.
ESP-IDF calls this method during ledc_timer_config, but only after more thoroughly configuring the timer then I can. I can't completely configure the timer, because I don't know what clock source to pick. That's because (on some hardware) the driver may pick different clocks to optimize timer resolution based on the requested PWM period in application code.
I considered leaving ledc_hal_timer_rst where it was, and only calling it the first time (and maybe when the clock selection changed).
I also considered adding some sort of hint to DT so init could pick a good timer source.
... but, I didn't want to complicate the code more than necessary to fix the issue.
There was a problem hiding this comment.
@braiden This driver needs some updates!
The original idea was to optimize the clock source selection according to a given period and cycle, in such manner that the user wouldn't need to care about it. But this no longer applies after some changes in the driver. I believe ledc_hal_timer_rst must be called when updating the clock source, but since it does not change anymore, no problem on keeping it where it is for now.
Thanks for the PR and feel free to contribute in anyway to make it better.
There was a problem hiding this comment.
I think the esp32 still has two clock to choose from? So it might be possible for the clock to change if pwm duty is set very slow then very fast. (I'll check code to confirm).
If so, I think its safer I keep ledc_hal_timer_rst where was before my change, and skip reset only if clock hasn't changed by the current call to pwm_set.
There was a problem hiding this comment.
ESP32 & ESP32s2 can select either APB (80mhz), or REF_TICK (10mhz) falling back on the slower clock if period is too slow: source. The clock can change at run-time if application code requests periods that might select different clocks.
I tested on ESP32 both with and without my fix applied. In both cases there was no PWM output when REF_TICK was selected. I think this PR is still safe since its not changing any behavior here. (although the only ESP32s i had available to test with are ESP32-D0WDQ6 and require CONFIG_ESP32_USE_UNSUPPORTED_REVISION)
I can follow-up with another PR for REF_TICK. What's the best way to discuss, should I log a bug and discuss there?
|
@braiden thanks for the PR! Could you address the tiny compliance error please, then amend your commit and force push to your branch? Thanks! |
|
@braiden I missed before: fix the typo in the commit message "peripherals". |
|
You may also want to have the message say "Fixes #81801" instead of what you have now, as I think otherwise github doesn't properly link that PR to the issue. Thank you! edit: I just manually established the linkage |
braiden
dismissed stale reviews from sylvioalves, LucasTambor, and wmrsouza
via
4f3e253
|
Sorry, I somehow closed this PR trying to fix compliance error, new PR here: |
I've reopened it. Please close the duplicate. |
fixes zephyrproject-rtos#81801 Move as much peripheral initialization as possible into driver init. Prior to this change `ledc_hal_timer_rst` is called every time the PWM period or duty is changed. Since `ledc_hal_timer_rst` resets this timer, it causes unwanted phase shifts. Also move `pwm_led_esp32_configure_pinctrl` to diver init. This was also called when PWM period or duty changed, and can call a short negative glitch if the ouput is high when pwm api method is called. Signed-off-by: Braiden Kindt <braiden@kindt.dev>
|
I've addressed all comments. Please take a look. |
|
I'm closing this PR as its being addressed in #82820 |
Fixes for #81801
Move as much peripheral initialization as possible into driver init.
Prior to this change
ledc_hal_timer_rstis called every time the PWM period or duty is changed. Sinceledc_hal_timer_rstresets this timer, it causes unwanted phase shifts.Also move
pwm_led_esp32_configure_pinctrlto diver init. This was also called when PWM period or duty changed, and can call a short negative glitch if the ouput is high when pwm api method is called.Tested on esp32c3. Phase no longer shifts, and period update only on timer overflow.