From 64f81aefaedcf31779f1366bea04cd625b79a95e Mon Sep 17 00:00:00 2001 From: kooho <2229179028@qq.com> Date: Thu, 18 Jul 2019 11:34:49 +0800 Subject: [PATCH] bugfix(GPIO): Fixed the bug that GPIO enables interrupts on one core, but registers interrupt service routines on another core closes https://github.com/espressif/esp-idf/issues/2808 closes https://github.com/espressif/esp-idf/issues/2845 --- components/driver/gpio.c | 56 +++++++++++-- components/driver/test/test_gpio.c | 125 +++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 8 deletions(-) diff --git a/components/driver/gpio.c b/components/driver/gpio.c index 2cb6512c52..f9ae07c370 100644 --- a/components/driver/gpio.c +++ b/components/driver/gpio.c @@ -20,21 +20,34 @@ #include "soc/soc.h" #include "soc/gpio_periph.h" #include "esp_log.h" +#include "esp_ipc.h" -static const char* GPIO_TAG = "gpio"; #define GPIO_CHECK(a, str, ret_val) \ if (!(a)) { \ ESP_LOGE(GPIO_TAG,"%s(%d): %s", __FUNCTION__, __LINE__, str); \ return (ret_val); \ } +#define GPIO_ISR_CORE_ID_UNINIT (3) typedef struct { gpio_isr_t fn; /*!< isr function */ void* args; /*!< isr function args */ } gpio_isr_func_t; +// Used by the IPC call to register the interrupt service routine. +typedef struct { + int source; /*!< ISR source */ + int intr_alloc_flags; /*!< ISR alloc flag */ + void (*fn)(void*); /*!< ISR function */ + void *arg; /*!< ISR function args*/ + void *handle; /*!< ISR handle */ + esp_err_t ret; +} gpio_isr_alloc_t; + +static const char* GPIO_TAG = "gpio"; static gpio_isr_func_t* gpio_isr_func = NULL; static gpio_isr_handle_t gpio_isr_handle; +static uint32_t isr_core_id = GPIO_ISR_CORE_ID_UNINIT; static portMUX_TYPE gpio_spinlock = portMUX_INITIALIZER_UNLOCKED; esp_err_t gpio_pullup_en(gpio_num_t gpio_num) @@ -100,7 +113,6 @@ static void gpio_intr_status_clr(gpio_num_t gpio_num) 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_intr_status_clr(gpio_num); if (core_id == 0) { GPIO.pin[gpio_num].int_ena = GPIO_PRO_CPU_INTR_ENA; //enable pro cpu intr @@ -112,7 +124,13 @@ static esp_err_t gpio_intr_enable_on_core (gpio_num_t gpio_num, uint32_t core_id esp_err_t gpio_intr_enable(gpio_num_t gpio_num) { - return gpio_intr_enable_on_core (gpio_num, xPortGetCoreID()); + GPIO_CHECK(GPIO_IS_VALID_GPIO(gpio_num), "GPIO number error", ESP_ERR_INVALID_ARG); + portENTER_CRITICAL(&gpio_spinlock); + if(isr_core_id == GPIO_ISR_CORE_ID_UNINIT) { + isr_core_id = xPortGetCoreID(); + } + portEXIT_CRITICAL(&gpio_spinlock); + return gpio_intr_enable_on_core (gpio_num, isr_core_id); } esp_err_t gpio_intr_disable(gpio_num_t gpio_num) @@ -342,16 +360,15 @@ static void IRAM_ATTR gpio_intr_service(void* arg) if (gpio_isr_func == NULL) { return; } - //read status to get interrupt status for GPIO0-31 - const uint32_t gpio_intr_status = GPIO.status; + const uint32_t gpio_intr_status = (isr_core_id == 0) ? GPIO.pcpu_int : GPIO.acpu_int; if (gpio_intr_status) { gpio_isr_loop(gpio_intr_status, 0); GPIO.status_w1tc = gpio_intr_status; } //read status1 to get interrupt status for GPIO32-39 - const uint32_t gpio_intr_status_h = GPIO.status1.intr_st; + const uint32_t gpio_intr_status_h = (isr_core_id == 0) ? GPIO.pcpu_int1.intr : GPIO.acpu_int1.intr; if (gpio_intr_status_h) { gpio_isr_loop(gpio_intr_status_h, 32); GPIO.status1_w1tc.intr_st = gpio_intr_status_h; @@ -393,12 +410,12 @@ esp_err_t gpio_install_isr_service(int intr_alloc_flags) esp_err_t ret; portENTER_CRITICAL(&gpio_spinlock); gpio_isr_func = (gpio_isr_func_t*) calloc(GPIO_NUM_MAX, sizeof(gpio_isr_func_t)); + portEXIT_CRITICAL(&gpio_spinlock); if (gpio_isr_func == NULL) { ret = ESP_ERR_NO_MEM; } else { ret = gpio_isr_register(gpio_intr_service, NULL, intr_alloc_flags, &gpio_isr_handle); } - portEXIT_CRITICAL(&gpio_spinlock); return ret; } @@ -411,14 +428,37 @@ void gpio_uninstall_isr_service() esp_intr_free(gpio_isr_handle); free(gpio_isr_func); gpio_isr_func = NULL; + isr_core_id = GPIO_ISR_CORE_ID_UNINIT; portEXIT_CRITICAL(&gpio_spinlock); return; } +static void gpio_isr_register_on_core_static(void *param) +{ + gpio_isr_alloc_t *p = (gpio_isr_alloc_t *)param; + //We need to check the return value. + p->ret = esp_intr_alloc(p->source, p->intr_alloc_flags, p->fn, p->arg, p->handle); +} + esp_err_t gpio_isr_register(void (*fn)(void*), void * arg, int intr_alloc_flags, gpio_isr_handle_t *handle) { GPIO_CHECK(fn, "GPIO ISR null", ESP_ERR_INVALID_ARG); - return esp_intr_alloc(ETS_GPIO_INTR_SOURCE, intr_alloc_flags, fn, arg, handle); + gpio_isr_alloc_t p; + p.source = ETS_GPIO_INTR_SOURCE; + p.intr_alloc_flags = intr_alloc_flags; + p.fn = fn; + p.arg = arg; + p.handle = handle; + portENTER_CRITICAL(&gpio_spinlock); + if(isr_core_id == GPIO_ISR_CORE_ID_UNINIT) { + isr_core_id = xPortGetCoreID(); + } + portEXIT_CRITICAL(&gpio_spinlock); + esp_err_t ret = esp_ipc_call_blocking(isr_core_id, gpio_isr_register_on_core_static, (void *)&p); + if(ret != ESP_OK || p.ret != ESP_OK) { + return ESP_ERR_NOT_FOUND; + } + return ESP_OK; } esp_err_t gpio_wakeup_enable(gpio_num_t gpio_num, gpio_int_type_t intr_type) diff --git a/components/driver/test/test_gpio.c b/components/driver/test/test_gpio.c index ee07484bea..c40815683c 100644 --- a/components/driver/test/test_gpio.c +++ b/components/driver/test/test_gpio.c @@ -616,3 +616,128 @@ TEST_CASE("GPIO drive capability test", "[gpio][ignore]") drive_capability_set_get(GPIO_OUTPUT_IO, GPIO_DRIVE_CAP_3); prompt_to_continue("If this test finishes"); } + +#if !CONFIG_FREERTOS_UNICORE +void gpio_enable_task(void *param) +{ + int gpio_num = (int)param; + TEST_ESP_OK(gpio_intr_enable(gpio_num)); + vTaskDelete(NULL); +} + +/** Test the GPIO Interrupt Enable API with dual core enabled. The GPIO ISR service routine is registered on one core. + * When the GPIO interrupt on another core is enabled, the GPIO interrupt will be lost. + * First on the core 0, Do the following steps: + * 1. Configure the GPIO18 input_output mode, and enable the rising edge interrupt mode. + * 2. Trigger the GPIO18 interrupt and check if the interrupt responds correctly. + * 3. Disable the GPIO18 interrupt + * Then on the core 1, Do the following steps: + * 1. Enable the GPIO18 interrupt again. + * 2. Trigger the GPIO18 interrupt and check if the interrupt responds correctly. + * + */ +TEST_CASE("GPIO Enable/Disable interrupt on multiple cores", "[gpio][ignore]") +{ + const int test_io18 = GPIO_NUM_18; + gpio_config_t io_conf; + io_conf.intr_type = GPIO_INTR_NEGEDGE; + io_conf.mode = GPIO_MODE_INPUT_OUTPUT; + io_conf.pin_bit_mask = (1ULL << test_io18); + io_conf.pull_down_en = 0; + io_conf.pull_up_en = 1; + TEST_ESP_OK(gpio_config(&io_conf)); + TEST_ESP_OK(gpio_set_level(test_io18, 0)); + TEST_ESP_OK(gpio_install_isr_service(0)); + TEST_ESP_OK(gpio_isr_handler_add(test_io18, gpio_isr_edge_handler, (void*) test_io18)); + vTaskDelay(1000 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_set_level(test_io18, 1)); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_set_level(test_io18, 0)); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_intr_disable(test_io18)); + TEST_ASSERT(edge_intr_times == 1); + xTaskCreatePinnedToCore(gpio_enable_task, "gpio_enable_task", 1024*4, (void*)test_io18, 8, NULL, (xPortGetCoreID() == 0)); + vTaskDelay(1000 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_set_level(test_io18, 1)); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_set_level(test_io18, 0)); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_intr_disable(test_io18)); + TEST_ESP_OK(gpio_isr_handler_remove(test_io18)); + gpio_uninstall_isr_service(); + TEST_ASSERT(edge_intr_times == 2); +} +#endif + +typedef struct { + int gpio_num; + int isr_cnt; +} gpio_isr_param_t; + +static void gpio_isr_handler(void* arg) +{ + gpio_isr_param_t *param = (gpio_isr_param_t *)arg; + ets_printf("GPIO[%d] intr, val: %d\n", param->gpio_num, gpio_get_level(param->gpio_num)); + param->isr_cnt++; +} + +/** The previous GPIO interrupt service routine polls the interrupt raw status register to find the GPIO that triggered the interrupt. + * But this will incorrectly handle the interrupt disabled GPIOs, because the raw interrupt status register can still be set when + * the trigger signal arrives, even if the interrupt is disabled. + * First on the core 0: + * 1. Configure the GPIO18 and GPIO19 input_output mode. + * 2. Enable GPIO18 dual edge triggered interrupt, enable GPIO19 falling edge triggered interrupt. + * 3. Trigger GPIO18 interrupt, than disable the GPIO8 interrupt, and than trigger GPIO18 again(This time will not respond to the interrupt). + * 4. Trigger GPIO19 interrupt. + * If the bug is not fixed, you will see, in the step 4, the interrupt of GPIO18 will also respond. + */ +TEST_CASE("GPIO ISR service test", "[gpio][ignore]") +{ + const int test_io18 = GPIO_NUM_18; + const int test_io19 = GPIO_NUM_19; + static gpio_isr_param_t io18_param = { + .gpio_num = GPIO_NUM_18, + .isr_cnt = 0, + }; + static gpio_isr_param_t io19_param = { + .gpio_num = GPIO_NUM_19, + .isr_cnt = 0, + }; + gpio_config_t io_conf; + io_conf.intr_type = GPIO_INTR_DISABLE; + io_conf.mode = GPIO_MODE_INPUT_OUTPUT; + io_conf.pin_bit_mask = (1ULL << test_io18) | (1ULL << test_io19); + io_conf.pull_down_en = 0; + io_conf.pull_up_en = 1; + TEST_ESP_OK(gpio_config(&io_conf)); + TEST_ESP_OK(gpio_set_level(test_io18, 0)); + TEST_ESP_OK(gpio_set_level(test_io19, 0)); + TEST_ESP_OK(gpio_install_isr_service(0)); + TEST_ESP_OK(gpio_set_intr_type(test_io18, GPIO_INTR_ANYEDGE)); + TEST_ESP_OK(gpio_set_intr_type(test_io19, GPIO_INTR_NEGEDGE)); + TEST_ESP_OK(gpio_isr_handler_add(test_io18, gpio_isr_handler, (void*)&io18_param)); + TEST_ESP_OK(gpio_isr_handler_add(test_io19, gpio_isr_handler, (void*)&io19_param)); + printf("Triggering the interrupt of GPIO18\n"); + vTaskDelay(1000 / portTICK_RATE_MS); + //Rising edge + TEST_ESP_OK(gpio_set_level(test_io18, 1)); + printf("Disable the interrupt of GPIO18"); + vTaskDelay(100 / portTICK_RATE_MS); + //Disable GPIO18 interrupt, GPIO18 will not respond to the next falling edge interrupt. + TEST_ESP_OK(gpio_intr_disable(test_io18)); + vTaskDelay(100 / portTICK_RATE_MS); + //Falling edge + TEST_ESP_OK(gpio_set_level(test_io18, 0)); + + printf("Triggering the interrupt of GPIO19\n"); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_set_level(test_io19, 1)); + vTaskDelay(100 / portTICK_RATE_MS); + //Falling edge + TEST_ESP_OK(gpio_set_level(test_io19, 0)); + vTaskDelay(100 / portTICK_RATE_MS); + TEST_ESP_OK(gpio_isr_handler_remove(test_io18)); + TEST_ESP_OK(gpio_isr_handler_remove(test_io19)); + gpio_uninstall_isr_service(); + TEST_ASSERT((io18_param.isr_cnt == 1) && (io19_param.isr_cnt == 1)); +} \ No newline at end of file -- 2.40.0