Fix #8899 [Bug]: [TloraPager] RotaryEncoder crash (#8933)

* Fix #8899 [Bug]: [TloraPager] RotaryEncoder crash

* Apply Copilot review

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

.github/actions/build-variant/action.yml

@@ -76,7 +76,7 @@ runs:
         done
 
     - name: PlatformIO ${{ inputs.arch }} download cache
-      uses: actions/cache@v4
+      uses: actions/cache@v5
       with:
         path: ~/.platformio/.cache
         key: pio-cache-${{ inputs.arch }}-${{ hashFiles('.github/actions/**', '**.ini') }}

.github/workflows/main_matrix.yml

@@ -77,16 +77,21 @@ jobs:
       fail-fast: false
       matrix:
         check: ${{ fromJson(needs.setup.outputs.check) }}
-
-    runs-on: ubuntu-latest
+    # Use 'arctastic' self-hosted runner pool when checking in the main repo
+    runs-on: ${{ github.repository_owner == 'meshtastic' && 'arctastic' || 'ubuntu-latest' }}
     if: ${{ github.event_name != 'workflow_dispatch' && github.repository == 'meshtastic/firmware' }}
     steps:
       - uses: actions/checkout@v6
-      - name: Build base
-        id: base
-        uses: ./.github/actions/setup-base
+        with:
+          submodules: recursive
+          ref: ${{github.event.pull_request.head.ref}}
+          repository: ${{github.event.pull_request.head.repo.full_name}}
       - name: Check ${{ matrix.check.board }}
-        run: bin/check-all.sh ${{ matrix.check.board }}
+        uses: meshtastic/gh-action-firmware@main
+        with:
+          pio_platform: ${{ matrix.check.platform }}
+          pio_env: ${{ matrix.check.board }}
+          pio_target: check
 
   build:
     needs: [setup, version]

bin/build-esp32.sh

@@ -22,7 +22,7 @@ export APP_VERSION=$VERSION
 
 basename=firmware-$1-$VERSION
 
-pio run --environment $1 # -v
+pio run --environment $1 -t mtjson # -v
 
 cp $BUILDDIR/$basename.elf $OUTDIR/$basename.elf
 
@@ -32,20 +32,10 @@ cp $BUILDDIR/$basename.factory.bin $OUTDIR/$basename.factory.bin
 echo "Copying ESP32 update bin file"
 cp $BUILDDIR/$basename.bin $OUTDIR/$basename.bin
 
-echo "Building Filesystem for ESP32 targets"
-# If you want to build the webui, uncomment the following lines
-# pio run --environment $1 -t buildfs
-# cp .pio/build/$1/littlefs.bin $OUTDIR/littlefswebui-$1-$VERSION.bin
-# # Remove webserver files from the filesystem and rebuild
-# ls -l data/static # Diagnostic list of files
-# rm -rf data/static
-pio run --environment $1 -t buildfs --disable-auto-clean
+echo "Copying Filesystem for ESP32 targets"
 cp $BUILDDIR/littlefs-$1-$VERSION.bin $OUTDIR/littlefs-$1-$VERSION.bin
 cp bin/device-install.* $OUTDIR/
 cp bin/device-update.* $OUTDIR/
 
-# Generate the manifest file
-echo "Generating Meshtastic manifest"
-TIMEFORMAT="Generated manifest in %E seconds"
-time pio run --environment $1 -t mtjson --silent --disable-auto-clean
+echo "Copying manifest"
 cp $BUILDDIR/$basename.mt.json $OUTDIR/$basename.mt.json

bin/build-nrf52.sh

@@ -22,7 +22,7 @@ export APP_VERSION=$VERSION
 
 basename=firmware-$1-$VERSION
 
-pio run --environment $1 # -v
+pio run --environment $1 -t mtjson # -v
 
 cp $BUILDDIR/$basename.elf $OUTDIR/$basename.elf
 
@@ -47,8 +47,5 @@ if (echo $1 | grep -q "rak4631"); then
 	cp $SRCHEX $OUTDIR/
 fi
 
-# Generate the manifest file
-echo "Generating Meshtastic manifest"
-TIMEFORMAT="Generated manifest in %E seconds"
-time pio run --environment $1 -t mtjson --silent --disable-auto-clean
+echo "Copying manifest"
 cp $BUILDDIR/$basename.mt.json $OUTDIR/$basename.mt.json

bin/build-rp2xx0.sh

@@ -22,15 +22,12 @@ export APP_VERSION=$VERSION
 
 basename=firmware-$1-$VERSION
 
-pio run --environment $1 # -v
+pio run --environment $1 -t mtjson # -v
 
 cp $BUILDDIR/$basename.elf $OUTDIR/$basename.elf
 
 echo "Copying uf2 file"
 cp $BUILDDIR/$basename.uf2 $OUTDIR/$basename.uf2
 
-# Generate the manifest file
-echo "Generating Meshtastic manifest"
-TIMEFORMAT="Generated manifest in %E seconds"
-time pio run --environment $1 -t mtjson --silent --disable-auto-clean
+echo "Copying manifest"
 cp $BUILDDIR/$basename.mt.json $OUTDIR/$basename.mt.json

bin/build-stm32wl.sh

@@ -22,15 +22,12 @@ export APP_VERSION=$VERSION
 
 basename=firmware-$1-$VERSION
 
-pio run --environment $1 # -v
+pio run --environment $1 -t mtjson # -v
 
 cp $BUILDDIR/$basename.elf $OUTDIR/$basename.elf
 
 echo "Copying STM32 bin file"
 cp $BUILDDIR/$basename.bin $OUTDIR/$basename.bin
 
-# Generate the manifest file
-echo "Generating Meshtastic manifest"
-TIMEFORMAT="Generated manifest in %E seconds"
-time pio run --environment $1 -t mtjson --silent --disable-auto-clean
+echo "Copying manifest"
 cp $BUILDDIR/$basename.mt.json $OUTDIR/$basename.mt.json

bin/platformio-custom.py

@@ -159,20 +159,22 @@ def load_boot_logo(source, target, env):
 
 # Load the boot logo on TFT builds
 if ("HAS_TFT", 1) in env.get("CPPDEFINES", []):
-    env.AddPreAction('$BUILD_DIR/littlefs.bin', load_boot_logo)
+    env.AddPreAction(f"$BUILD_DIR/{lfsbin}", load_boot_logo)
 
-# Rename (mv) littlefs.bin to include the PROGNAME
-# This ensures the littlefs.bin is named consistently with the firmware
-env.AddPostAction('$BUILD_DIR/littlefs.bin', env.VerboseAction(
-    f'mv $BUILD_DIR/littlefs.bin $BUILD_DIR/{lfsbin}',
-    f'Renaming littlefs.bin to {lfsbin}'
-))
+mtjson_deps = ["buildprog"]
+if platform.name == "espressif32":
+    # Build littlefs image as part of mtjson target
+    # Equivalent to `pio run -t buildfs`
+    target_lfs = env.DataToBin(
+        join("$BUILD_DIR", "${ESP32_FS_IMAGE_NAME}"), "$PROJECT_DATA_DIR"
+    )
+    mtjson_deps.append(target_lfs)
 
 env.AddCustomTarget(
     name="mtjson",
-    dependencies=None,
+    dependencies=mtjson_deps,
     actions=[manifest_gather],
     title="Meshtastic Manifest",
     description="Generating Meshtastic manifest JSON + Checksums",
-    always_build=True,
+    always_build=False,
 )

bin/platformio-pre.py

@@ -11,6 +11,9 @@ else:
     prefsLoc = env["PROJECT_DIR"] + "/version.properties"
     verObj = readProps(prefsLoc)
     env.Replace(PROGNAME=f"firmware-{env.get('PIOENV')}-{verObj['long']}")
+    env.Replace(ESP32_FS_IMAGE_NAME=f"littlefs-{env.get('PIOENV')}-{verObj['long']}")
 
 # Print the new program name for verification
 print(f"PROGNAME: {env.get('PROGNAME')}")
+if platform.name == "espressif32":
+    print(f"ESP32_FS_IMAGE_NAME: {env.get('ESP32_FS_IMAGE_NAME')}")

src/input/InputBroker.h

@@ -53,6 +53,7 @@ typedef struct _InputEvent {
 class InputPollable
 {
   public:
+    virtual ~InputPollable() = default;
     virtual void pollOnce() = 0;
 };
 

src/input/RotaryEncoderImpl.cpp

@@ -3,6 +3,9 @@
 #include "RotaryEncoderImpl.h"
 #include "InputBroker.h"
 #include "RotaryEncoder.h"
+#ifdef ARCH_ESP32
+#include "sleep.h"
+#endif
 
 #define ORIGIN_NAME "RotaryEncoder"
 
@@ -11,6 +14,20 @@ RotaryEncoderImpl *rotaryEncoderImpl;
 RotaryEncoderImpl::RotaryEncoderImpl()
 {
     rotary = nullptr;
+#ifdef ARCH_ESP32
+    isFirstInit = true;
+#endif
+}
+
+RotaryEncoderImpl::~RotaryEncoderImpl()
+{
+    LOG_DEBUG("RotaryEncoderImpl destructor");
+    detachRotaryEncoderInterrupts();
+
+    if (rotary != nullptr) {
+        delete rotary;
+        rotary = nullptr;
+    }
 }
 
 bool RotaryEncoderImpl::init()
@@ -25,15 +42,22 @@ bool RotaryEncoderImpl::init()
     eventCcw = static_cast<input_broker_event>(moduleConfig.canned_message.inputbroker_event_ccw);
     eventPressed = static_cast<input_broker_event>(moduleConfig.canned_message.inputbroker_event_press);
 
-    rotary = new RotaryEncoder(moduleConfig.canned_message.inputbroker_pin_a, moduleConfig.canned_message.inputbroker_pin_b,
-                               moduleConfig.canned_message.inputbroker_pin_press);
-    rotary->resetButton();
+    if (rotary == nullptr) {
+        rotary = new RotaryEncoder(moduleConfig.canned_message.inputbroker_pin_a, moduleConfig.canned_message.inputbroker_pin_b,
+                                   moduleConfig.canned_message.inputbroker_pin_press);
+    }
 
-    interruptInstance = this;
-    auto interruptHandler = []() { inputBroker->requestPollSoon(interruptInstance); };
-    attachInterrupt(moduleConfig.canned_message.inputbroker_pin_a, interruptHandler, CHANGE);
-    attachInterrupt(moduleConfig.canned_message.inputbroker_pin_b, interruptHandler, CHANGE);
-    attachInterrupt(moduleConfig.canned_message.inputbroker_pin_press, interruptHandler, CHANGE);
+    attachRotaryEncoderInterrupts();
+
+#ifdef ARCH_ESP32
+    // Register callbacks for before and after lightsleep
+    // Used to detach and reattach interrupts
+    if (isFirstInit) {
+        lsObserver.observe(&notifyLightSleep);
+        lsEndObserver.observe(&notifyLightSleepEnd);
+        isFirstInit = false;
+    }
+#endif
 
     LOG_INFO("RotaryEncoder initialized pins(%d, %d, %d), events(%d, %d, %d)", moduleConfig.canned_message.inputbroker_pin_a,
              moduleConfig.canned_message.inputbroker_pin_b, moduleConfig.canned_message.inputbroker_pin_press, eventCw, eventCcw,
@@ -71,6 +95,50 @@ void RotaryEncoderImpl::pollOnce()
     }
 }
 
+void RotaryEncoderImpl::detachRotaryEncoderInterrupts()
+{
+    LOG_DEBUG("RotaryEncoderImpl detach button interrupts");
+    if (interruptInstance == this) {
+        detachInterrupt(moduleConfig.canned_message.inputbroker_pin_a);
+        detachInterrupt(moduleConfig.canned_message.inputbroker_pin_b);
+        detachInterrupt(moduleConfig.canned_message.inputbroker_pin_press);
+        interruptInstance = nullptr;
+    } else {
+        LOG_WARN("RotaryEncoderImpl: interrupts already detached");
+    }
+}
+
+void RotaryEncoderImpl::attachRotaryEncoderInterrupts()
+{
+    LOG_DEBUG("RotaryEncoderImpl attach button interrupts");
+    if (rotary != nullptr && interruptInstance == nullptr) {
+        rotary->resetButton();
+
+        interruptInstance = this;
+        auto interruptHandler = []() { inputBroker->requestPollSoon(interruptInstance); };
+        attachInterrupt(moduleConfig.canned_message.inputbroker_pin_a, interruptHandler, CHANGE);
+        attachInterrupt(moduleConfig.canned_message.inputbroker_pin_b, interruptHandler, CHANGE);
+        attachInterrupt(moduleConfig.canned_message.inputbroker_pin_press, interruptHandler, CHANGE);
+    } else {
+        LOG_WARN("RotaryEncoderImpl: interrupts already attached");
+    }
+}
+
+#ifdef ARCH_ESP32
+
+int RotaryEncoderImpl::beforeLightSleep(void *unused)
+{
+    detachRotaryEncoderInterrupts();
+    return 0; // Indicates success;
+}
+
+int RotaryEncoderImpl::afterLightSleep(esp_sleep_wakeup_cause_t cause)
+{
+    attachRotaryEncoderInterrupts();
+    return 0; // Indicates success;
+}
+#endif
+
 RotaryEncoderImpl *RotaryEncoderImpl::interruptInstance;
 
 #endif

src/input/RotaryEncoderImpl.h

@@ -8,12 +8,18 @@
 
 class RotaryEncoder;
 
-class RotaryEncoderImpl : public InputPollable
+class RotaryEncoderImpl final : public InputPollable
 {
   public:
     RotaryEncoderImpl();
-    bool init(void);
+    ~RotaryEncoderImpl() override;
+    bool init();
     virtual void pollOnce() override;
+    // Disconnect and reconnect interrupts for light sleep
+#ifdef ARCH_ESP32
+    int beforeLightSleep(void *unused);
+    int afterLightSleep(esp_sleep_wakeup_cause_t cause);
+#endif
 
   protected:
     static RotaryEncoderImpl *interruptInstance;
@@ -23,6 +29,21 @@ class RotaryEncoderImpl : public InputPollable
     input_broker_event eventPressed = INPUT_BROKER_NONE;
 
     RotaryEncoder *rotary;
+
+  private:
+#ifdef ARCH_ESP32
+    bool isFirstInit;
+#endif
+    void detachRotaryEncoderInterrupts();
+    void attachRotaryEncoderInterrupts();
+
+#ifdef ARCH_ESP32
+    // Get notified when lightsleep begins and ends
+    CallbackObserver<RotaryEncoderImpl, void *> lsObserver =
+        CallbackObserver<RotaryEncoderImpl, void *>(this, &RotaryEncoderImpl::beforeLightSleep);
+    CallbackObserver<RotaryEncoderImpl, esp_sleep_wakeup_cause_t> lsEndObserver =
+        CallbackObserver<RotaryEncoderImpl, esp_sleep_wakeup_cause_t>(this, &RotaryEncoderImpl::afterLightSleep);
+#endif
 };
 
 extern RotaryEncoderImpl *rotaryEncoderImpl;