From 6b9ffead073ca39a00e60c402eaf990716756119 Mon Sep 17 00:00:00 2001 From: "Dirk O. Kaar" Date: Mon, 17 Jun 2019 14:00:46 +0200 Subject: [PATCH 1/6] Properly check for "functional" ISRs and expose C-style attachInterruptArg --- cores/esp8266/Arduino.h | 1 + cores/esp8266/FunctionalInterrupt.cpp | 6 +- cores/esp8266/core_esp8266_wiring_digital.cpp | 116 ++++++++++-------- 3 files changed, 70 insertions(+), 53 deletions(-) diff --git a/cores/esp8266/Arduino.h b/cores/esp8266/Arduino.h index 34e5fb8fce..80bf1f6ede 100644 --- a/cores/esp8266/Arduino.h +++ b/cores/esp8266/Arduino.h @@ -218,6 +218,7 @@ uint8_t shiftIn(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder); void attachInterrupt(uint8_t pin, void (*)(void), int mode); void detachInterrupt(uint8_t pin); +void attachInterruptArg(uint8_t pin, void (*)(void*), void* arg, int mode); void preinit(void); void setup(void); diff --git a/cores/esp8266/FunctionalInterrupt.cpp b/cores/esp8266/FunctionalInterrupt.cpp index fa57e217e5..c69f94dd7b 100644 --- a/cores/esp8266/FunctionalInterrupt.cpp +++ b/cores/esp8266/FunctionalInterrupt.cpp @@ -7,7 +7,7 @@ typedef void (*voidFuncPtr)(void); typedef void (*voidFuncPtrArg)(void*); // Helper functions for Functional interrupt routines -extern "C" void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void*fp , int mode); +extern "C" void __attachInterruptFunctionalArg(uint8_t pin, voidFuncPtr userFunc, void*fp, int mode, bool functional); void ICACHE_RAM_ATTR interruptFunctional(void* arg) @@ -47,7 +47,7 @@ void attachInterrupt(uint8_t pin, std::function intRoutine, int mode as->interruptInfo = ii; as->functionInfo = fi; - __attachInterruptArg (pin, (voidFuncPtr)interruptFunctional, as, mode); + __attachInterruptFunctionalArg(pin, (voidFuncPtr)interruptFunctional, as, mode, true); } void attachScheduledInterrupt(uint8_t pin, std::function scheduledIntRoutine, int mode) @@ -61,5 +61,5 @@ void attachScheduledInterrupt(uint8_t pin, std::function sc as->interruptInfo = ii; as->functionInfo = fi; - __attachInterruptArg (pin, (voidFuncPtr)interruptFunctional, as, mode); + __attachInterruptFunctionalArg(pin, (voidFuncPtr)interruptFunctional, as, mode, true); } diff --git a/cores/esp8266/core_esp8266_wiring_digital.cpp b/cores/esp8266/core_esp8266_wiring_digital.cpp index 0a8a7252f1..8c7221250a 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.cpp +++ b/cores/esp8266/core_esp8266_wiring_digital.cpp @@ -109,8 +109,9 @@ typedef void (*voidFuncPtrArg)(void*); typedef struct { uint8_t mode; - void (*fn)(void); + voidFuncPtr fn; void * arg; + bool functional; } interrupt_handler_t; //duplicate from functionalInterrupt.h keep in sync @@ -125,11 +126,11 @@ typedef struct { void* functionInfo; } ArgStructure; -static interrupt_handler_t interrupt_handlers[16]; +static interrupt_handler_t interrupt_handlers[16] = { {0, 0, 0, 0}, }; static uint32_t interrupt_reg = 0; -void ICACHE_RAM_ATTR interrupt_handler(void *arg) { - (void) arg; +void ICACHE_RAM_ATTR interrupt_handler(void*) +{ uint32_t status = GPIE; GPIEC = status;//clear them interrupts uint32_t levels = GPI; @@ -144,34 +145,37 @@ void ICACHE_RAM_ATTR interrupt_handler(void *arg) { if (handler->fn && (handler->mode == CHANGE || (handler->mode & 1) == !!(levels & (1 << i)))) { - // to make ISR compatible to Arduino AVR model where interrupts are disabled - // we disable them before we call the client ISR - uint32_t savedPS = xt_rsil(15); // stop other interrupts - ArgStructure* localArg = (ArgStructure*)handler->arg; - if (localArg && localArg->interruptInfo) - { - localArg->interruptInfo->pin = i; - localArg->interruptInfo->value = __digitalRead(i); - localArg->interruptInfo->micro = micros(); - } - if (handler->arg) - { - ((voidFuncPtrArg)handler->fn)(handler->arg); - } - else - { - handler->fn(); + // to make ISR compatible to Arduino AVR model where interrupts are disabled + // we disable them before we call the client ISR + uint32_t savedPS = xt_rsil(15); // stop other interrupts + if (handler->functional) + { + ArgStructure* localArg = (ArgStructure*)handler->arg; + if (localArg && localArg->interruptInfo) + { + localArg->interruptInfo->pin = i; + localArg->interruptInfo->value = __digitalRead(i); + localArg->interruptInfo->micro = micros(); + } + } + if (handler->arg) + { + ((voidFuncPtrArg)handler->fn)(handler->arg); + } + else + { + handler->fn(); + } + xt_wsr_ps(savedPS); } - xt_wsr_ps(savedPS); - } } ETS_GPIO_INTR_ENABLE(); } extern void cleanupFunctional(void* arg); -extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFunc, void *arg, int mode) { - +extern void __attachInterruptFunctionalArg(uint8_t pin, voidFuncPtrArg userFunc, void* arg, int mode, bool functional) +{ // #5780 // https://github.com/esp8266/esp8266-wiki/wiki/Memory-Map if ((uint32_t)userFunc >= 0x40200000) @@ -183,14 +187,15 @@ extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFu if(pin < 16) { ETS_GPIO_INTR_DISABLE(); - interrupt_handler_t *handler = &interrupt_handlers[pin]; + interrupt_handler_t* handler = &interrupt_handlers[pin]; handler->mode = mode; - handler->fn = userFunc; - if (handler->arg) // Clean when new attach without detach - { - cleanupFunctional(handler->arg); - } + handler->fn = (voidFuncPtr)userFunc; + if (handler->functional && handler->arg) // Clean when new attach without detach + { + cleanupFunctional(handler->arg); + } handler->arg = arg; + handler->functional = functional; interrupt_reg |= (1 << pin); GPC(pin) &= ~(0xF << GPCI);//INT mode disabled GPIEC = (1 << pin); //Clear Interrupt for this pin @@ -200,28 +205,38 @@ extern void ICACHE_RAM_ATTR __attachInterruptArg(uint8_t pin, voidFuncPtr userFu } } -extern void ICACHE_RAM_ATTR __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode ) +extern void __attachInterruptArg(uint8_t pin, voidFuncPtrArg userFunc, void* arg, int mode) { - __attachInterruptArg (pin, userFunc, 0, mode); + __attachInterruptFunctionalArg(pin, userFunc, arg, mode, false); } -extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) { - if(pin < 16) { - ETS_GPIO_INTR_DISABLE(); - GPC(pin) &= ~(0xF << GPCI);//INT mode disabled - GPIEC = (1 << pin); //Clear Interrupt for this pin - interrupt_reg &= ~(1 << pin); - interrupt_handler_t *handler = &interrupt_handlers[pin]; - handler->mode = 0; - handler->fn = 0; - if (handler->arg) - { - cleanupFunctional(handler->arg); - } - handler->arg = 0; - if (interrupt_reg) - ETS_GPIO_INTR_ENABLE(); - } +extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode) +{ + __attachInterruptFunctionalArg(pin, (voidFuncPtrArg)userFunc, 0, mode, false); +} + +extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) +{ + if (pin < 16) + { + ETS_GPIO_INTR_DISABLE(); + GPC(pin) &= ~(0xF << GPCI);//INT mode disabled + GPIEC = (1 << pin); //Clear Interrupt for this pin + interrupt_reg &= ~(1 << pin); + interrupt_handler_t* handler = &interrupt_handlers[pin]; + handler->mode = 0; + handler->fn = 0; + if (handler->functional && handler->arg) + { + cleanupFunctional(handler->arg); + } + handler->arg = 0; + handler->functional = false; + if (interrupt_reg) + { + ETS_GPIO_INTR_ENABLE(); + } + } } void initPins() { @@ -243,6 +258,7 @@ extern void pinMode(uint8_t pin, uint8_t mode) __attribute__ ((weak, alias("__pi extern void digitalWrite(uint8_t pin, uint8_t val) __attribute__ ((weak, alias("__digitalWrite"))); extern int digitalRead(uint8_t pin) __attribute__ ((weak, alias("__digitalRead"))); extern void attachInterrupt(uint8_t pin, voidFuncPtr handler, int mode) __attribute__ ((weak, alias("__attachInterrupt"))); +extern void attachInterruptArg(uint8_t pin, voidFuncPtrArg handler, void* arg, int mode) __attribute__((weak, alias("__attachInterruptArg"))); extern void detachInterrupt(uint8_t pin) __attribute__ ((weak, alias("__detachInterrupt"))); }; From 422cdebdda76365f1119c7dbe80194a957c66142 Mon Sep 17 00:00:00 2001 From: "Dirk O. Kaar" Date: Wed, 26 Jun 2019 08:08:41 +0200 Subject: [PATCH 2/6] Indentation --- cores/esp8266/FunctionalInterrupt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/FunctionalInterrupt.cpp b/cores/esp8266/FunctionalInterrupt.cpp index c69f94dd7b..665d8043b3 100644 --- a/cores/esp8266/FunctionalInterrupt.cpp +++ b/cores/esp8266/FunctionalInterrupt.cpp @@ -47,7 +47,7 @@ void attachInterrupt(uint8_t pin, std::function intRoutine, int mode as->interruptInfo = ii; as->functionInfo = fi; - __attachInterruptFunctionalArg(pin, (voidFuncPtr)interruptFunctional, as, mode, true); + __attachInterruptFunctionalArg(pin, (voidFuncPtr)interruptFunctional, as, mode, true); } void attachScheduledInterrupt(uint8_t pin, std::function scheduledIntRoutine, int mode) @@ -61,5 +61,5 @@ void attachScheduledInterrupt(uint8_t pin, std::function sc as->interruptInfo = ii; as->functionInfo = fi; - __attachInterruptFunctionalArg(pin, (voidFuncPtr)interruptFunctional, as, mode, true); + __attachInterruptFunctionalArg(pin, (voidFuncPtr)interruptFunctional, as, mode, true); } From e0e81593011d17d58f9d5e95dc2c38b8d5b5d4aa Mon Sep 17 00:00:00 2001 From: "Dirk O. Kaar" Date: Tue, 25 Jun 2019 13:10:37 +0200 Subject: [PATCH 3/6] Use RAII idiom (cherry picked from commit 15c0b5b356aad0c3032b96ed6db0ec70cbf719d3) # Conflicts: # cores/esp8266/core_esp8266_wiring_digital.cpp --- cores/esp8266/core_esp8266_wiring_digital.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_wiring_digital.cpp b/cores/esp8266/core_esp8266_wiring_digital.cpp index 8c7221250a..e32c8507a3 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.cpp +++ b/cores/esp8266/core_esp8266_wiring_digital.cpp @@ -26,6 +26,7 @@ #include "ets_sys.h" #include "user_interface.h" #include "core_esp8266_waveform.h" +#include "interrupts.h" extern "C" { @@ -147,7 +148,7 @@ void ICACHE_RAM_ATTR interrupt_handler(void*) (handler->mode & 1) == !!(levels & (1 << i)))) { // to make ISR compatible to Arduino AVR model where interrupts are disabled // we disable them before we call the client ISR - uint32_t savedPS = xt_rsil(15); // stop other interrupts + esp8266::InterruptLock irqLock; // stop other interrupts if (handler->functional) { ArgStructure* localArg = (ArgStructure*)handler->arg; @@ -166,7 +167,6 @@ void ICACHE_RAM_ATTR interrupt_handler(void*) { handler->fn(); } - xt_wsr_ps(savedPS); } } ETS_GPIO_INTR_ENABLE(); From c3d359c349d22524ce30b65e2be77dbe2de5160d Mon Sep 17 00:00:00 2001 From: "Dirk O. Kaar" Date: Wed, 26 Jun 2019 08:17:13 +0200 Subject: [PATCH 4/6] Easier reviewability --- cores/esp8266/core_esp8266_wiring_digital.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/cores/esp8266/core_esp8266_wiring_digital.cpp b/cores/esp8266/core_esp8266_wiring_digital.cpp index e32c8507a3..6ae863ab50 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.cpp +++ b/cores/esp8266/core_esp8266_wiring_digital.cpp @@ -210,13 +210,7 @@ extern void __attachInterruptArg(uint8_t pin, voidFuncPtrArg userFunc, void* arg __attachInterruptFunctionalArg(pin, userFunc, arg, mode, false); } -extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode) -{ - __attachInterruptFunctionalArg(pin, (voidFuncPtrArg)userFunc, 0, mode, false); -} - -extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) -{ +extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) { if (pin < 16) { ETS_GPIO_INTR_DISABLE(); @@ -239,6 +233,11 @@ extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) } } +extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode) +{ + __attachInterruptFunctionalArg(pin, (voidFuncPtrArg)userFunc, 0, mode, false); +} + void initPins() { //Disable UART interrupts system_set_os_print(0); From 3ad3163bfae0aa9cb29c89331c5fac8942f44afc Mon Sep 17 00:00:00 2001 From: "Dirk O. Kaar" Date: Wed, 3 Jul 2019 07:52:36 +0200 Subject: [PATCH 5/6] Refactored after review input. --- cores/esp8266/core_esp8266_wiring_digital.cpp | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/cores/esp8266/core_esp8266_wiring_digital.cpp b/cores/esp8266/core_esp8266_wiring_digital.cpp index 6ae863ab50..71db2c6080 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.cpp +++ b/cores/esp8266/core_esp8266_wiring_digital.cpp @@ -174,6 +174,19 @@ void ICACHE_RAM_ATTR interrupt_handler(void*) extern void cleanupFunctional(void* arg); +void set_interrupt_handlers(uint8_t pin, voidFuncPtr userFunc, void* arg, uint8_t mode, bool functional) +{ + interrupt_handler_t* handler = &interrupt_handlers[pin]; + handler->mode = mode; + handler->fn = userFunc; + if (handler->functional && handler->arg) // Clean when new attach without detach + { + cleanupFunctional(handler->arg); + } + handler->arg = arg; + handler->functional = functional; +} + extern void __attachInterruptFunctionalArg(uint8_t pin, voidFuncPtrArg userFunc, void* arg, int mode, bool functional) { // #5780 @@ -187,15 +200,7 @@ extern void __attachInterruptFunctionalArg(uint8_t pin, voidFuncPtrArg userFunc, if(pin < 16) { ETS_GPIO_INTR_DISABLE(); - interrupt_handler_t* handler = &interrupt_handlers[pin]; - handler->mode = mode; - handler->fn = (voidFuncPtr)userFunc; - if (handler->functional && handler->arg) // Clean when new attach without detach - { - cleanupFunctional(handler->arg); - } - handler->arg = arg; - handler->functional = functional; + set_interrupt_handlers(pin, (voidFuncPtr)userFunc, arg, mode, functional); interrupt_reg |= (1 << pin); GPC(pin) &= ~(0xF << GPCI);//INT mode disabled GPIEC = (1 << pin); //Clear Interrupt for this pin @@ -217,15 +222,7 @@ extern void ICACHE_RAM_ATTR __detachInterrupt(uint8_t pin) { GPC(pin) &= ~(0xF << GPCI);//INT mode disabled GPIEC = (1 << pin); //Clear Interrupt for this pin interrupt_reg &= ~(1 << pin); - interrupt_handler_t* handler = &interrupt_handlers[pin]; - handler->mode = 0; - handler->fn = 0; - if (handler->functional && handler->arg) - { - cleanupFunctional(handler->arg); - } - handler->arg = 0; - handler->functional = false; + set_interrupt_handlers(pin, nullptr, nullptr, 0, false); if (interrupt_reg) { ETS_GPIO_INTR_ENABLE(); From 498356da7cb8bc49404b2c103ac609f1a8fbb705 Mon Sep 17 00:00:00 2001 From: "Dirk O. Kaar" Date: Wed, 3 Jul 2019 08:50:38 +0200 Subject: [PATCH 6/6] Finish up insights from review comments. --- cores/esp8266/core_esp8266_wiring_digital.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_wiring_digital.cpp b/cores/esp8266/core_esp8266_wiring_digital.cpp index 71db2c6080..a1a0d51ff0 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.cpp +++ b/cores/esp8266/core_esp8266_wiring_digital.cpp @@ -174,7 +174,7 @@ void ICACHE_RAM_ATTR interrupt_handler(void*) extern void cleanupFunctional(void* arg); -void set_interrupt_handlers(uint8_t pin, voidFuncPtr userFunc, void* arg, uint8_t mode, bool functional) +static void set_interrupt_handlers(uint8_t pin, voidFuncPtr userFunc, void* arg, uint8_t mode, bool functional) { interrupt_handler_t* handler = &interrupt_handlers[pin]; handler->mode = mode; @@ -235,7 +235,7 @@ extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int mode) __attachInterruptFunctionalArg(pin, (voidFuncPtrArg)userFunc, 0, mode, false); } -void initPins() { +extern void initPins() { //Disable UART interrupts system_set_os_print(0); U0IE = 0;