From 9b3e3d1b3ffc2470f32498253a50f09e3eb184ef Mon Sep 17 00:00:00 2001 From: Song Ruo Jing Date: Tue, 17 Jun 2025 20:11:17 +0800 Subject: [PATCH] fix(ledc): duty_start bit should wait for its self-clear before next set on esp32 --- components/driver/ledc/ledc.c | 5 ++--- components/hal/esp32/include/hal/ledc_ll.h | 10 ++++++---- components/hal/esp32c2/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32c3/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32c6/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32h2/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32s2/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32s3/include/hal/ledc_ll.h | 7 +++---- components/hal/include/hal/ledc_hal.h | 3 +-- components/hal/ledc_hal_iram.c | 4 ++-- 10 files changed, 29 insertions(+), 35 deletions(-) diff --git a/components/driver/ledc/ledc.c b/components/driver/ledc/ledc.c index 0ac394ee74..38b76b8df4 100644 --- a/components/driver/ledc/ledc.c +++ b/components/driver/ledc/ledc.c @@ -632,7 +632,7 @@ esp_err_t ledc_channel_config(const ledc_channel_config_t *ledc_conf) static void _ledc_update_duty(ledc_mode_t speed_mode, ledc_channel_t channel) { ledc_hal_set_sig_out_en(&(p_ledc_obj[speed_mode]->ledc_hal), channel, true); - ledc_hal_set_duty_start(&(p_ledc_obj[speed_mode]->ledc_hal), channel, true); + ledc_hal_set_duty_start(&(p_ledc_obj[speed_mode]->ledc_hal), channel); ledc_ls_channel_update(speed_mode, channel); } @@ -655,7 +655,6 @@ esp_err_t ledc_stop(ledc_mode_t speed_mode, ledc_channel_t channel, uint32_t idl portENTER_CRITICAL(&ledc_spinlock); ledc_hal_set_idle_level(&(p_ledc_obj[speed_mode]->ledc_hal), channel, idle_level); ledc_hal_set_sig_out_en(&(p_ledc_obj[speed_mode]->ledc_hal), channel, false); - ledc_hal_set_duty_start(&(p_ledc_obj[speed_mode]->ledc_hal), channel, false); ledc_ls_channel_update(speed_mode, channel); portEXIT_CRITICAL(&ledc_spinlock); return ESP_OK; @@ -890,7 +889,7 @@ static void IRAM_ATTR ledc_fade_isr(void *arg) cycle, scale); s_ledc_fade_rec[speed_mode][channel]->fsm = LEDC_FSM_HW_FADE; - ledc_hal_set_duty_start(&(p_ledc_obj[speed_mode]->ledc_hal), channel, true); + ledc_hal_set_duty_start(&(p_ledc_obj[speed_mode]->ledc_hal), channel); ledc_ls_channel_update(speed_mode, channel); } portEXIT_CRITICAL_ISR(&ledc_spinlock); diff --git a/components/hal/esp32/include/hal/ledc_ll.h b/components/hal/esp32/include/hal/ledc_ll.h index 212744f83a..444f4730ed 100644 --- a/components/hal/esp32/include/hal/ledc_ll.h +++ b/components/hal/esp32/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2019-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2019-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -411,13 +411,15 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, high-speed mode or low-speed mode * @param channel_num LEDC channel index (0-7), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + // wait until the last duty change took effect (duty_start bit will be self-cleared when duty update or fade is done) + // this is necessary on ESP32 only, otherwise, internal logic might mess up (later targets with SOC_LEDC_SUPPORT_FADE_STOP allow to re-configure parameters while last update is still in progress) + while (hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start); + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/esp32c2/include/hal/ledc_ll.h b/components/hal/esp32c2/include/hal/ledc_ll.h index 3b8d91f6a6..a1dc64bd8e 100644 --- a/components/hal/esp32c2/include/hal/ledc_ll.h +++ b/components/hal/esp32c2/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -396,13 +396,12 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, high-speed mode or low-speed mode * @param channel_num LEDC channel index (0-7), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/esp32c3/include/hal/ledc_ll.h b/components/hal/esp32c3/include/hal/ledc_ll.h index 0edca33890..6fbb52fcf5 100644 --- a/components/hal/esp32c3/include/hal/ledc_ll.h +++ b/components/hal/esp32c3/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -396,13 +396,12 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, high-speed mode or low-speed mode * @param channel_num LEDC channel index (0-7), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/esp32c6/include/hal/ledc_ll.h b/components/hal/esp32c6/include/hal/ledc_ll.h index 49eaa0a098..2144f18f67 100644 --- a/components/hal/esp32c6/include/hal/ledc_ll.h +++ b/components/hal/esp32c6/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -499,13 +499,12 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, low-speed mode only * @param channel_num LEDC channel index (0-5), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/esp32h2/include/hal/ledc_ll.h b/components/hal/esp32h2/include/hal/ledc_ll.h index 7c319e374a..c534a483d6 100644 --- a/components/hal/esp32h2/include/hal/ledc_ll.h +++ b/components/hal/esp32h2/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -497,13 +497,12 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, low-speed mode only * @param channel_num LEDC channel index (0-5), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/esp32s2/include/hal/ledc_ll.h b/components/hal/esp32s2/include/hal/ledc_ll.h index fbdd6cc2f1..42136e587c 100644 --- a/components/hal/esp32s2/include/hal/ledc_ll.h +++ b/components/hal/esp32s2/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2019-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2019-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -428,13 +428,12 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, high-speed mode or low-speed mode * @param channel_num LEDC channel index (0-7), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/esp32s3/include/hal/ledc_ll.h b/components/hal/esp32s3/include/hal/ledc_ll.h index 101443a2f0..66fcd194cf 100644 --- a/components/hal/esp32s3/include/hal/ledc_ll.h +++ b/components/hal/esp32s3/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -397,13 +397,12 @@ static inline void ledc_ll_set_sig_out_en(ledc_dev_t *hw, ledc_mode_t speed_mode * @param hw Beginning address of the peripheral registers * @param speed_mode LEDC speed_mode, high-speed mode or low-speed mode * @param channel_num LEDC channel index (0-7), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num, bool duty_start) +static inline void ledc_ll_set_duty_start(ledc_dev_t *hw, ledc_mode_t speed_mode, ledc_channel_t channel_num) { - hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = duty_start; + hw->channel_group[speed_mode].channel[channel_num].conf1.duty_start = 1; } /** diff --git a/components/hal/include/hal/ledc_hal.h b/components/hal/include/hal/ledc_hal.h index 91610ee7eb..dbad16c6da 100644 --- a/components/hal/include/hal/ledc_hal.h +++ b/components/hal/include/hal/ledc_hal.h @@ -255,11 +255,10 @@ void ledc_hal_ls_channel_update(ledc_hal_context_t *hal, ledc_channel_t channel_ * * @param hal Context of the HAL layer * @param channel_num LEDC channel index (0-7), select from ledc_channel_t - * @param duty_start The duty start * * @return None */ -void ledc_hal_set_duty_start(ledc_hal_context_t *hal, ledc_channel_t channel_num, bool duty_start); +void ledc_hal_set_duty_start(ledc_hal_context_t *hal, ledc_channel_t channel_num); /** * @brief Set LEDC the integer part of duty value diff --git a/components/hal/ledc_hal_iram.c b/components/hal/ledc_hal_iram.c index 0ea5f8617d..9761028135 100644 --- a/components/hal/ledc_hal_iram.c +++ b/components/hal/ledc_hal_iram.c @@ -15,9 +15,9 @@ void ledc_hal_ls_channel_update(ledc_hal_context_t *hal, ledc_channel_t channel_ ledc_ll_ls_channel_update(hal->dev, hal->speed_mode, channel_num); } -void ledc_hal_set_duty_start(ledc_hal_context_t *hal, ledc_channel_t channel_num, bool duty_start) +void ledc_hal_set_duty_start(ledc_hal_context_t *hal, ledc_channel_t channel_num) { - ledc_ll_set_duty_start(hal->dev, hal->speed_mode, channel_num, duty_start); + ledc_ll_set_duty_start(hal->dev, hal->speed_mode, channel_num); } void ledc_hal_set_duty_int_part(ledc_hal_context_t *hal, ledc_channel_t channel_num, uint32_t duty_val)