From d87de032dfcbc5d348118cb4edd7b212666bb9c3 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/esp_driver_ledc/src/ledc.c | 5 ++--- .../esp_driver_ledc/test_apps/ledc/main/test_ledc.c | 6 ++++++ components/hal/esp32/include/hal/ledc_ll.h | 8 +++++--- components/hal/esp32c2/include/hal/ledc_ll.h | 5 ++--- components/hal/esp32c3/include/hal/ledc_ll.h | 5 ++--- components/hal/esp32c5/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32c6/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32c61/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32h2/include/hal/ledc_ll.h | 7 +++---- components/hal/esp32p4/include/hal/ledc_ll.h | 5 ++--- components/hal/esp32s2/include/hal/ledc_ll.h | 5 ++--- components/hal/esp32s3/include/hal/ledc_ll.h | 5 ++--- components/hal/include/hal/ledc_hal.h | 3 +-- components/hal/ledc_hal_iram.c | 4 ++-- 14 files changed, 38 insertions(+), 41 deletions(-) diff --git a/components/esp_driver_ledc/src/ledc.c b/components/esp_driver_ledc/src/ledc.c index 1db84af62c..29d2fcb928 100644 --- a/components/esp_driver_ledc/src/ledc.c +++ b/components/esp_driver_ledc/src/ledc.c @@ -975,7 +975,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); } @@ -998,7 +998,6 @@ esp_err_t ledc_stop(ledc_mode_t speed_mode, ledc_channel_t channel, uint32_t idl portENTER_CRITICAL_SAFE(&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_SAFE(&ledc_spinlock); return ESP_OK; @@ -1261,7 +1260,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/esp_driver_ledc/test_apps/ledc/main/test_ledc.c b/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c index c6a3a7483e..0e103d8167 100644 --- a/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c +++ b/components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c @@ -150,6 +150,12 @@ TEST_CASE("LEDC output idle level test", "[ledc]") TEST_ESP_OK(ledc_stop(test_speed_mode, LEDC_CHANNEL_0, !current_level)); vTaskDelay(1000 / portTICK_PERIOD_MS); TEST_ASSERT_EQUAL_INT32(!current_level, LEDC.channel_group[test_speed_mode].channel[LEDC_CHANNEL_0].conf0.idle_lv); + // check real output level over some period + gpio_input_enable(PULSE_IO); + for (int i = 0; i < 40; i++) { + TEST_ASSERT_EQUAL_INT32(!current_level, gpio_get_level(PULSE_IO)); + esp_rom_delay_us(50); + } } TEST_CASE("LEDC iterate over all channel and timer configs", "[ledc]") diff --git a/components/hal/esp32/include/hal/ledc_ll.h b/components/hal/esp32/include/hal/ledc_ll.h index 9f98ecc72a..198c56bd5d 100644 --- a/components/hal/esp32/include/hal/ledc_ll.h +++ b/components/hal/esp32/include/hal/ledc_ll.h @@ -481,13 +481,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 0470664986..04d2517f77 100644 --- a/components/hal/esp32c2/include/hal/ledc_ll.h +++ b/components/hal/esp32c2/include/hal/ledc_ll.h @@ -458,13 +458,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 f96582f614..ae7dfcc94e 100644 --- a/components/hal/esp32c3/include/hal/ledc_ll.h +++ b/components/hal/esp32c3/include/hal/ledc_ll.h @@ -459,13 +459,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/esp32c5/include/hal/ledc_ll.h b/components/hal/esp32c5/include/hal/ledc_ll.h index 184ba2355d..3e237c6801 100644 --- a/components/hal/esp32c5/include/hal/ledc_ll.h +++ b/components/hal/esp32c5/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -458,13 +458,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/esp32c6/include/hal/ledc_ll.h b/components/hal/esp32c6/include/hal/ledc_ll.h index a7f1374c4b..e6a0c73e3f 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-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -579,13 +579,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/esp32c61/include/hal/ledc_ll.h b/components/hal/esp32c61/include/hal/ledc_ll.h index 38da76b0f5..4cd0f760ae 100644 --- a/components/hal/esp32c61/include/hal/ledc_ll.h +++ b/components/hal/esp32c61/include/hal/ledc_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -458,13 +458,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 56ae897e13..8351003001 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-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -577,13 +577,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/esp32p4/include/hal/ledc_ll.h b/components/hal/esp32p4/include/hal/ledc_ll.h index 38c3d5358d..76ce621674 100644 --- a/components/hal/esp32p4/include/hal/ledc_ll.h +++ b/components/hal/esp32p4/include/hal/ledc_ll.h @@ -484,13 +484,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 2bc2627125..266d18b72a 100644 --- a/components/hal/esp32s2/include/hal/ledc_ll.h +++ b/components/hal/esp32s2/include/hal/ledc_ll.h @@ -498,13 +498,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 e9a8ad17fd..e012868178 100644 --- a/components/hal/esp32s3/include/hal/ledc_ll.h +++ b/components/hal/esp32s3/include/hal/ledc_ll.h @@ -459,13 +459,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 fe9275ee9b..18ac920a2d 100644 --- a/components/hal/include/hal/ledc_hal.h +++ b/components/hal/include/hal/ledc_hal.h @@ -263,11 +263,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 ac3112caa8..34884c78e7 100644 --- a/components/hal/ledc_hal_iram.c +++ b/components/hal/ledc_hal_iram.c @@ -16,9 +16,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)