Merge branch 'bugfix/gpio_intr_enable_bug' into 'master'

bugfix(GPIO):  Fixed the  GPIO  interrupt  enable  bug

Closes IDFGH-495

See merge request espressif/esp-idf!5571

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)

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));
+}