From 57642c10b6491a30be932097edd45280500b9b70 Mon Sep 17 00:00:00 2001 From: Makuna Date: Mon, 3 Aug 2015 19:35:17 -0700 Subject: [PATCH 1/3] Interrupt cleanup Fixes issue of reentrant calls to nointerrupts() exposed functional replacements to cli sei and SREG when dealing with interrupts InterruptLock class to auto stop and restore interrupt level Fix user ISR calls to be like Arduino with interrupts disabled fully. --- cores/esp8266/Arduino.h | 49 +++++++++++++++++---- cores/esp8266/core_esp8266_timer.c | 12 ++++- cores/esp8266/core_esp8266_wiring_digital.c | 7 +-- libraries/Servo/src/esp8266/Servo.cpp | 4 -- tools/sdk/include/ets_sys.h | 4 +- 5 files changed, 58 insertions(+), 18 deletions(-) diff --git a/cores/esp8266/Arduino.h b/cores/esp8266/Arduino.h index 6b379d5b1..b293bb633 100644 --- a/cores/esp8266/Arduino.h +++ b/cores/esp8266/Arduino.h @@ -137,17 +137,50 @@ void timer0_detachInterrupt(void); void ets_intr_lock(); void ets_intr_unlock(); -// level (0-15), -// level 15 will disable ALL interrupts, -// level 0 will disable most software interrupts +#ifndef __STRINGIFY +#define __STRINGIFY(a) #a +#endif + +// these low level routines provide a replacement for SREG interrupt save that AVR uses +// but are esp8266 specific. A normal use pattern is like // -#define xt_disable_interrupts(state, level) __asm__ __volatile__("rsil %0," __STRINGIFY(level) : "=a" (state)) -#define xt_enable_interrupts(state) __asm__ __volatile__("wsr %0,ps; isync" :: "a" (state) : "memory") +//{ +// uint32_t savedPS = xt_rsil(1); // this routine will allow level 2 and above +// // do work here +// xt_wsr_ps(savedPS); // restore the state +//} +// +// level (0-15), interrupts of the given level and above will be active +// level 15 will disable ALL interrupts, +// level 0 will enable ALL interrupts, +// +#define xt_rsil(level) (__extension__({uint32_t state; __asm__ __volatile__("rsil %0," __STRINGIFY(level) : "=a" (state)); state;})) +#define xt_wsr_ps(state) __asm__ __volatile__("wsr %0,ps; isync" :: "a" (state) : "memory") -extern uint32_t interruptsState; +#define interrupts() xt_rsil(0) +#define noInterrupts() xt_rsil(15) + +// this auto class wraps up xt_rsil so your code can be simplier, but can only be +// used in an ino or cpp files. A normal use pattern is like +// +//{ +// { +// InterruptLock(1); // this routine will allow level 2 and above +// // do work within interrupt lock here +// } +// do work outside of interrupt lock here outside its scope +//} +// +#define InterruptLock(intrLevel) \ +class _AutoDisableIntr { \ +public: \ + _AutoDisableIntr() { _savedPS = xt_rsil(intrLevel); } \ + ~_AutoDisableIntr() { xt_wsr_ps(_savedPS); } \ +private: \ + uint32_t _savedPS; \ + }; \ +_AutoDisableIntr _autoDisableIntr -#define interrupts() xt_enable_interrupts(interruptsState) -#define noInterrupts() __asm__ __volatile__("rsil %0,15" : "=a" (interruptsState)) #define clockCyclesPerMicrosecond() ( F_CPU / 1000000L ) #define clockCyclesToMicroseconds(a) ( (a) / clockCyclesPerMicrosecond() ) diff --git a/cores/esp8266/core_esp8266_timer.c b/cores/esp8266/core_esp8266_timer.c index 819c5f990..b130d3c93 100644 --- a/cores/esp8266/core_esp8266_timer.c +++ b/cores/esp8266/core_esp8266_timer.c @@ -32,7 +32,13 @@ static volatile timercallback timer1_user_cb = NULL; void timer1_isr_handler(void *para){ if ((T1C & ((1 << TCAR) | (1 << TCIT))) == 0) TEIE &= ~TEIE1;//edge int disable T1I = 0; - if (timer1_user_cb) timer1_user_cb(); + if (timer1_user_cb) { + // 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 + timer1_user_cb(); + xt_wsr_ps(savedPS); + } } void timer1_isr_init(){ @@ -72,7 +78,11 @@ static volatile timercallback timer0_user_cb = NULL; void timer0_isr_handler(void* para){ if (timer0_user_cb) { + // 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 timer0_user_cb(); + xt_wsr_ps(savedPS); } } diff --git a/cores/esp8266/core_esp8266_wiring_digital.c b/cores/esp8266/core_esp8266_wiring_digital.c index 5be065af5..c64961f03 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.c +++ b/cores/esp8266/core_esp8266_wiring_digital.c @@ -123,7 +123,11 @@ void interrupt_handler(void *arg) { if (handler->fn && (handler->mode == CHANGE || (handler->mode & 1) == digitalRead(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 handler->fn(); + xt_wsr_ps(savedPS); } } ETS_GPIO_INTR_ENABLE(); @@ -152,9 +156,6 @@ extern void __detachInterrupt(uint8_t pin) { } } -// stored state for the noInterrupts/interrupts methods -uint32_t interruptsState = 0; - void initPins() { //Disable UART interrupts system_set_os_print(0); diff --git a/libraries/Servo/src/esp8266/Servo.cpp b/libraries/Servo/src/esp8266/Servo.cpp index e9fce77f1..af8bf549e 100644 --- a/libraries/Servo/src/esp8266/Servo.cpp +++ b/libraries/Servo/src/esp8266/Servo.cpp @@ -59,8 +59,6 @@ static uint8_t s_servoCount = 0; // the total number of attached s_se //------------------------------------------------------------------------------ template void Servo_Handler(T* timer) { - noInterrupts(); - uint8_t servoIndex; // clear interrupt @@ -101,8 +99,6 @@ template void Servo_Handler(T* timer) timer->setEndOfCycle(); } - - interrupts(); } static void initISR(ServoTimerSequence timerId) diff --git a/tools/sdk/include/ets_sys.h b/tools/sdk/include/ets_sys.h index 607d601f1..526a24d40 100644 --- a/tools/sdk/include/ets_sys.h +++ b/tools/sdk/include/ets_sys.h @@ -65,8 +65,8 @@ inline bool ETS_INTR_WITHINISR() { uint32_t ps; __asm__ __volatile__("rsr %0,ps":"=a" (ps)); - // PS.EXCM bit check - return ((ps & (1 << 4)) != 0); + // PS.INTLEVEL check + return ((ps & 0x0f) != 0); } inline uint32_t ETS_INTR_ENABLED(void) From dfeed84ecb06c442969fe388ae05306e4c30aff2 Mon Sep 17 00:00:00 2001 From: Makuna Date: Mon, 3 Aug 2015 19:55:56 -0700 Subject: [PATCH 2/3] make compatible with existing interrupt lock class Support both the normal auto lock at all levels, and the lock at a specific level requiring different syntax --- cores/esp8266/Arduino.h | 21 --------------------- cores/esp8266/interrupts.h | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/cores/esp8266/Arduino.h b/cores/esp8266/Arduino.h index b293bb633..fa65a2d02 100644 --- a/cores/esp8266/Arduino.h +++ b/cores/esp8266/Arduino.h @@ -160,27 +160,6 @@ void ets_intr_unlock(); #define interrupts() xt_rsil(0) #define noInterrupts() xt_rsil(15) -// this auto class wraps up xt_rsil so your code can be simplier, but can only be -// used in an ino or cpp files. A normal use pattern is like -// -//{ -// { -// InterruptLock(1); // this routine will allow level 2 and above -// // do work within interrupt lock here -// } -// do work outside of interrupt lock here outside its scope -//} -// -#define InterruptLock(intrLevel) \ -class _AutoDisableIntr { \ -public: \ - _AutoDisableIntr() { _savedPS = xt_rsil(intrLevel); } \ - ~_AutoDisableIntr() { xt_wsr_ps(_savedPS); } \ -private: \ - uint32_t _savedPS; \ - }; \ -_AutoDisableIntr _autoDisableIntr - #define clockCyclesPerMicrosecond() ( F_CPU / 1000000L ) #define clockCyclesToMicroseconds(a) ( (a) / clockCyclesPerMicrosecond() ) diff --git a/cores/esp8266/interrupts.h b/cores/esp8266/interrupts.h index d86f89b1f..78982fe45 100644 --- a/cores/esp8266/interrupts.h +++ b/cores/esp8266/interrupts.h @@ -8,23 +8,51 @@ extern "C" { #include "ets_sys.h" } +// these auto classes wrap up xt_rsil so your code can be simplier, but can only be +// used in an ino or cpp files. -#define xt_disable_interrupts(state, level) __asm__ __volatile__("rsil %0," __STRINGIFY(level) : "=a" (state)) -#define xt_enable_interrupts(state) __asm__ __volatile__("wsr %0,ps; isync" :: "a" (state) : "memory") +// InterruptLock is used when you want to completely disable locks +//{ +// { +// InterruptLock lock; +// // do work within interrupt lock here +// } +// do work outside of interrupt lock here outside its scope +//} +// class InterruptLock { public: InterruptLock() { - xt_disable_interrupts(_state, 15); + _state = = xt_rsil(15); } ~InterruptLock() { - xt_enable_interrupts(_state); + xt_wsr_ps(_state); } protected: uint32_t _state; }; +// AutoInterruptLock is when you need to set a specific level, A normal use pattern is like +// +//{ +// { +// AutoInterruptLock(1); // this routine will allow level 2 and above +// // do work within interrupt lock here +// } +// do work outside of interrupt lock here outside its scope +//} +// +#define AutoInterruptLock(intrLevel) \ +class _AutoDisableIntr { \ +public: \ + _AutoDisableIntr() { _savedPS = xt_rsil(intrLevel); } \ + ~_AutoDisableIntr() { xt_wsr_ps(_savedPS); } \ +private: \ + uint32_t _savedPS; \ + }; \ +_AutoDisableIntr _autoDisableIntr #endif //INTERRUPTS_H From a2673f2f4bf387a1a53e400a60bf241e6360a7ee Mon Sep 17 00:00:00 2001 From: Makuna Date: Mon, 3 Aug 2015 19:58:42 -0700 Subject: [PATCH 3/3] copy paste error fi --- cores/esp8266/interrupts.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/interrupts.h b/cores/esp8266/interrupts.h index 78982fe45..813997447 100644 --- a/cores/esp8266/interrupts.h +++ b/cores/esp8266/interrupts.h @@ -24,7 +24,7 @@ extern "C" { class InterruptLock { public: InterruptLock() { - _state = = xt_rsil(15); + _state = xt_rsil(15); } ~InterruptLock() {