From 0de35f92c8078a9a5b14de2554daf6023ccfa4ff Mon Sep 17 00:00:00 2001 From: mikee47 Date: Fri, 12 Apr 2024 20:53:06 +0100 Subject: [PATCH 1/4] Disable NMI property when interrupt cleared --- Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h | 1 + Sming/Arch/Esp8266/Components/esp8266/include/espinc/eagle_soc.h | 1 + 2 files changed, 2 insertions(+) diff --git a/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h b/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h index 1d7a7459f7..54f5c96297 100644 --- a/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h +++ b/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h @@ -127,6 +127,7 @@ __forceinline void IRAM_ATTR hw_timer1_detach_interrupt(void) { hw_timer1_disable(); ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); + REG_WRITE(NMI_INT_ENABLE_REG, 0); ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); } diff --git a/Sming/Arch/Esp8266/Components/esp8266/include/espinc/eagle_soc.h b/Sming/Arch/Esp8266/Components/esp8266/include/espinc/eagle_soc.h index fcb72fcc97..99244138f1 100644 --- a/Sming/Arch/Esp8266/Components/esp8266/include/espinc/eagle_soc.h +++ b/Sming/Arch/Esp8266/Components/esp8266/include/espinc/eagle_soc.h @@ -126,6 +126,7 @@ //}} //Interrupt remap control registers define{{ +#define NMI_INT_ENABLE_REG (PERIPHS_DPORT_BASEADDR) #define EDGE_INT_ENABLE_REG (PERIPHS_DPORT_BASEADDR + 0x04) #define WDT_EDGE_INT_ENABLE() SET_PERI_REG_MASK(EDGE_INT_ENABLE_REG, BIT0) #define TM1_EDGE_INT_ENABLE() SET_PERI_REG_MASK(EDGE_INT_ENABLE_REG, BIT1) From a869da958931b446151c467e2f0bfbfdf02e5411 Mon Sep 17 00:00:00 2001 From: mikee47 Date: Sat, 13 Apr 2024 08:28:47 +0100 Subject: [PATCH 2/4] Revise `hw_timer1_detach_interrupt` and document change --- .../Esp8266/Components/driver/hw_timer.cpp | 27 +++++++++++++++++++ .../driver/include/driver/hw_timer.h | 8 +----- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp b/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp index b7a8641980..eab17e8a77 100644 --- a/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp +++ b/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp @@ -51,6 +51,33 @@ void hw_timer1_attach_interrupt(hw_timer_source_type_t source_type, hw_timer_cal } } +void IRAM_ATTR hw_timer1_detach_interrupt(void) +{ + hw_timer1_disable(); + + /* + * This macro calls SDK `NmiTimSetFunc` which doesn't actually disable NMI (a known bug). + * If we subsequently enable FRC interrupts, the timer won't work so we need to properly + * disable NMI manually. + * + * The NmiTimSetFunc code looks like this: + * + * uint32_t value = REG_READ(NMI_INT_ENABLE_REG); + * value &= ~0x1f; + * value |= 0x0f; + * REG_WRITE(NMI_INT_ENABLE_REG, value); + * + * Note that there is no published documentation for this register. + * Clearing it to zero appears to work but may have unintended side-effects. + */ + ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); + auto value = REG_READ(NMI_INT_ENABLE_REG); + REG_WRITE(NMI_INT_ENABLE_REG, value & ~0x1f); + + // FRC interrupts + ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); +} + void hw_timer_init(void) { constexpr uint32_t FRC2_ENABLE_TIMER = BIT7; diff --git a/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h b/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h index 54f5c96297..04395c83c7 100644 --- a/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h +++ b/Sming/Arch/Esp8266/Components/driver/include/driver/hw_timer.h @@ -123,13 +123,7 @@ __forceinline void IRAM_ATTR hw_timer1_disable(void) /** * @brief Detach interrupt from the timer */ -__forceinline void IRAM_ATTR hw_timer1_detach_interrupt(void) -{ - hw_timer1_disable(); - ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); - REG_WRITE(NMI_INT_ENABLE_REG, 0); - ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); -} +void hw_timer1_detach_interrupt(void); /** * @brief Get timer1 count From ce9b2ec7e07605a752633dcf4ff4e5fe195107c7 Mon Sep 17 00:00:00 2001 From: mikee47 Date: Sat, 13 Apr 2024 08:28:47 +0100 Subject: [PATCH 3/4] Add `hw_timer1_disable_nmi`, call when attaching FRC interrupt routine --- .../Esp8266/Components/driver/hw_timer.cpp | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp b/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp index eab17e8a77..3d15c4525a 100644 --- a/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp +++ b/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp @@ -36,6 +36,28 @@ static void IRAM_ATTR nmi_handler() nmi_callback.func(nmi_callback.arg); } +/* + * The `ETS_FRC_TIMER1_NMI_INTR_ATTACH` macro calls SDK `NmiTimSetFunc` which + * doesn't actually disable NMI (a known bug). + * If we subsequently enable FRC interrupts, the timer won't work so we need to properly + * disable NMI manually. + * + * The NmiTimSetFunc code looks like this: + * + * uint32_t value = REG_READ(NMI_INT_ENABLE_REG); + * value &= ~0x1f; + * value |= 0x0f; + * REG_WRITE(NMI_INT_ENABLE_REG, value); + * + * Note that there is no published documentation for this register. + * Clearing it to zero appears to work but may have unintended side-effects. + */ +static void IRAM_ATTR hw_timer1_disable_nmi() +{ + auto value = REG_READ(NMI_INT_ENABLE_REG); + REG_WRITE(NMI_INT_ENABLE_REG, value & ~0x1f); +} + void hw_timer1_attach_interrupt(hw_timer_source_type_t source_type, hw_timer_callback_t callback, void* arg) { if(source_type == TIMER_NMI_SOURCE) { @@ -47,6 +69,7 @@ void hw_timer1_attach_interrupt(hw_timer_source_type_t source_type, hw_timer_cal ETS_FRC_TIMER1_NMI_INTR_ATTACH(nmi_handler); } } else { + hw_timer1_disable_nmi(); ETS_FRC_TIMER1_INTR_ATTACH(callback, arg); } } @@ -54,27 +77,7 @@ void hw_timer1_attach_interrupt(hw_timer_source_type_t source_type, hw_timer_cal void IRAM_ATTR hw_timer1_detach_interrupt(void) { hw_timer1_disable(); - - /* - * This macro calls SDK `NmiTimSetFunc` which doesn't actually disable NMI (a known bug). - * If we subsequently enable FRC interrupts, the timer won't work so we need to properly - * disable NMI manually. - * - * The NmiTimSetFunc code looks like this: - * - * uint32_t value = REG_READ(NMI_INT_ENABLE_REG); - * value &= ~0x1f; - * value |= 0x0f; - * REG_WRITE(NMI_INT_ENABLE_REG, value); - * - * Note that there is no published documentation for this register. - * Clearing it to zero appears to work but may have unintended side-effects. - */ - ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); - auto value = REG_READ(NMI_INT_ENABLE_REG); - REG_WRITE(NMI_INT_ENABLE_REG, value & ~0x1f); - - // FRC interrupts + hw_timer1_disable_nmi(); ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); } From 9f55db9f9140a7261dc6df24bc4236637d46c926 Mon Sep 17 00:00:00 2001 From: mikee47 Date: Sun, 14 Apr 2024 11:11:25 +0100 Subject: [PATCH 4/4] Fix HostTests memory leak --- tests/HostTests/modules/Timers.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/tests/HostTests/modules/Timers.cpp b/tests/HostTests/modules/Timers.cpp index 361627e98d..f03b3ef2a9 100644 --- a/tests/HostTests/modules/Timers.cpp +++ b/tests/HostTests/modules/Timers.cpp @@ -79,9 +79,6 @@ class CallbackTimerTest : public TestGroup statusTimer.stop(); Serial.print("statusTimer stopped: "); Serial.println(statusTimer); - - // Release any allocated delegate memory - timer64.setCallback(TimerDelegate(nullptr)); done = true; } @@ -113,17 +110,6 @@ class CallbackTimerTest : public TestGroup Serial.println(statusTimer); - { - auto tmp = new Timer; - tmp->initializeMs<1200>( - [](void* arg) { - auto self = static_cast(arg); - Serial << self->timer64 << _F(" fired") << endl; - }, - this); - tmp->startOnce(); - } - if(1) { longTimer.setCallback([this]() { --activeTimerCount; @@ -222,7 +208,6 @@ class CallbackTimerTest : public TestGroup private: Timer statusTimer; unsigned statusTimerCount = 0; - Timer timer64; HardwareTimerTest timer1; Timer longTimer; uint32_t longStartTicks = 0;