From 3017b65d4db7bd02a97ee8904a947e4a90d5a2f9 Mon Sep 17 00:00:00 2001 From: Song Ruo Jing Date: Wed, 24 Aug 2022 17:06:20 +0800 Subject: [PATCH] gpio: Fix interrupt lost issue In previous gpio default isr, interrupt status bits get cleared at the exit of the isr. However, for edge-triggered interrupt type, the interrupt status bit should be cleared before entering the per-pin handlers to avoid any potential interrupt lost. Closes https://github.com/espressif/esp-idf/pull/6853 --- components/driver/gpio/gpio.c | 22 ++++++++++++++++--- components/driver/include/driver/gpio.h | 20 ++++++++++++++++- components/hal/include/hal/gpio_hal.h | 21 ++++++------------ components/hal/include/hal/gpio_types.h | 13 ----------- .../release-5.x/peripherals.rst | 8 ++++++- tools/mocks/hal/include/hal/gpio_types.h | 13 ----------- 6 files changed, 52 insertions(+), 45 deletions(-) diff --git a/components/driver/gpio/gpio.c b/components/driver/gpio/gpio.c index 4ced5e8baf..138e9b159d 100644 --- a/components/driver/gpio/gpio.c +++ b/components/driver/gpio/gpio.c @@ -53,6 +53,7 @@ typedef struct { uint32_t isr_core_id; gpio_isr_func_t *gpio_isr_func; gpio_isr_handle_t gpio_isr_handle; + uint64_t isr_clr_on_entry_mask; // for edge-triggered interrupts, interrupt status bits should be cleared before entering per-pin handlers } gpio_context_t; static gpio_hal_context_t _gpio_hal = { @@ -64,6 +65,7 @@ static gpio_context_t gpio_context = { .gpio_spinlock = portMUX_INITIALIZER_UNLOCKED, .isr_core_id = GPIO_ISR_CORE_ID_UNINIT, .gpio_isr_func = NULL, + .isr_clr_on_entry_mask = 0, }; esp_err_t gpio_pullup_en(gpio_num_t gpio_num) @@ -149,13 +151,17 @@ esp_err_t gpio_set_intr_type(gpio_num_t gpio_num, gpio_int_type_t intr_type) portENTER_CRITICAL(&gpio_context.gpio_spinlock); gpio_hal_set_intr_type(gpio_context.gpio_hal, gpio_num, intr_type); + if (intr_type == GPIO_INTR_POSEDGE || intr_type == GPIO_INTR_NEGEDGE || intr_type == GPIO_INTR_ANYEDGE) { + gpio_context.isr_clr_on_entry_mask |= (1ULL << (gpio_num)); + } else { + gpio_context.isr_clr_on_entry_mask &= ~(1ULL << (gpio_num)); + } portEXIT_CRITICAL(&gpio_context.gpio_spinlock); return ESP_OK; } static esp_err_t gpio_intr_enable_on_core(gpio_num_t gpio_num, uint32_t core_id) { - GPIO_CHECK(GPIO_IS_VALID_GPIO(gpio_num), "GPIO number error", ESP_ERR_INVALID_ARG); gpio_hal_intr_enable_on_core(gpio_context.gpio_hal, gpio_num, core_id); return ESP_OK; } @@ -412,9 +418,21 @@ static inline void IRAM_ATTR gpio_isr_loop(uint32_t status, const uint32_t gpio_ status &= ~(1 << nbit); int gpio_num = gpio_num_start + nbit; + bool intr_status_bit_cleared = false; + // Edge-triggered type interrupt can clear the interrupt status bit before entering per-pin interrupt handler + if ((1ULL << (gpio_num)) & gpio_context.isr_clr_on_entry_mask) { + intr_status_bit_cleared = true; + gpio_hal_clear_intr_status_bit(gpio_context.gpio_hal, gpio_num); + } + if (gpio_context.gpio_isr_func[gpio_num].fn != NULL) { gpio_context.gpio_isr_func[gpio_num].fn(gpio_context.gpio_isr_func[gpio_num].args); } + + // If the interrupt status bit was not cleared at the entry, then must clear it before exiting + if (!intr_status_bit_cleared) { + gpio_hal_clear_intr_status_bit(gpio_context.gpio_hal, gpio_num); + } } } @@ -431,7 +449,6 @@ static void IRAM_ATTR gpio_intr_service(void *arg) if (gpio_intr_status) { gpio_isr_loop(gpio_intr_status, 0); - gpio_hal_clear_intr_status(gpio_context.gpio_hal, gpio_intr_status); } //read status1 to get interrupt status for GPIO32-39 @@ -440,7 +457,6 @@ static void IRAM_ATTR gpio_intr_service(void *arg) if (gpio_intr_status_h) { gpio_isr_loop(gpio_intr_status_h, 32); - gpio_hal_clear_intr_status_high(gpio_context.gpio_hal, gpio_intr_status_h); } } diff --git a/components/driver/include/driver/gpio.h b/components/driver/include/driver/gpio.h index d91a56497e..7a0c749169 100644 --- a/components/driver/include/driver/gpio.h +++ b/components/driver/include/driver/gpio.h @@ -29,6 +29,24 @@ extern "C" { typedef intr_handle_t gpio_isr_handle_t; +/** + * @brief GPIO interrupt handler + * + * @param arg User registered data + */ +typedef void (*gpio_isr_t)(void *arg); + +/** + * @brief Configuration parameters of GPIO pad for gpio_config function + */ +typedef struct { + uint64_t pin_bit_mask; /*!< GPIO pin: set with bit mask, each bit maps to a GPIO */ + gpio_mode_t mode; /*!< GPIO mode: set input/output mode */ + gpio_pullup_t pull_up_en; /*!< GPIO pull-up */ + gpio_pulldown_t pull_down_en; /*!< GPIO pull-down */ + gpio_int_type_t intr_type; /*!< GPIO interrupt type */ +} gpio_config_t; + /** * @brief GPIO common configuration * @@ -255,7 +273,7 @@ esp_err_t gpio_pulldown_en(gpio_num_t gpio_num); esp_err_t gpio_pulldown_dis(gpio_num_t gpio_num); /** - * @brief Install the driver's GPIO ISR handler service, which allows per-pin GPIO interrupt handlers. + * @brief Install the GPIO driver's ETS_GPIO_INTR_SOURCE ISR handler service, which allows per-pin GPIO interrupt handlers. * * This function is incompatible with gpio_isr_register() - if that function is used, a single global ISR is registered for all GPIO interrupts. If this function is used, the ISR service provides a global GPIO ISR and individual pin handlers are registered via the gpio_isr_handler_add() function. * diff --git a/components/hal/include/hal/gpio_hal.h b/components/hal/include/hal/gpio_hal.h index 78c4d92424..e90bfb4d6b 100644 --- a/components/hal/include/hal/gpio_hal.h +++ b/components/hal/include/hal/gpio_hal.h @@ -95,20 +95,13 @@ typedef struct { #define gpio_hal_get_intr_status_high(hal, core_id, status) gpio_ll_get_intr_status_high((hal)->dev, core_id, status) /** - * @brief Clear GPIO interrupt status - * - * @param hal Context of the HAL layer - * @param mask interrupt status clear mask - */ -#define gpio_hal_clear_intr_status(hal, mask) gpio_ll_clear_intr_status((hal)->dev, mask) - -/** - * @brief Clear GPIO interrupt status high - * - * @param hal Context of the HAL layer - * @param mask interrupt status high clear mask - */ -#define gpio_hal_clear_intr_status_high(hal, mask) gpio_ll_clear_intr_status_high((hal)->dev, mask) + * @brief Clear GPIO interrupt status bit + * + * @param hal Context of the HAL layer + * @param gpio_num GPIO number. If you want to clear the interrupt status bit of e.g. GPIO16, gpio_num should be GPIO_NUM_16 (16); + */ +#define gpio_hal_clear_intr_status_bit(hal, gpio_num) (((gpio_num) < 32) ? gpio_ll_clear_intr_status((hal)->dev, 1 << gpio_num) \ + : gpio_ll_clear_intr_status_high((hal)->dev, 1 << (gpio_num - 32))) /** * @brief Enable GPIO module interrupt signal diff --git a/components/hal/include/hal/gpio_types.h b/components/hal/include/hal/gpio_types.h index dbc2208a07..a85e8f7ef0 100644 --- a/components/hal/include/hal/gpio_types.h +++ b/components/hal/include/hal/gpio_types.h @@ -386,17 +386,6 @@ typedef enum { GPIO_PULLDOWN_ENABLE = 0x1, /*!< Enable GPIO pull-down resistor */ } gpio_pulldown_t; -/** - * @brief Configuration parameters of GPIO pad for gpio_config function - */ -typedef struct { - uint64_t pin_bit_mask; /*!< GPIO pin: set with bit mask, each bit maps to a GPIO */ - gpio_mode_t mode; /*!< GPIO mode: set input/output mode */ - gpio_pullup_t pull_up_en; /*!< GPIO pull-up */ - gpio_pulldown_t pull_down_en; /*!< GPIO pull-down */ - gpio_int_type_t intr_type; /*!< GPIO interrupt type */ -} gpio_config_t; - typedef enum { GPIO_PULLUP_ONLY, /*!< Pad pull up */ GPIO_PULLDOWN_ONLY, /*!< Pad pull down */ @@ -413,8 +402,6 @@ typedef enum { GPIO_DRIVE_CAP_MAX, } gpio_drive_cap_t; -typedef void (*gpio_isr_t)(void *); - #ifdef __cplusplus } #endif diff --git a/docs/en/migration-guides/release-5.x/peripherals.rst b/docs/en/migration-guides/release-5.x/peripherals.rst index 84c04d27ae..31911b7134 100644 --- a/docs/en/migration-guides/release-5.x/peripherals.rst +++ b/docs/en/migration-guides/release-5.x/peripherals.rst @@ -31,7 +31,13 @@ ADC GPIO ---- -The previous Kconfig option `RTCIO_SUPPORT_RTC_GPIO_DESC` has been removed, thus the ``rtc_gpio_desc`` array is unavailable. Please use ``rtc_io_desc`` array instead. +- The previous Kconfig option `RTCIO_SUPPORT_RTC_GPIO_DESC` has been removed, thus the ``rtc_gpio_desc`` array is unavailable. Please use ``rtc_io_desc`` array instead. + +- GPIO interrupt users callbacks should no longer read the GPIO interrupt status register to get the triggered GPIO's pin number. Users should use the callback argument to determine the GPIO's pin number instead. + + - Previously, when a GPIO interrupt occurs, the GPIO's interrupt status register is cleared after calling the user callbacks. Thus, it was possible for users to read the GPIO's interrupt status register inside the callback to determine which GPIO was triggered. + - However, clearing the interrupt status after the user callbacks can potentially cause edge-triggered interrupts to be lost. For example, if an edge-triggered interrupt (re)triggers while the user callbacks are being called, that interrupt will be cleared without its registered user callback being handled. + - Now, the GPIO's interrupt status register is cleared **before** invoking the user callbacks. Thus, users can no longer read the GPIO interrupt status register to determine which pin has triggered the interrupt. Instead, users should use the callback argument to pass the pin number. .. only:: SOC_SDM_SUPPORTED diff --git a/tools/mocks/hal/include/hal/gpio_types.h b/tools/mocks/hal/include/hal/gpio_types.h index 29797e0dea..9311b611f4 100644 --- a/tools/mocks/hal/include/hal/gpio_types.h +++ b/tools/mocks/hal/include/hal/gpio_types.h @@ -95,17 +95,6 @@ typedef enum { GPIO_PULLDOWN_ENABLE = 0x1, /*!< Enable GPIO pull-down resistor */ } gpio_pulldown_t; -/** - * @brief Configuration parameters of GPIO pad for gpio_config function - */ -typedef struct { - uint64_t pin_bit_mask; /*!< GPIO pin: set with bit mask, each bit maps to a GPIO */ - gpio_mode_t mode; /*!< GPIO mode: set input/output mode */ - gpio_pullup_t pull_up_en; /*!< GPIO pull-up */ - gpio_pulldown_t pull_down_en; /*!< GPIO pull-down */ - gpio_int_type_t intr_type; /*!< GPIO interrupt type */ -} gpio_config_t; - typedef enum { GPIO_PULLUP_ONLY, /*!< Pad pull up */ GPIO_PULLDOWN_ONLY, /*!< Pad pull down */ @@ -122,8 +111,6 @@ typedef enum { GPIO_DRIVE_CAP_MAX, } gpio_drive_cap_t; -typedef void (*gpio_isr_t)(void *); - #ifdef __cplusplus } #endif