From 61282cc5dd14d2f74868384067820a55e1deddd0 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 ------------- tools/mocks/hal/include/hal/gpio_types.h | 13 ------------- 5 files changed, 45 insertions(+), 44 deletions(-) diff --git a/components/driver/gpio/gpio.c b/components/driver/gpio/gpio.c index 4bb6c3689f..f5644c3917 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/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