Merge branch 'bugfix/ledc_update_duty_wait' into 'master'

fix(ledc): duty_start update bit should wait for its self-clear before next set

Closes IDF-11989

See merge request espressif/esp-idf!39949

Author: songruo

components/esp_driver_ledc/CMakeLists.txt

@@ -10,13 +10,12 @@ endif()
 if(${target} STREQUAL "linux")
     set(priv_requires "")
 else()
-    set(priv_requires esp_pm)
+    set(priv_requires esp_pm esp_driver_gpio)
 endif()
 
 idf_component_register(
     SRCS ${srcs}
     INCLUDE_DIRS ${public_include}
     PRIV_REQUIRES "${priv_requires}"
-    REQUIRES esp_driver_gpio    # IDF-11989: Remove this in IDF v6.0
     LDFRAGMENTS "linker.lf"
 )

components/esp_driver_ledc/include/driver/ledc.h

@@ -9,27 +9,11 @@
 #include "esp_err.h"
 #include "esp_intr_alloc.h"
 #include "hal/ledc_types.h"
-#include "driver/gpio.h"
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-#if SOC_LEDC_SUPPORT_APB_CLOCK
-/**
- * @brief Frequency of one of the LEDC peripheral clock sources, APB_CLK
- * @note  This macro should have no use in your application, we keep it here only for backward compatible
- */
-#define LEDC_APB_CLK_HZ _Pragma ("GCC warning \"'LEDC_APB_CLK_HZ' macro is deprecated\"") (APB_CLK_FREQ)
-#endif
-#if SOC_LEDC_SUPPORT_REF_TICK
-/**
- * @brief Frequency of one of the LEDC peripheral clock sources, REF_TICK
- * @note  This macro should have no use in your application, we keep it here only for backward compatible
- */
-#define LEDC_REF_CLK_HZ _Pragma ("GCC warning \"'LEDC_REF_CLK_HZ' macro is deprecated\"") (REF_CLK_FREQ)
-#endif
-
 #define LEDC_ERR_DUTY           (0xFFFFFFFF)
 #define LEDC_ERR_VAL            (-1)
 
@@ -52,7 +36,7 @@ typedef struct {
     int gpio_num;                   /*!< the LEDC output gpio_num, if you want to use gpio16, gpio_num = 16 */
     ledc_mode_t speed_mode;         /*!< LEDC speed speed_mode, high-speed mode (only exists on esp32) or low-speed mode */
     ledc_channel_t channel;         /*!< LEDC channel (0 - LEDC_CHANNEL_MAX-1) */
-    ledc_intr_type_t intr_type;     /*!< configure interrupt, Fade interrupt enable  or Fade interrupt disable */
+    ledc_intr_type_t intr_type __attribute__((deprecated));     /*!< @deprecated, no need to explicitly configure interrupt, handled in the driver */
     ledc_timer_t timer_sel;         /*!< Select the timer source of channel (0 - LEDC_TIMER_MAX-1) */
     uint32_t duty;                  /*!< LEDC channel duty, the range of duty setting is [0, (2**duty_resolution)] */
     int hpoint;                     /*!< LEDC channel hpoint value, the range is [0, (2**duty_resolution)-1] */
@@ -347,28 +331,7 @@ esp_err_t ledc_set_fade(ledc_mode_t speed_mode, ledc_channel_t channel, uint32_t
  *     - ESP_ERR_INVALID_ARG Parameter error
  *     - ESP_ERR_NOT_FOUND Failed to find available interrupt source
  */
-esp_err_t ledc_isr_register(void (*fn)(void *), void *arg, int intr_alloc_flags, ledc_isr_handle_t *handle);
-
-/**
- * @brief Configure LEDC timer settings
- *
- * This function does not take care of whether the chosen clock source is enabled or not, also does not handle the clock source
- * to meet channel sleep mode choice.
- *
- * If the chosen clock source is a new clock source to the LEDC timer, please use `ledc_timer_config`;
- * If the clock source is kept to be the same, but frequency needs to be updated, please use `ledc_set_freq`.
- *
- * @param speed_mode Select the LEDC channel group with specified speed mode. Note that not all targets support high speed mode.
- * @param timer_sel  Timer index (0-3), there are 4 timers in LEDC module
- * @param clock_divider Timer clock divide value, the timer clock is divided from the selected clock source
- * @param duty_resolution Resolution of duty setting in number of bits. The range is [1, SOC_LEDC_TIMER_BIT_WIDTH]
- * @param clk_src Select LEDC source clock.
- *
- * @return
- *     - (-1) Parameter error
- *     - Other Current LEDC duty
- */
-esp_err_t ledc_timer_set(ledc_mode_t speed_mode, ledc_timer_t timer_sel, uint32_t clock_divider, uint32_t duty_resolution, ledc_clk_src_t clk_src) __attribute__((deprecated("Please use ledc_timer_config() or ledc_set_freq()")));
+esp_err_t ledc_isr_register(void (*fn)(void *), void *arg, int intr_alloc_flags, ledc_isr_handle_t *handle) __attribute__((deprecated("LEDC interrupt handling is implemented by driver itself, please only register event callbacks if necessary")));
 
 /**
  * @brief Reset LEDC timer

components/esp_driver_ledc/src/ledc.c

@@ -20,6 +20,7 @@
 #include "clk_ctrl_os.h"
 #include "esp_private/esp_sleep_internal.h"
 #include "esp_private/periph_ctrl.h"
+#include "driver/gpio.h"
 #include "esp_private/gpio.h"
 #include "esp_private/esp_clk_tree_common.h"
 #include "esp_private/esp_gpio_reserve.h"
@@ -260,13 +261,6 @@ static esp_err_t ledc_set_timer_params(ledc_mode_t speed_mode, ledc_timer_t time
     return ESP_OK;
 }
 
-// Deprecated public API
-esp_err_t ledc_timer_set(ledc_mode_t speed_mode, ledc_timer_t timer_sel, uint32_t clock_divider, uint32_t duty_resolution,
-                         ledc_clk_src_t clk_src)
-{
-    return ledc_set_timer_params(speed_mode, timer_sel, clock_divider, duty_resolution, clk_src);
-}
-
 static IRAM_ATTR esp_err_t ledc_duty_config(ledc_mode_t speed_mode, ledc_channel_t channel, int hpoint_val,
                                             int duty_val, ledc_duty_direction_t duty_direction, uint32_t duty_num, uint32_t duty_cycle, uint32_t duty_scale)
 {
@@ -832,15 +826,13 @@ esp_err_t ledc_channel_config(const ledc_channel_config_t *ledc_conf)
     int gpio_num = ledc_conf->gpio_num;
     uint32_t ledc_channel = ledc_conf->channel;
     uint32_t timer_select = ledc_conf->timer_sel;
-    uint32_t intr_type = ledc_conf->intr_type;
     uint32_t duty = ledc_conf->duty;
     uint32_t hpoint = ledc_conf->hpoint;
     bool output_invert = ledc_conf->flags.output_invert;
     LEDC_ARG_CHECK(ledc_channel < LEDC_CHANNEL_MAX, "ledc_channel");
     LEDC_ARG_CHECK(speed_mode < LEDC_SPEED_MODE_MAX, "speed_mode");
     LEDC_ARG_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(gpio_num), "gpio_num");
     LEDC_ARG_CHECK(timer_select < LEDC_TIMER_MAX, "timer_select");
-    LEDC_ARG_CHECK(intr_type < LEDC_INTR_MAX, "intr_type");
     LEDC_ARG_CHECK(ledc_conf->sleep_mode < LEDC_SLEEP_MODE_INVALID, "sleep_mode");
 #if !SOC_LEDC_SUPPORT_SLEEP_RETENTION
     ESP_RETURN_ON_FALSE(ledc_conf->sleep_mode != LEDC_SLEEP_MODE_NO_ALIVE_ALLOW_PD, ESP_ERR_NOT_SUPPORTED, LEDC_TAG, "register back up is not supported");
@@ -881,10 +873,6 @@ esp_err_t ledc_channel_config(const ledc_channel_config_t *ledc_conf)
     ledc_update_duty(speed_mode, ledc_channel);
     /*bind the channel with the timer*/
     ledc_bind_channel_timer(speed_mode, ledc_channel, timer_select);
-    /*set interrupt type*/
-    portENTER_CRITICAL(&ledc_spinlock);
-    ledc_enable_intr_type(speed_mode, ledc_channel, intr_type);
-    portEXIT_CRITICAL(&ledc_spinlock);
     ESP_LOGD(LEDC_TAG, "LEDC_PWM CHANNEL %"PRIu32"|GPIO %02u|Duty %04"PRIu32"|Time %"PRIu32,
              ledc_channel, gpio_num, duty, timer_select);
     /*set LEDC signal in gpio matrix*/
@@ -975,7 +963,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 +986,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 +1248,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);
@@ -1534,7 +1521,7 @@ esp_err_t ledc_fade_func_install(int intr_alloc_flags)
 {
     LEDC_CHECK(s_ledc_fade_isr_handle == NULL, "fade function already installed", ESP_ERR_INVALID_STATE);
     //OR intr_alloc_flags with ESP_INTR_FLAG_IRAM because the fade isr is in IRAM
-    return ledc_isr_register(ledc_fade_isr, NULL, intr_alloc_flags | ESP_INTR_FLAG_IRAM, &s_ledc_fade_isr_handle);
+    return esp_intr_alloc(ETS_LEDC_INTR_SOURCE, intr_alloc_flags | ESP_INTR_FLAG_IRAM, ledc_fade_isr, NULL, &s_ledc_fade_isr_handle);
 }
 
 void ledc_fade_func_uninstall(void)

components/esp_driver_ledc/test_apps/ledc/main/test_ledc.c

@@ -14,14 +14,13 @@
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
 #include "unity.h"
-#include "soc/gpio_periph.h"
-#include "soc/io_mux_reg.h"
 #include "esp_system.h"
 #include "esp_timer.h"
 #include "driver/ledc.h"
 #include "soc/ledc_struct.h"
 #include "esp_clk_tree.h"
 #include "test_ledc_utils.h"
+#include "driver/gpio.h"
 
 static void fade_setup(void)
 {
@@ -91,13 +90,6 @@ TEST_CASE("LEDC channel config wrong channel", "[ledc]")
     TEST_ASSERT(ledc_channel_config(&ledc_ch_config) == ESP_ERR_INVALID_ARG);
 }
 
-TEST_CASE("LEDC channel config wrong interrupt type", "[ledc]")
-{
-    ledc_channel_config_t ledc_ch_config = initialize_channel_config();
-    ledc_ch_config.intr_type = 2;
-    TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG, ledc_channel_config(&ledc_ch_config));
-}
-
 TEST_CASE("LEDC wrong timer", "[ledc]")
 {
     ledc_channel_config_t ledc_ch_config = initialize_channel_config();
@@ -150,6 +142,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]")

components/esp_driver_ledc/test_apps/ledc/main/test_ledc_utils.c

@@ -18,7 +18,6 @@ ledc_channel_config_t initialize_channel_config(void)
     config.gpio_num = PULSE_IO;
     config.speed_mode = TEST_SPEED_MODE;
     config.channel  = LEDC_CHANNEL_0;
-    config.intr_type = LEDC_INTR_DISABLE;
     config.timer_sel = LEDC_TIMER_0;
     config.duty = 4000;
     config.hpoint = 0;

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;
 }
 
 /**

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;
 }
 
 /**

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;
 }
 
 /**

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;
 }
 
 /**

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;
 }
 
 /**