Merge branch 'refactor/gptimer_isr_logs_opt_int' into 'master'

feat(gptimer): make start and stop function idempotent, also refactored the doc structure

Closes IDFGH-11157, IDFGH-12474, IDF-12513, and IDFCI-2734

See merge request espressif/esp-idf!36983

Author: suda-morris

components/esp_adc/test_apps/adc/main/test_adc_driver_iram.c

@@ -43,7 +43,7 @@ extern void spi_flash_enable_interrupts_caches_and_other_cpu(void);
 __attribute__((unused))
 static void s_test_cache_disable_period_us(test_adc_iram_ctx_t *ctx, uint32_t period_us);
 
-#if CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM && CONFIG_GPTIMER_ISR_IRAM_SAFE
+#if CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM && CONFIG_GPTIMER_ISR_CACHE_SAFE
 /*---------------------------------------------------------------
         ADC oneshot work with cache safe ISR
 ---------------------------------------------------------------*/
@@ -140,7 +140,7 @@ TEST_CASE("ADC oneshot fast work with ISR and Flash", "[adc_oneshot]")
     TEST_ESP_OK(gptimer_del_timer(timer));
     TEST_ESP_OK(adc_oneshot_del_unit(oneshot_handle));
 }
-#endif  //#if CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM && CONFIG_GPTIMER_ISR_IRAM_SAFE
+#endif  //#if CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM && CONFIG_GPTIMER_ISR_CACHE_SAFE
 
 #if CONFIG_ADC_CONTINUOUS_ISR_IRAM_SAFE || CONFIG_GDMA_ISR_IRAM_SAFE
 #include "esp_adc/adc_continuous.h"

components/esp_adc/test_apps/adc/sdkconfig.ci.esp32c2_xtal26m_iram_safe

@@ -3,7 +3,7 @@ CONFIG_XTAL_FREQ_26=y
 
 CONFIG_COMPILER_DUMP_RTL_FILES=y
 CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM=y
-CONFIG_GPTIMER_ISR_IRAM_SAFE=y
+CONFIG_GPTIMER_ISR_CACHE_SAFE=y
 CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM=y
 CONFIG_ADC_CONTINUOUS_ISR_IRAM_SAFE=y
 CONFIG_COMPILER_OPTIMIZATION_SIZE=y

components/esp_adc/test_apps/adc/sdkconfig.ci.iram_safe

@@ -1,6 +1,6 @@
 CONFIG_COMPILER_DUMP_RTL_FILES=y
 CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM=y
-CONFIG_GPTIMER_ISR_IRAM_SAFE=y
+CONFIG_GPTIMER_ISR_CACHE_SAFE=y
 CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM=y
 CONFIG_ADC_CONTINUOUS_ISR_IRAM_SAFE=y
 CONFIG_COMPILER_OPTIMIZATION_SIZE=y

components/esp_adc/test_apps/adc/sdkconfig.ci.pm_enable

@@ -1,6 +1,6 @@
 CONFIG_COMPILER_DUMP_RTL_FILES=y
 CONFIG_ADC_ONESHOT_CTRL_FUNC_IN_IRAM=y
-CONFIG_GPTIMER_ISR_IRAM_SAFE=y
+CONFIG_GPTIMER_ISR_CACHE_SAFE=y
 CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM=y
 CONFIG_ADC_CONTINUOUS_ISR_IRAM_SAFE=y
 CONFIG_COMPILER_OPTIMIZATION_SIZE=y

components/esp_driver_gptimer/Kconfig

@@ -1,31 +1,45 @@
 menu "ESP-Driver:GPTimer Configurations"
     depends on SOC_GPTIMER_SUPPORTED
+
     config GPTIMER_ISR_HANDLER_IN_IRAM
-        bool "Place GPTimer ISR handler into IRAM"
+        bool "Place GPTimer ISR handler in IRAM to reduce latency"
         default y
+        select GPTIMER_OBJ_CACHE_SAFE
         help
-            Place GPTimer ISR handler into IRAM for better performance and fewer cache misses.
+            Place GPTimer ISR handler in IRAM to reduce latency caused by cache miss.
 
     config GPTIMER_CTRL_FUNC_IN_IRAM
-        bool "Place GPTimer control functions into IRAM"
+        bool "Place GPTimer control functions in IRAM"
         default n
+        select GPTIMER_OBJ_CACHE_SAFE
         help
-            Place GPTimer control functions (like start/stop) into IRAM,
-            so that these functions can be IRAM-safe and able to be called in the other IRAM interrupt context.
-            Enabling this option can improve driver performance as well.
+            Place GPTimer control functions (like start/stop) in IRAM, to reduce latency caused by cache miss.
+            If enabled, these functions can also be called when cache is disabled.
 
-    config GPTIMER_ISR_IRAM_SAFE
-        bool "GPTimer ISR IRAM-Safe"
+    config GPTIMER_ISR_CACHE_SAFE
+        bool "Allow GPTimer ISR to execute when cache is disabled"
         select GPTIMER_ISR_HANDLER_IN_IRAM
         default n
         help
-            Ensure the GPTimer interrupt is IRAM-Safe by allowing the interrupt handler to be
-            executable when the cache is disabled (e.g. SPI Flash write).
+            Enable this option to allow the GPTimer Interrupt Service Routine (ISR)
+            to execute even when the cache is disabled. This can be useful in scenarios where the cache
+            might be turned off, but the GPTimer functionality is still required to operate correctly.
+
+    config GPTIMER_OBJ_CACHE_SAFE
+        bool
+        default n
+        help
+            This will ensure the GPTimer object will not be allocated from a memory region
+            where its cache can be disabled.
 
     config GPTIMER_ENABLE_DEBUG_LOG
-        bool "Enable debug log"
+        bool "Force enable debug log"
         default n
         help
-            whether to enable the debug log message for GPTimer driver.
-            Note that, this option only controls the GPTimer driver log, won't affect other drivers.
+            If enabled, GPTimer component will:
+            1. ignore the global logging settings
+            2. compile all log messages into the binary
+            3. set the runtime log level to VERBOSE
+            Please enable this option by caution, as it will increase the binary size.
+
 endmenu

components/esp_driver_gptimer/include/driver/gptimer.h

@@ -135,7 +135,7 @@ esp_err_t gptimer_get_captured_count(gptimer_handle_t timer, uint64_t *value);
 /**
  * @brief Group of supported GPTimer callbacks
  * @note The callbacks are all running under ISR environment
- * @note When CONFIG_GPTIMER_ISR_IRAM_SAFE is enabled, the callback itself and functions called by it should be placed in IRAM.
+ * @note When CONFIG_GPTIMER_ISR_CACHE_SAFE is enabled, the callback itself and functions called by it should be placed in IRAM.
  */
 typedef struct {
     gptimer_alarm_cb_t on_alarm; /*!< Timer alarm callback */

components/esp_driver_gptimer/sdkconfig.rename

@@ -0,0 +1,4 @@
+# sdkconfig replacement configurations for deprecated options formatted as
+# CONFIG_DEPRECATED_OPTION CONFIG_NEW_OPTION
+
+CONFIG_GPTIMER_ISR_IRAM_SAFE   CONFIG_GPTIMER_ISR_CACHE_SAFE

components/esp_driver_gptimer/src/gptimer.c

@@ -1,27 +1,14 @@
 /*
- * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 #include <stdlib.h>
 #include <sys/lock.h>
-#include "sdkconfig.h"
-#if CONFIG_GPTIMER_ENABLE_DEBUG_LOG
-// The local log level must be defined before including esp_log.h
-// Set the maximum log level for this source file
-#define LOG_LOCAL_LEVEL ESP_LOG_DEBUG
-#endif
-#include "freertos/FreeRTOS.h"
-#include "esp_attr.h"
-#include "esp_err.h"
-#include "esp_log.h"
-#include "esp_check.h"
 #include "driver/gptimer.h"
-#include "esp_memory_utils.h"
 #include "gptimer_priv.h"
-
-static const char *TAG = "gptimer";
+#include "esp_memory_utils.h"
 
 static void gptimer_default_isr(void *args);
 
@@ -136,9 +123,6 @@ static esp_err_t gptimer_destroy(gptimer_t *timer)
 
 esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *ret_timer)
 {
-#if CONFIG_GPTIMER_ENABLE_DEBUG_LOG
-    esp_log_level_set(TAG, ESP_LOG_DEBUG);
-#endif
     esp_err_t ret = ESP_OK;
     gptimer_t *timer = NULL;
     ESP_RETURN_ON_FALSE(config && ret_timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
@@ -188,7 +172,7 @@ esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *re
     timer->direction = config->direction;
     timer->intr_priority = config->intr_priority;
     timer->flags.intr_shared = config->flags.intr_shared;
-    ESP_LOGD(TAG, "new gptimer (%d,%d) at %p, resolution=%"PRIu32"Hz", group_id, timer_id, timer, timer->resolution_hz);
+    ESP_LOGD(TAG, "new gptimer (%d,%d) at %p, %zu bytes used", group_id, timer_id, timer, heap_caps_get_allocated_size(timer));
     *ret_timer = timer;
     return ESP_OK;
 
@@ -231,7 +215,9 @@ esp_err_t gptimer_del_timer(gptimer_handle_t timer)
 
 esp_err_t gptimer_set_raw_count(gptimer_handle_t timer, unsigned long long value)
 {
-    ESP_RETURN_ON_FALSE_ISR(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    if (timer == NULL) {
+        return ESP_ERR_INVALID_ARG;
+    }
 
     portENTER_CRITICAL_SAFE(&timer->spinlock);
     timer_hal_set_counter_value(&timer->hal, value);
@@ -241,7 +227,9 @@ esp_err_t gptimer_set_raw_count(gptimer_handle_t timer, unsigned long long value
 
 esp_err_t gptimer_get_raw_count(gptimer_handle_t timer, unsigned long long *value)
 {
-    ESP_RETURN_ON_FALSE_ISR(timer && value, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    if (timer == NULL || value == NULL) {
+        return ESP_ERR_INVALID_ARG;
+    }
 
     portENTER_CRITICAL_SAFE(&timer->spinlock);
     *value = timer_hal_capture_and_get_counter_value(&timer->hal);
@@ -258,7 +246,9 @@ esp_err_t gptimer_get_resolution(gptimer_handle_t timer, uint32_t *out_resolutio
 
 esp_err_t gptimer_get_captured_count(gptimer_handle_t timer, uint64_t *value)
 {
-    ESP_RETURN_ON_FALSE_ISR(timer && value, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    if (timer == NULL || value == NULL) {
+        return ESP_ERR_INVALID_ARG;
+    }
 
     portENTER_CRITICAL_SAFE(&timer->spinlock);
     *value = timer_ll_get_counter_value(timer->hal.dev, timer->timer_id);
@@ -274,7 +264,7 @@ esp_err_t gptimer_register_event_callbacks(gptimer_handle_t timer, const gptimer
     int group_id = group->group_id;
     int timer_id = timer->timer_id;
 
-#if CONFIG_GPTIMER_ISR_IRAM_SAFE
+#if CONFIG_GPTIMER_ISR_CACHE_SAFE
     if (cbs->on_alarm) {
         ESP_RETURN_ON_FALSE(esp_ptr_in_iram(cbs->on_alarm), ESP_ERR_INVALID_ARG, TAG, "on_alarm callback not in IRAM");
     }
@@ -308,14 +298,22 @@ esp_err_t gptimer_register_event_callbacks(gptimer_handle_t timer, const gptimer
 
 esp_err_t gptimer_set_alarm_action(gptimer_handle_t timer, const gptimer_alarm_config_t *config)
 {
-    ESP_RETURN_ON_FALSE_ISR(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    if (timer == NULL) {
+        return ESP_ERR_INVALID_ARG;
+    }
     if (config) {
 #if CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM
-        ESP_RETURN_ON_FALSE_ISR(esp_ptr_internal(config), ESP_ERR_INVALID_ARG, TAG, "alarm config struct not in internal RAM");
+        // when the function is placed in IRAM, we expect the config struct is also placed in internal RAM
+        // if the cache is disabled, the function can still access the config struct
+        if (esp_ptr_internal(config) == false) {
+            return ESP_ERR_INVALID_ARG;
+        }
 #endif
         // When auto_reload is enabled, alarm_count should not be equal to reload_count
         bool valid_auto_reload = !config->flags.auto_reload_on_alarm || config->alarm_count != config->reload_count;
-        ESP_RETURN_ON_FALSE_ISR(valid_auto_reload, ESP_ERR_INVALID_ARG, TAG, "reload count can't equal to alarm count");
+        if (valid_auto_reload == false) {
+            return ESP_ERR_INVALID_ARG;
+        }
 
         portENTER_CRITICAL_SAFE(&timer->spinlock);
         timer->reload_count = config->reload_count;
@@ -343,6 +341,7 @@ esp_err_t gptimer_set_alarm_action(gptimer_handle_t timer, const gptimer_alarm_c
 esp_err_t gptimer_enable(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    // the only acceptable FSM change: init->enable
     gptimer_fsm_t expected_fsm = GPTIMER_FSM_INIT;
     ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_ENABLE),
                         ESP_ERR_INVALID_STATE, TAG, "timer not in init state");
@@ -363,6 +362,7 @@ esp_err_t gptimer_enable(gptimer_handle_t timer)
 esp_err_t gptimer_disable(gptimer_handle_t timer)
 {
     ESP_RETURN_ON_FALSE(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    // the only acceptable FSM change: enable->init
     gptimer_fsm_t expected_fsm = GPTIMER_FSM_ENABLE;
     ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_INIT),
                         ESP_ERR_INVALID_STATE, TAG, "timer not in enable state");
@@ -382,7 +382,14 @@ esp_err_t gptimer_disable(gptimer_handle_t timer)
 
 esp_err_t gptimer_start(gptimer_handle_t timer)
 {
-    ESP_RETURN_ON_FALSE_ISR(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    if (timer == NULL) {
+        return ESP_ERR_INVALID_ARG;
+    }
+
+    // if the timer is already started, do nothing
+    if (atomic_load(&timer->fsm) == GPTIMER_FSM_RUN) {
+        return ESP_OK;
+    }
 
     gptimer_fsm_t expected_fsm = GPTIMER_FSM_ENABLE;
     if (atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_RUN_WAIT)) {
@@ -396,15 +403,24 @@ esp_err_t gptimer_start(gptimer_handle_t timer)
         atomic_store(&timer->fsm, GPTIMER_FSM_RUN);
         portEXIT_CRITICAL_SAFE(&timer->spinlock);
     } else {
-        ESP_RETURN_ON_FALSE_ISR(false, ESP_ERR_INVALID_STATE, TAG, "timer is not ready for a new start");
+        // return error if the timer is not in the expected state
+        return ESP_ERR_INVALID_STATE;
     }
 
     return ESP_OK;
 }
 
 esp_err_t gptimer_stop(gptimer_handle_t timer)
 {
-    ESP_RETURN_ON_FALSE_ISR(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    if (timer == NULL) {
+        // not printing error message here because the return value already indicates the error well
+        return ESP_ERR_INVALID_ARG;
+    }
+
+    // if the timer is not started, do nothing
+    if (atomic_load(&timer->fsm) == GPTIMER_FSM_ENABLE) {
+        return ESP_OK;
+    }
 
     gptimer_fsm_t expected_fsm = GPTIMER_FSM_RUN;
     if (atomic_compare_exchange_strong(&timer->fsm, &expected_fsm, GPTIMER_FSM_ENABLE_WAIT)) {
@@ -415,7 +431,8 @@ esp_err_t gptimer_stop(gptimer_handle_t timer)
         atomic_store(&timer->fsm, GPTIMER_FSM_ENABLE);
         portEXIT_CRITICAL_SAFE(&timer->spinlock);
     } else {
-        ESP_RETURN_ON_FALSE_ISR(false, ESP_ERR_INVALID_STATE, TAG, "timer is not running");
+        // return error if the timer is not in the expected state
+        return ESP_ERR_INVALID_STATE;
     }
 
     return ESP_OK;

components/esp_driver_gptimer/src/gptimer_common.c

@@ -1,18 +1,14 @@
 /*
- * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2025 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 #include <sys/lock.h>
-#include "esp_check.h"
 #include "esp_clk_tree.h"
-#include "esp_private/esp_clk_tree_common.h"
 #include "esp_private/gptimer.h"
 #include "gptimer_priv.h"
-#include "soc/soc_caps.h"
-
-static const char *TAG = "gptimer";
+#include "esp_private/esp_clk_tree_common.h"
 
 typedef struct gptimer_platform_t {
     _lock_t mutex;                             // platform level mutex lock
@@ -182,3 +178,11 @@ int gptimer_get_group_id(gptimer_handle_t timer, int *group_id)
     *group_id = timer->group->group_id;
     return ESP_OK;
 }
+
+#if CONFIG_GPTIMER_ENABLE_DEBUG_LOG
+__attribute__((constructor))
+static void gptimer_override_default_log_level(void)
+{
+    esp_log_level_set(TAG, ESP_LOG_VERBOSE);
+}
+#endif