Merge branch 'bugfix/ledc_max_duty_cycle' into 'master'

fix(ledc): fix ledc driver 100% duty cycle configuration

Closes IDFGH-10881 and IDFGH-10254

See merge request espressif/esp-idf!26067

components/driver/CMakeLists.txt

@@ -125,6 +125,8 @@ endif()
 # LEDC related source files
 if(CONFIG_SOC_LEDC_SUPPORTED)
     list(APPEND srcs "ledc/ledc.c")
+
+    list(APPEND ldfragments "ledc/linker.lf")
 endif()
 
 # MCPWM related source files

components/driver/Kconfig

@@ -409,15 +409,7 @@ menu "Driver Configurations"
 
     endmenu # Parallel IO Configuration
 
-    menu "LEDC Configuration"
-        config LEDC_CTRL_FUNC_IN_IRAM
-            bool "Place LEDC control functions into IRAM"
-            default n
-            help
-                Place LEDC control functions (ledc_update_duty and ledc_stop) into IRAM,
-                so that these functions can be IRAM-safe and able to be called in an IRAM context.
-                Enabling this option can improve driver performance as well.
-    endmenu # LEDC Configuration
+    orsource "./ledc/Kconfig.ledc"
 
     menu "I2C Configuration"
         config I2C_ISR_IRAM_SAFE

components/driver/ledc/Kconfig.ledc

@@ -0,0 +1,11 @@
+menu "LEDC Configuration"
+
+    config LEDC_CTRL_FUNC_IN_IRAM
+        bool "Place LEDC control functions into IRAM"
+        default n
+        help
+            Place LEDC control functions (ledc_update_duty and ledc_stop) into IRAM,
+            so that these functions can be IRAM-safe and able to be called in an IRAM context.
+            Enabling this option can improve driver performance as well.
+
+endmenu # LEDC Configuration

components/driver/ledc/ledc.c

@@ -7,6 +7,7 @@
 #include "esp_types.h"
 #include "freertos/FreeRTOS.h"
 #include "freertos/semphr.h"
+#include "freertos/idf_additions.h"
 #include "esp_log.h"
 #include "esp_check.h"
 #include "soc/gpio_periph.h"
@@ -58,9 +59,6 @@ typedef struct {
     ledc_fade_mode_t mode;
     SemaphoreHandle_t ledc_fade_sem;
     SemaphoreHandle_t ledc_fade_mux;
-#if CONFIG_SPIRAM_USE_MALLOC
-    StaticQueue_t ledc_fade_sem_storage;
-#endif
     ledc_cb_t ledc_fade_callback;
     void *cb_user_arg;
     volatile ledc_fade_fsm_t fsm;
@@ -672,8 +670,13 @@ esp_err_t ledc_channel_config(const ledc_channel_config_t *ledc_conf)
 #endif
 
     /*set channel parameters*/
-    /*   channel parameters decide how the waveform looks like in one period*/
-    /*   set channel duty and hpoint value, duty range is (0 ~ ((2 ** duty_resolution) - 1)), max hpoint value is 0xfffff*/
+    /*   channel parameters decide how the waveform looks like in one period */
+    /*   set channel duty and hpoint value, duty range is [0, (2**duty_res)], hpoint range is [0, (2**duty_res)-1] */
+    /*   Note: On ESP32, ESP32S2, ESP32S3, ESP32C3, ESP32C2, ESP32C6, ESP32H2, ESP32P4, due to a hardware bug,
+     *         100% duty cycle (i.e. 2**duty_res) is not reachable when the binded timer selects the maximum duty
+     *         resolution. For example, the max duty resolution on ESP32C3 is 14-bit width, then set duty to (2**14)
+     *         will mess up the duty calculation in hardware.
+    */
     ledc_set_duty_with_hpoint(speed_mode, ledc_channel, duty, hpoint);
     /*update duty settings*/
     ledc_update_duty(speed_mode, ledc_channel);
@@ -859,7 +862,7 @@ static inline void IRAM_ATTR ledc_calc_fade_end_channel(uint32_t *fade_end_statu
     *channel = i;
 }
 
-void IRAM_ATTR ledc_fade_isr(void *arg)
+static void IRAM_ATTR ledc_fade_isr(void *arg)
 {
     bool cb_yield = false;
     BaseType_t HPTaskAwoken = pdFALSE;
@@ -987,7 +990,7 @@ static esp_err_t ledc_fade_channel_deinit(ledc_mode_t speed_mode, ledc_channel_t
             s_ledc_fade_rec[speed_mode][channel]->ledc_fade_mux = NULL;
         }
         if (s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem) {
-            vSemaphoreDelete(s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem);
+            vSemaphoreDeleteWithCaps(s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem);
             s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem = NULL;
         }
         free(s_ledc_fade_rec[speed_mode][channel]);
@@ -1003,23 +1006,13 @@ static esp_err_t ledc_fade_channel_init_check(ledc_mode_t speed_mode, ledc_chann
         return ESP_FAIL;
     }
     if (s_ledc_fade_rec[speed_mode][channel] == NULL) {
-#if CONFIG_SPIRAM_USE_MALLOC
+        // Always malloc internally since LEDC ISR is always placed in IRAM
         s_ledc_fade_rec[speed_mode][channel] = (ledc_fade_t *) heap_caps_calloc(1, sizeof(ledc_fade_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
         if (s_ledc_fade_rec[speed_mode][channel] == NULL) {
             ledc_fade_channel_deinit(speed_mode, channel);
             return ESP_ERR_NO_MEM;
         }
-
-        memset(&s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem_storage, 0, sizeof(StaticQueue_t));
-        s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem = xSemaphoreCreateBinaryStatic(&s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem_storage);
-#else
-        s_ledc_fade_rec[speed_mode][channel] = (ledc_fade_t *) calloc(1, sizeof(ledc_fade_t));
-        if (s_ledc_fade_rec[speed_mode][channel] == NULL) {
-            ledc_fade_channel_deinit(speed_mode, channel);
-            return ESP_ERR_NO_MEM;
-        }
-        s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem = xSemaphoreCreateBinary();
-#endif
+        s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem = xSemaphoreCreateBinaryWithCaps(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
         s_ledc_fade_rec[speed_mode][channel]->ledc_fade_mux = xSemaphoreCreateMutex();
         xSemaphoreGive(s_ledc_fade_rec[speed_mode][channel]->ledc_fade_sem);
         s_ledc_fade_rec[speed_mode][channel]->fsm = LEDC_FSM_IDLE;
@@ -1070,8 +1063,9 @@ static esp_err_t _ledc_set_fade_with_step(ledc_mode_t speed_mode, ledc_channel_t
         ESP_LOGD(LEDC_TAG, "cur duty: %"PRIu32"; target: %"PRIu32", step: %d, cycle: %d; scale: %d; dir: %d",
                  duty_cur, target_duty, step_num, cycle_num, scale, dir);
     } else {
+        // Directly set duty to the target, does not care on the dir
         portENTER_CRITICAL(&ledc_spinlock);
-        ledc_duty_config(speed_mode, channel, LEDC_VAL_NO_CHANGE, target_duty, dir, 1, 1, 0);
+        ledc_duty_config(speed_mode, channel, LEDC_VAL_NO_CHANGE, target_duty, 1, 1, 1, 0);
         portEXIT_CRITICAL(&ledc_spinlock);
         ESP_LOGD(LEDC_TAG, "Set to target duty: %"PRIu32, target_duty);
     }
@@ -1231,6 +1225,7 @@ esp_err_t ledc_fade_stop(ledc_mode_t speed_mode, ledc_channel_t channel)
 
 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);
 }

components/driver/ledc/linker.lf

@@ -0,0 +1,12 @@
+[mapping:ledc_driver]
+archive: libdriver.a
+entries:
+    if LEDC_CTRL_FUNC_IN_IRAM = y:
+        ledc: ledc_stop (noflash)
+        ledc: ledc_update_duty (noflash)
+        ledc: _ledc_update_duty (noflash)
+
+[mapping:ledc_hal]
+archive: libhal.a
+entries:
+    ledc_hal_iram (noflash)

components/driver/linker.lf

@@ -15,7 +15,3 @@ entries:
     if DAC_CTRL_FUNC_IN_IRAM = y:
         dac_oneshot: dac_oneshot_output_voltage (noflash)
         dac_continuous: dac_continuous_write_asynchronously (noflash)
-    if LEDC_CTRL_FUNC_IN_IRAM = y:
-        ledc: ledc_stop (noflash)
-        ledc: ledc_update_duty (noflash)
-        ledc: _ledc_update_duty (noflash)

components/driver/test_apps/ledc/main/CMakeLists.txt

@@ -4,5 +4,5 @@ set(srcs "test_app_main.c"
 # In order for the cases defined by `TEST_CASE` to be linked into the final elf,
 # the component can be registered as WHOLE_ARCHIVE
 idf_component_register(SRCS ${srcs}
-                       PRIV_REQUIRES unity driver esp_timer
+                       PRIV_REQUIRES unity driver esp_timer esp_psram
                        WHOLE_ARCHIVE)

components/driver/test_apps/ledc/pytest_ledc.py

@@ -1,11 +1,12 @@
-# SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD
 # SPDX-License-Identifier: CC0-1.0
 
 import pytest
 from pytest_embedded_idf import IdfDut
 
 
 @pytest.mark.supported_targets
+@pytest.mark.temp_skip_ci(targets=['esp32s3'], reason='skip due to duplication with test_ledc_psram')
 @pytest.mark.generic
 @pytest.mark.parametrize(
     'config',
@@ -17,3 +18,17 @@
 )
 def test_ledc(dut: IdfDut) -> None:
     dut.run_all_single_board_cases()
+
+
+@pytest.mark.esp32s3
+@pytest.mark.octal_psram
+@pytest.mark.parametrize(
+    'config',
+    [
+        'iram_safe',
+        'release',
+    ],
+    indirect=True,
+)
+def test_ledc_psram(dut: IdfDut) -> None:
+    dut.run_all_single_board_cases()

components/driver/test_apps/ledc/sdkconfig.defaults

@@ -1,4 +1,4 @@
 CONFIG_FREERTOS_HZ=1000
-CONFIG_ESP_TASK_WDT=n
+CONFIG_ESP_TASK_WDT_INIT=n
 # Disable memory protection, because "LEDC continue work after software reset" test case requires a cpu reset
 CONFIG_ESP_SYSTEM_MEMPROT_FEATURE=n

components/driver/test_apps/ledc/sdkconfig.defaults.esp32s3

@@ -0,0 +1,4 @@
+CONFIG_SPIRAM=y
+CONFIG_SPIRAM_MODE_OCT=y
+CONFIG_SPIRAM_SPEED_80M=y
+CONFIG_SPIRAM_MALLOC_ALWAYSINTERNAL=0

components/hal/linker.lf

@@ -14,8 +14,6 @@ entries:
             spi_hal_iram (noflash)
         if HAL_SPI_SLAVE_FUNC_IN_IRAM = y:
             spi_slave_hal_iram (noflash)
-    if SOC_LEDC_SUPPORTED = y:
-        ledc_hal_iram (noflash)
     if SOC_I2C_SUPPORTED = y:
         i2c_hal_iram (noflash)
     if HAL_WDT_USE_ROM_IMPL = n:

docs/en/api-reference/peripherals/ledc.rst

@@ -246,7 +246,13 @@ To set the duty cycle, use the dedicated function :cpp:func:`ledc_set_duty`. Aft
 
 Another way to set the duty cycle, as well as some other channel parameters, is by calling :cpp:func:`ledc_channel_config` covered in Section :ref:`ledc-api-configure-channel`.
 
-The range of the duty cycle values passed to functions depends on selected ``duty_resolution`` and should be from ``0`` to ``(2 ** duty_resolution) - 1``. For example, if the selected duty resolution is 10, then the duty cycle values can range from 0 to 1023. This provides the resolution of ~ 0.1%.
+The range of the duty cycle values passed to functions depends on selected ``duty_resolution`` and should be from ``0`` to ``(2 ** duty_resolution)``. For example, if the selected duty resolution is 10, then the duty cycle values can range from 0 to 1024. This provides the resolution of ~ 0.1%.
+
+.. only:: esp32 or esp32s2 or esp32s3 or esp32c3 or esp32c2 or esp32c6 or esp32h2 or esp32p4
+
+    .. warning::
+
+        On {IDF_TARGET_NAME}, when channel's binded timer selects its maximum duty resolution, the duty cycle value cannot be set to ``(2 ** duty_resolution)``. Otherwise, the internal duty counter in the hardware will overflow and be messed up.
 
 
 Change PWM Duty Cycle Using Hardware

docs/zh_CN/api-reference/peripherals/ledc.rst

@@ -246,7 +246,13 @@ LED PWM 控制器可在无需 CPU 干预的情况下自动改变占空比，实
 
 另外一种设置占空比和其他通道参数的方式是调用 :ref:`ledc-api-configure-channel` 一节提到的函数 :cpp:func:`ledc_channel_config`。
 
-传递给函数的占空比数值范围取决于选定的 ``duty_resolution``，应为 ``0`` 至 ``(2 ** duty_resolution) - 1``。例如，如选定的占空比分辨率为 10，则占空比的数值范围为 0 至 1023。此时分辨率为 ~ 0.1%。
+传递给函数的占空比数值范围取决于选定的 ``duty_resolution``，应为 ``0`` 至 ``(2 ** duty_resolution)``。例如，如选定的占空比分辨率为 10，则占空比的数值范围为 0 至 1024。此时分辨率为 ~ 0.1%。
+
+.. only:: esp32 or esp32s2 or esp32s3 or esp32c3 or esp32c2 or esp32c6 or esp32h2 or esp32p4
+
+    .. warning::
+
+        在 {IDF_TARGET_NAME} 上，当通道绑定的定时器配置了其最大 PWM 占空比分辨率（ ``MAX_DUTY_RES`` ），通道的占空比不能被设置到 ``(2 ** MAX_DUTY_RES)`` 。否则，硬件内部占空比计数器会溢出，并导致占空比计算错误。
 
 
 使用硬件改变 PWM 占空比

examples/peripherals/ledc/ledc_basic/README.md

@@ -27,26 +27,26 @@ The example uses fixed PWM frequency of 5 kHz, duty cycle in 50%, and output GPI
 
 Depending on the selected `LEDC_FREQUENCY`, you will need to change the `LEDC_DUTY_RES`.
 
-To dinamicaly set the duty and frequency, you can use the following functions:
+To dynamically set the duty and frequency, you can use the following functions:
 
 To set the frequency to 2.5 kHZ i.e:
 
 ```c
 ledc_set_freq(LEDC_MODE, LEDC_TIMER, 2500);
 ```
 
-Now the duty to 100% i.e:
+Now set the duty to 100% i.e:
 
 ```c
-ledc_set_duty(LEDC_MODE, LEDC_CHANNEL, 8191);
+ledc_set_duty(LEDC_MODE, LEDC_CHANNEL, 8192);
 ledc_update_duty(LEDC_MODE, LEDC_CHANNEL);
 ```
 
 To change the duty cycle you need to calculate the duty range according to the duty resolution.
 
 If duty resolution is 13 bits:
 
-Duty range: `0 to (2 ** 13) - 1 = 8191` where 0 is 0% and 8191 is 100%.
+Duty range: `0 to (2 ** 13) = 8191` where 0 is 0% and 8192 is 100%.
 
 ### Build and Flash
 

examples/peripherals/ledc/ledc_basic/main/ledc_basic_example_main.c

@@ -15,9 +15,15 @@
 #define LEDC_OUTPUT_IO          (5) // Define the output GPIO
 #define LEDC_CHANNEL            LEDC_CHANNEL_0
 #define LEDC_DUTY_RES           LEDC_TIMER_13_BIT // Set duty resolution to 13 bits
-#define LEDC_DUTY               (4095) // Set duty to 50%. ((2 ** 13) - 1) * 50% = 4095
+#define LEDC_DUTY               (4096) // Set duty to 50%. (2 ** 13) * 50% = 4096
 #define LEDC_FREQUENCY          (5000) // Frequency in Hertz. Set frequency at 5 kHz
 
+/* Warning:
+ * For ESP32, ESP32S2, ESP32S3, ESP32C3, ESP32C2, ESP32C6, ESP32H2, ESP32P4 targets,
+ * when LEDC_DUTY_RES selects the maximum duty resolution (i.e. value equal to SOC_LEDC_TIMER_BIT_WIDTH),
+ * 100% duty cycle is not reachable (duty cannot be set to (2 ** SOC_LEDC_TIMER_BIT_WIDTH)).
+ */
+
 static void example_ledc_init(void)
 {
     // Prepare and then apply the LEDC PWM timer configuration