refactored ButtonThread, fix IRQ issues (#3214)

* refactored  ButtonThread, fix IRQ issues

* fix copy&paste syntax error

src/ButtonThread.cpp

@@ -0,0 +1,219 @@
+#include "ButtonThread.h"
+#include "GPS.h"
+#include "MeshService.h"
+#include "PowerFSM.h"
+#include "RadioLibInterface.h"
+#include "buzz.h"
+#include "graphics/Screen.h"
+#include "main.h"
+#include "modules/ExternalNotificationModule.h"
+#include "power.h"
+#ifdef ARCH_PORTDUINO
+#include "platform/portduino/PortduinoGlue.h"
+#endif
+
+#define DEBUG_BUTTONS 0
+#if DEBUG_BUTTONS
+#define LOG_BUTTON(...) LOG_DEBUG(__VA_ARGS__)
+#else
+#define LOG_BUTTON(...)
+#endif
+
+using namespace concurrency;
+
+volatile ButtonThread::ButtonEventType ButtonThread::btnEvent = ButtonThread::BUTTON_EVENT_NONE;
+
+ButtonThread::ButtonThread() : OSThread("Button")
+{
+#if defined(ARCH_PORTDUINO) || defined(BUTTON_PIN)
+#if defined(ARCH_PORTDUINO)
+    if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC) {
+        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;
+    this->userButton = OneButton(pin, true, true);
+    LOG_DEBUG("Using GPIO%02d for button\n", pin);
+#endif
+
+#ifdef INPUT_PULLUP_SENSE
+    // Some platforms (nrf52) have a SENSE variant which allows wake from sleep - override what OneButton did
+    pinMode(pin, INPUT_PULLUP_SENSE);
+#endif
+    userButton.attachClick(userButtonPressed);
+    userButton.setClickMs(250);
+    userButton.setPressMs(c_longPressTime);
+    userButton.setDebounceMs(1);
+    userButton.attachDoubleClick(userButtonDoublePressed);
+    userButton.attachMultiClick(userButtonMultiPressed);
+#ifndef T_DECK // T-Deck immediately wakes up after shutdown, so disable this function
+    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
+    // Some platforms (nrf52) have a SENSE variant which allows wake from sleep - override what OneButton did
+    pinMode(BUTTON_PIN_ALT, INPUT_PULLUP_SENSE);
+#endif
+    userButtonAlt.attachClick(userButtonPressed);
+    userButtonAlt.setClickMs(250);
+    userButtonAlt.setPressMs(c_longPressTime);
+    userButtonAlt.setDebounceMs(1);
+    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.attachClick(touchPressed);
+    wakeOnIrq(BUTTON_PIN_TOUCH, FALLING);
+#endif
+}
+
+int32_t ButtonThread::runOnce()
+{
+    // If the button is pressed we suppress CPU sleep until release
+    canSleep = true; // Assume we should not keep the board awake
+
+#if defined(BUTTON_PIN)
+    userButton.tick();
+    canSleep &= userButton.isIdle();
+#elif defined(ARCH_PORTDUINO)
+    if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC) {
+        userButton.tick();
+        canSleep &= userButton.isIdle();
+    }
+#endif
+#ifdef BUTTON_PIN_ALT
+    userButtonAlt.tick();
+    canSleep &= userButtonAlt.isIdle();
+#endif
+#ifdef BUTTON_PIN_TOUCH
+    userButtonTouch.tick();
+    canSleep &= userButtonTouch.isIdle();
+#endif
+
+    if (btnEvent != BUTTON_EVENT_NONE) {
+        switch (btnEvent) {
+        case BUTTON_EVENT_PRESSED: {
+            LOG_BUTTON("press!\n");
+#ifdef BUTTON_PIN
+            if (((config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN) !=
+                 moduleConfig.canned_message.inputbroker_pin_press) ||
+                !(moduleConfig.canned_message.updown1_enabled || moduleConfig.canned_message.rotary1_enabled) ||
+                !moduleConfig.canned_message.enabled) {
+                powerFSM.trigger(EVENT_PRESS);
+            }
+#endif
+#if defined(ARCH_PORTDUINO)
+            if ((settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC) &&
+                    (settingsMap[user] != moduleConfig.canned_message.inputbroker_pin_press) ||
+                !moduleConfig.canned_message.enabled) {
+                powerFSM.trigger(EVENT_PRESS);
+            }
+#endif
+            break;
+        }
+
+        case BUTTON_EVENT_DOUBLE_PRESSED: {
+            LOG_BUTTON("Double press!\n");
+#if defined(USE_EINK) && defined(PIN_EINK_EN)
+            digitalWrite(PIN_EINK_EN, digitalRead(PIN_EINK_EN) == LOW);
+#endif
+            service.refreshLocalMeshNode();
+            service.sendNetworkPing(NODENUM_BROADCAST, true);
+            if (screen)
+                screen->print("Sent ad-hoc ping\n");
+            break;
+        }
+
+        case BUTTON_EVENT_MULTI_PRESSED: {
+            LOG_BUTTON("Multi press!\n");
+            if (!config.device.disable_triple_click && (gps != nullptr)) {
+                gps->toggleGpsMode();
+                if (screen)
+                    screen->forceDisplay();
+            }
+            break;
+        }
+
+        case BUTTON_EVENT_LONG_PRESSED: {
+            LOG_BUTTON("Long press!\n");
+            powerFSM.trigger(EVENT_PRESS);
+            if (screen)
+                screen->startShutdownScreen();
+            playBeep();
+            break;
+        }
+
+        // Do actual shutdown when button released, otherwise the button release
+        // may wake the board immediatedly.
+        case BUTTON_EVENT_LONG_RELEASED: {
+            LOG_INFO("Shutdown from long press\n");
+            playShutdownMelody();
+            delay(3000);
+            power->shutdown();
+            break;
+        }
+        case BUTTON_EVENT_TOUCH_PRESSED: {
+            LOG_BUTTON("Touch press!\n");
+            if (screen)
+                screen->forceDisplay();
+            break;
+        }
+        default:
+            break;
+        }
+        btnEvent = BUTTON_EVENT_NONE;
+    }
+
+    return 50;
+}
+
+/**
+ * Watch a GPIO and if we get an IRQ, wake the main thread.
+ * Use to add wake on button press
+ */
+void ButtonThread::wakeOnIrq(int irq, int mode)
+{
+    attachInterrupt(
+        irq,
+        [] {
+            BaseType_t higherWake = 0;
+            mainDelay.interruptFromISR(&higherWake);
+        },
+        FALLING);
+}
+
+void ButtonThread::userButtonPressedLongStart()
+{
+    if (millis() > c_holdOffTime) {
+        btnEvent = BUTTON_EVENT_LONG_PRESSED;
+    }
+}
+
+void ButtonThread::userButtonPressedLongStop()
+{
+    if (millis() > c_holdOffTime) {
+        btnEvent = BUTTON_EVENT_LONG_RELEASED;
+    }
+}

src/ButtonThread.h

@@ -1,34 +1,29 @@
-#include "PowerFSM.h"
+#pragma once
-#include "RadioLibInterface.h"
+
-#include "buzz.h"
+#include "OneButton.h"
 #include "concurrency/OSThread.h"
 #include "configuration.h"
-#include "graphics/Screen.h"
-#include "main.h"
-#include "modules/ExternalNotificationModule.h"
-#include "power.h"
-#include <OneButton.h>
-
-namespace concurrency
-{
-/**
- * Watch a GPIO and if we get an IRQ, wake the main thread.
- * Use to add wake on button press
- */
-void wakeOnIrq(int irq, int mode)
-{
-    attachInterrupt(
-        irq,
-        [] {
-            BaseType_t higherWake = 0;
-            mainDelay.interruptFromISR(&higherWake);
-        },
-        FALLING);
-}
 
 class ButtonThread : public concurrency::OSThread
 {
-// Prepare for button presses
+  public:
+    static const uint32_t c_longPressTime = 5000; // shutdown after 5s
+    static const uint32_t c_holdOffTime = 30000;  // hold off 30s after boot
+
+    enum ButtonEventType {
+        BUTTON_EVENT_NONE,
+        BUTTON_EVENT_PRESSED,
+        BUTTON_EVENT_DOUBLE_PRESSED,
+        BUTTON_EVENT_MULTI_PRESSED,
+        BUTTON_EVENT_LONG_PRESSED,
+        BUTTON_EVENT_LONG_RELEASED,
+        BUTTON_EVENT_TOUCH_PRESSED
+    };
+
+    ButtonThread();
+    int32_t runOnce() override;
+
+  private:
 #ifdef BUTTON_PIN
     OneButton userButton;
 #endif
@@ -41,200 +36,17 @@ class ButtonThread : public concurrency::OSThread
 #if defined(ARCH_PORTDUINO)
     OneButton userButton;
 #endif
-    static bool shutdown_on_long_stop;
 
-  public:
+    // set during IRQ
-    static uint32_t longPressTime;
+    static volatile ButtonEventType btnEvent;
 
-    // callback returns the period for the next callback invocation (or 0 if we should no longer be called)
+    static void wakeOnIrq(int irq, int mode);
-    ButtonThread() : OSThread("Button")
-    {
-#if defined(ARCH_PORTDUINO) || defined(BUTTON_PIN)
-#if defined(ARCH_PORTDUINO)
-        if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC)
-            userButton = OneButton(settingsMap[user], true, true);
-#elif defined(BUTTON_PIN)
-        int pin = config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN;
-        userButton = OneButton(pin, true, true);
-#endif
 
-#ifdef INPUT_PULLUP_SENSE
+    // IRQ callbacks
-        // Some platforms (nrf52) have a SENSE variant which allows wake from sleep - override what OneButton did
+    static void touchPressed() { btnEvent = BUTTON_EVENT_TOUCH_PRESSED; }
-        pinMode(pin, INPUT_PULLUP_SENSE);
+    static void userButtonPressed() { btnEvent = BUTTON_EVENT_PRESSED; }
-#endif
+    static void userButtonDoublePressed() { btnEvent = BUTTON_EVENT_DOUBLE_PRESSED; }
-        userButton.attachClick(userButtonPressed);
+    static void userButtonMultiPressed() { btnEvent = BUTTON_EVENT_MULTI_PRESSED; }
-        userButton.setClickMs(400);
+    static void userButtonPressedLongStart();
-        userButton.setPressMs(1000);
+    static void userButtonPressedLongStop();
-        userButton.setDebounceMs(10);
-        userButton.attachDuringLongPress(userButtonPressedLong);
-        userButton.attachDoubleClick(userButtonDoublePressed);
-        userButton.attachMultiClick(userButtonMultiPressed);
-        userButton.attachLongPressStart(userButtonPressedLongStart);
-        userButton.attachLongPressStop(userButtonPressedLongStop);
-#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
-        // Some platforms (nrf52) have a SENSE variant which allows wake from sleep - override what OneButton did
-        pinMode(BUTTON_PIN_ALT, INPUT_PULLUP_SENSE);
-#endif
-        userButtonAlt.attachClick(userButtonPressed);
-        userButtonAlt.attachDuringLongPress(userButtonPressedLong);
-        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.attachClick(touchPressed);
-        wakeOnIrq(BUTTON_PIN_TOUCH, FALLING);
-#endif
-    }
-
-  protected:
-    /// If the button is pressed we suppress CPU sleep until release
-    int32_t runOnce() override
-    {
-        canSleep = true; // Assume we should not keep the board awake
-
-#if defined(BUTTON_PIN)
-        userButton.tick();
-        canSleep &= userButton.isIdle();
-#elif defined(ARCH_PORTDUINO)
-        if (settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC) {
-            userButton.tick();
-            canSleep &= userButton.isIdle();
-        }
-#endif
-#ifdef BUTTON_PIN_ALT
-        userButtonAlt.tick();
-        canSleep &= userButtonAlt.isIdle();
-#endif
-#ifdef BUTTON_PIN_TOUCH
-        userButtonTouch.tick();
-        canSleep &= userButtonTouch.isIdle();
-#endif
-        // if (!canSleep) LOG_DEBUG("Suppressing sleep!\n");
-        // else LOG_DEBUG("sleep ok\n");
-
-        return 50;
-    }
-
-  private:
-    static void touchPressed()
-    {
-        screen->forceDisplay();
-        LOG_DEBUG("touch press!\n");
-    }
-
-    static void userButtonPressed()
-    {
-        // LOG_DEBUG("press!\n");
-#ifdef BUTTON_PIN
-        if (((config.device.button_gpio ? config.device.button_gpio : BUTTON_PIN) !=
-             moduleConfig.canned_message.inputbroker_pin_press) ||
-            !(moduleConfig.canned_message.updown1_enabled || moduleConfig.canned_message.rotary1_enabled) ||
-            !moduleConfig.canned_message.enabled) {
-            powerFSM.trigger(EVENT_PRESS);
-        }
-#endif
-#if defined(ARCH_PORTDUINO)
-        if ((settingsMap.count(user) != 0 && settingsMap[user] != RADIOLIB_NC) &&
-                (settingsMap[user] != moduleConfig.canned_message.inputbroker_pin_press) ||
-            !moduleConfig.canned_message.enabled) {
-            powerFSM.trigger(EVENT_PRESS);
-        }
-#endif
-    }
-    static void userButtonPressedLong()
-    {
-        // LOG_DEBUG("Long press!\n");
-        // If user button is held down for 5 seconds, shutdown the device.
-        if ((millis() - longPressTime > 5000) && (longPressTime > 0)) {
-#if defined(ARCH_NRF52) || defined(ARCH_ESP32)
-            // Do actual shutdown when button released, otherwise the button release
-            // may wake the board immediatedly.
-            if ((!shutdown_on_long_stop) && (millis() > 30 * 1000)) {
-                screen->startShutdownScreen();
-                LOG_INFO("Shutdown from long press");
-                playBeep();
-#ifdef PIN_LED1
-                ledOff(PIN_LED1);
-#endif
-#ifdef PIN_LED2
-                ledOff(PIN_LED2);
-#endif
-#ifdef PIN_LED3
-                ledOff(PIN_LED3);
-#endif
-                shutdown_on_long_stop = true;
-            }
-#endif
-        } else {
-            // LOG_DEBUG("Long press %u\n", (millis() - longPressTime));
-        }
-    }
-
-    static void userButtonDoublePressed()
-    {
-#if defined(USE_EINK) && defined(PIN_EINK_EN)
-        digitalWrite(PIN_EINK_EN, digitalRead(PIN_EINK_EN) == LOW);
-#endif
-        screen->print("Sent ad-hoc ping\n");
-        service.refreshLocalMeshNode();
-        service.sendNetworkPing(NODENUM_BROADCAST, true);
-    }
-
-    static void userButtonMultiPressed()
-    {
-        if (!config.device.disable_triple_click && (gps != nullptr)) {
-            gps->toggleGpsMode();
-            screen->forceDisplay();
-        }
-    }
-
-    static void userButtonPressedLongStart()
-    {
-#ifdef T_DECK
-        // False positive long-press triggered on T-Deck with i2s audio, so short circuit
-        if (moduleConfig.external_notification.enabled && (externalNotificationModule->nagCycleCutoff != UINT32_MAX)) {
-            return;
-        }
-#endif
-        if (millis() > 30 * 1000) {
-            LOG_DEBUG("Long press start!\n");
-            longPressTime = millis();
-        }
-    }
-
-    static void userButtonPressedLongStop()
-    {
-        if (millis() > 30 * 1000) {
-            LOG_DEBUG("Long press stop!\n");
-            longPressTime = 0;
-            if (shutdown_on_long_stop) {
-                playShutdownMelody();
-                delay(3000);
-                power->shutdown();
-            }
-        }
-    }
 };
-
-} // namespace concurrency

src/main.cpp

@@ -191,15 +191,10 @@ static int32_t ledBlinker()
 
 uint32_t timeLastPowered = 0;
 
-#if HAS_BUTTON || defined(ARCH_PORTDUINO)
-bool ButtonThread::shutdown_on_long_stop = false;
-#endif
-
 static Periodic *ledPeriodic;
 static OSThread *powerFSMthread;
 #if HAS_BUTTON || defined(ARCH_PORTDUINO)
 static OSThread *buttonThread;
-uint32_t ButtonThread::longPressTime = 0;
 #endif
 static OSThread *accelerometerThread;
 static OSThread *ambientLightingThread;