From 895535ea3bf60af38781ac2890a24371dd0b76a8 Mon Sep 17 00:00:00 2001
From: Mike <mike@sillyhouse.net>
Date: Mon, 15 Apr 2024 08:41:35 +0100
Subject: [PATCH] esp8266: NMI not disabled when timer1 ISR cleared (#2764)

SDK doesn't actually disable NMI interrupts when interrupt callback cleared. This blocks regular (FRC) timer1 interrupts if subsequently enabled.

The HardwareTimer (timer1) appears to stop working if applications switch from using NMI interrupts to regular timer interrupts.

A more subtle manifestation of this bug is that the original NMI callback continues to be called when a different FRC handler is set.
---
 .../Esp8266/Components/driver/hw_timer.cpp    | 30 +++++++++++++++++++
 .../driver/include/driver/hw_timer.h          |  7 +----
 .../esp8266/include/espinc/eagle_soc.h        |  1 +
 tests/HostTests/modules/Timers.cpp            | 15 ----------
 4 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp b/Sming/Arch/Esp8266/Components/driver/hw_timer.cpp
index b7a8641980..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,10 +69,18 @@ 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);
 	}
 }
 
+void IRAM_ATTR hw_timer1_detach_interrupt(void)
+{
+	hw_timer1_disable();
+	hw_timer1_disable_nmi();
+	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 1d7a7459f7..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,12 +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);
-	ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL);
-}
+void hw_timer1_detach_interrupt(void);
 
 /**
  * @brief Get timer1 count
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)
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<CallbackTimerTest*>(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;