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.

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;

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

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)

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;