Fix button interrupt after light sleep (#3587)

* Make ButtonThread instance extern
Previously was a static local instance in setup(). Now declared in ButtonThread.cpp, accessible via extern declaration in ButtonThread.

* Extract attachInterrupt() calls to public method; create matching method for detachInterrupt()

* Change suspension of button interrupts for light-sleep

* Fix declaration for ARCH_PORTDUINO

* Remove LOG_DEBUG used during testing

* Don't assume device has a button..

* Guard entire constructor code

* Don't use BUTTON_PIN with ARCH_PORTDUINO

---------

Co-authored-by: Manuel <71137295+mverch67@users.noreply.github.com>

src/ButtonThread.cpp

@@ -23,18 +23,24 @@
 
 using namespace concurrency;
 
+ButtonThread *buttonThread; // Declared extern in header
 volatile ButtonThread::ButtonEventType ButtonThread::btnEvent = ButtonThread::BUTTON_EVENT_NONE;
 
+#if defined(BUTTON_PIN) || defined(ARCH_PORTDUINO)
+OneButton ButtonThread::userButton; // Get reference to static member
+#endif
+
 ButtonThread::ButtonThread() : OSThread("Button")
 {
-#if defined(ARCH_PORTDUINO) || defined(BUTTON_PIN)
+#if defined(BUTTON_PIN) || defined(ARCH_PORTDUINO)
+
 #if defined(ARCH_PORTDUINO)
     if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC) {
-        userButton = OneButton(settingsMap[user], true, true);
+        this->userButton = OneButton(settingsMap[user], true, true);
         LOG_DEBUG("Using GPIO%02d for button\n", settingsMap[user]);
     }
 #elif defined(BUTTON_PIN)
-    int pin = config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN;
+    int pin = config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN; // Resolved button pin
     this->userButton = OneButton(pin, true, true);
     LOG_DEBUG("Using GPIO%02d for button\n", pin);
 #endif
@@ -43,6 +49,8 @@ ButtonThread::ButtonThread() : OSThread("Button")
     // Some platforms (nrf52) have a SENSE variant which allows wake from sleep - override what OneButton did
     pinMode(pin, INPUT_PULLUP_SENSE);
 #endif
+
+#if defined(BUTTON_PIN) || defined(ARCH_PORTDUINO)
     userButton.attachClick(userButtonPressed);
     userButton.setClickMs(250);
     userButton.setPressMs(c_longPressTime);
@@ -53,21 +61,8 @@ ButtonThread::ButtonThread() : OSThread("Button")
     userButton.attachLongPressStart(userButtonPressedLongStart);
     userButton.attachLongPressStop(userButtonPressedLongStop);
 #endif
-#if defined(ARCH_PORTDUINO)
-    if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC)
-        wakeOnIrq(settingsMap[user], FALLING);
-#else
-    static OneButton *pBtn = &userButton; // only one instance of ButtonThread is created, so static is safe
-    attachInterrupt(
-        pin,
-        []() {
-            BaseType_t higherWake = 0;
-            mainDelay.interruptFromISR(&higherWake);
-            pBtn->tick();
-        },
-        CHANGE);
-#endif
 #endif
+
 #ifdef BUTTON_PIN_ALT
     userButtonAlt = OneButton(BUTTON_PIN_ALT, true, true);
 #ifdef INPUT_PULLUP_SENSE
@@ -81,14 +76,15 @@ ButtonThread::ButtonThread() : OSThread("Button")
     userButtonAlt.attachDoubleClick(userButtonDoublePressed);
     userButtonAlt.attachLongPressStart(userButtonPressedLongStart);
     userButtonAlt.attachLongPressStop(userButtonPressedLongStop);
-    wakeOnIrq(BUTTON_PIN_ALT, FALLING);
 #endif
 
 #ifdef BUTTON_PIN_TOUCH
     userButtonTouch = OneButton(BUTTON_PIN_TOUCH, true, true);
     userButtonTouch.setPressMs(400);
     userButtonTouch.attachLongPressStart(touchPressedLongStart); // Better handling with longpress than click?
-    wakeOnIrq(BUTTON_PIN_TOUCH, FALLING);
+#endif
+
+    attachButtonInterrupts();
 #endif
 }
 
@@ -220,6 +216,58 @@ int32_t ButtonThread::runOnce()
     return 50;
 }
 
+/*
+ * Attach (or re-attach) hardware interrupts for buttons
+ * Public method. Used outside class when waking from MCU sleep
+ */
+void ButtonThread::attachButtonInterrupts()
+{
+#if defined(ARCH_PORTDUINO)
+    if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC)
+        wakeOnIrq(settingsMap[user], FALLING);
+#elif defined(BUTTON_PIN)
+    // Interrupt for user button, during normal use. Improves responsiveness.
+    attachInterrupt(
+        config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN,
+        []() {
+            BaseType_t higherWake = 0;
+            mainDelay.interruptFromISR(&higherWake);
+            ButtonThread::userButton.tick();
+        },
+        CHANGE);
+#endif
+
+#ifdef BUTTON_PIN_ALT
+    wakeOnIrq(BUTTON_PIN_ALT, FALLING);
+#endif
+
+#ifdef BUTTON_PIN_TOUCH
+    wakeOnIrq(BUTTON_PIN_TOUCH, FALLING);
+#endif
+}
+
+/*
+ * Detach the "normal" button interrupts.
+ * Public method. Used before attaching a "wake-on-button" interrupt for MCU sleep
+ */
+void ButtonThread::detachButtonInterrupts()
+{
+#if defined(ARCH_PORTDUINO)
+    if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC)
+        detachInterrupt(settingsMap[user]);
+#elif defined(BUTTON_PIN)
+    detachInterrupt(config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN);
+#endif
+
+#ifdef BUTTON_PIN_ALT
+    detachInterrupt(BUTTON_PIN_ALT);
+#endif
+
+#ifdef BUTTON_PIN_TOUCH
+    detachInterrupt(BUTTON_PIN_TOUCH);
+#endif
+}
+
 /**
  * Watch a GPIO and if we get an IRQ, wake the main thread.
  * Use to add wake on button press

src/ButtonThread.h

@@ -22,11 +22,13 @@ class ButtonThread : public concurrency::OSThread
 
     ButtonThread();
     int32_t runOnce() override;
+    void attachButtonInterrupts();
+    void detachButtonInterrupts();
     void storeClickCount();
 
   private:
-#ifdef BUTTON_PIN
+#if defined(BUTTON_PIN) || defined(ARCH_PORTDUINO)
-    OneButton userButton;
+    static OneButton userButton; // Static - accessed from an interrupt
 #endif
 #ifdef BUTTON_PIN_ALT
     OneButton userButtonAlt;
@@ -34,9 +36,6 @@ class ButtonThread : public concurrency::OSThread
 #ifdef BUTTON_PIN_TOUCH
     OneButton userButtonTouch;
 #endif
-#if defined(ARCH_PORTDUINO)
-    OneButton userButton;
-#endif
 
     // set during IRQ
     static volatile ButtonEventType btnEvent;
@@ -54,3 +53,5 @@ class ButtonThread : public concurrency::OSThread
     static void userButtonPressedLongStop();
     static void touchPressedLongStart() { btnEvent = BUTTON_EVENT_TOUCH_LONG_PRESSED; }
 };
+
+extern ButtonThread *buttonThread;

src/main.cpp

@@ -188,9 +188,6 @@ uint32_t timeLastPowered = 0;
 
 static Periodic *ledPeriodic;
 static OSThread *powerFSMthread;
-#if HAS_BUTTON || defined(ARCH_PORTDUINO)
-static OSThread *buttonThread;
-#endif
 static OSThread *accelerometerThread;
 static OSThread *ambientLightingThread;
 SPISettings spiSettings(4000000, MSBFIRST, SPI_MODE0);

src/sleep.cpp

@@ -4,6 +4,7 @@
 #include "GPS.h"
 #endif
 
+#include "ButtonThread.h"
 #include "MeshRadio.h"
 #include "MeshService.h"
 #include "NodeDB.h"
@@ -337,7 +338,10 @@ esp_sleep_wakeup_cause_t doLightSleep(uint64_t sleepMsec) // FIXME, use a more r
 #ifdef BUTTON_PIN
     // The enableLoraInterrupt() method is using ext0_wakeup, so we are forced to use GPIO wakeup
     gpio_num_t pin = (gpio_num_t)(config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN);
-    gpio_intr_disable(pin);
+
+    // Have to *fully* detach the normal button-interrupts first
+    buttonThread->detachButtonInterrupts();
+
     gpio_wakeup_enable(pin, GPIO_INTR_LOW_LEVEL);
     esp_sleep_enable_gpio_wakeup();
 #endif
@@ -364,9 +368,9 @@ esp_sleep_wakeup_cause_t doLightSleep(uint64_t sleepMsec) // FIXME, use a more r
     assert(res == ESP_OK);
 
 #ifdef BUTTON_PIN
+    // Disable wake-on-button interrupt. Re-attach normal button-interrupts
     gpio_wakeup_disable(pin);
-    // Would have thought that need gpio_intr_enable() here, but nope..
+    buttonThread->attachButtonInterrupts();
-    // Works fine without it; crashes with it.
 #endif
 
     esp_sleep_wakeup_cause_t cause = esp_sleep_get_wakeup_cause();