feat: take the SPI bus lock around our own transfers

include/graphics/DeviceScreen.h

@@ -3,6 +3,7 @@
 #include "comms/IClientBase.h"
 #include "graphics/DeviceGUI.h"
 #include "graphics/driver/DisplayDriverConfig.h"
+#include "util/ISpiLock.h"
 
 #if defined(ARDUINO_ARCH_ESP32)
 #include "esp_sleep.h"
@@ -16,8 +17,18 @@ class DeviceScreen
 {
   public:
     static DeviceScreen &create(void);
-    static DeviceScreen &create(const DisplayDriverConfig *cfg);
-    static DeviceScreen &create(DisplayDriverConfig &&cfg);
+
+    /**
+     * @param spiLock the host's bus lock, for boards where the display or SD card shares
+     * an SPI bus with peripherals the host drives itself. See ISpiLock - note the
+     * reentrancy requirement.
+     *
+     * Taken by reference rather than pointer so that create(nullptr) stays unambiguous
+     * against the config-taking overloads below.
+     */
+    static DeviceScreen &create(ISpiLock &spiLock);
+    static DeviceScreen &create(const DisplayDriverConfig *cfg, ISpiLock *spiLock = nullptr);
+    static DeviceScreen &create(DisplayDriverConfig &&cfg, ISpiLock *spiLock = nullptr);
 
     void init(IClientBase *client);
     void task_handler(void);

include/graphics/driver/LGFXDriver.h

@@ -6,6 +6,7 @@
 #include "input/InputDriver.h"
 #include "lvgl_private.h"
 #include "util/ILog.h"
+#include "util/ISpiLock.h"
 #include <functional>
 
 constexpr uint32_t defaultLongPressTime = 700; // ms until long press is detected (lvgl default is 400)
@@ -103,8 +104,11 @@ template <class LGFX> void LGFXDriver<LGFX>::task_handler(void)
                             lv_indev_enable(DisplayDriver::touch, false);
                             lv_indev_enable(InputDriver::instance()->getButton(), true);
                         }
-                        lgfx->sleep();
-                        lgfx->powerSaveOn();
+                        {
+                            ISpiLock::Guard bus;
+                            lgfx->sleep();
+                            lgfx->powerSaveOn();
+                        }
                         powerSaving = true;
                     }
                 }
@@ -129,9 +133,14 @@ template <class LGFX> void LGFXDriver<LGFX>::task_handler(void)
                         ILOG_INFO("leaving powersave");
                         powerSaving = false;
                         DisplayDriver::view->triggerHeartbeat();
-                        lgfx->powerSaveOff();
-                        lgfx->wakeup();
-                        lgfx->setBrightness(lastBrightness);
+                        {
+                            // Scoped so the bus is not held across view->sleep() above,
+                            // which suspends the whole SoC in light sleep.
+                            ISpiLock::Guard bus;
+                            lgfx->powerSaveOff();
+                            lgfx->wakeup();
+                            lgfx->setBrightness(lastBrightness);
+                        }
                         DisplayDriver::view->screenSaving(false);
                         if (hasTouch() && hasButton()) {
                             ILOG_DEBUG("enable touch, disable button input");
@@ -149,14 +158,20 @@ template <class LGFX> void LGFXDriver<LGFX>::task_handler(void)
             else {
                 if (!powerSaving) {
                     DisplayDriver::view->blankScreen(true);
-                    lgfx->sleep();
-                    lgfx->powerSaveOn();
+                    {
+                        ISpiLock::Guard bus;
+                        lgfx->sleep();
+                        lgfx->powerSaveOn();
+                    }
                     powerSaving = true;
                 }
                 if (screenTimeout > lv_display_get_inactive_time(NULL)) {
                     DisplayDriver::view->blankScreen(false);
-                    lgfx->powerSaveOff();
-                    lgfx->wakeup();
+                    {
+                        ISpiLock::Guard bus;
+                        lgfx->powerSaveOff();
+                        lgfx->wakeup();
+                    }
                     powerSaving = false;
                     lv_disp_trig_activity(NULL);
                 }
@@ -178,8 +193,11 @@ template <class LGFX> void LGFXDriver<LGFX>::display_flush(lv_display_t *disp, c
 {
     uint32_t w = lv_area_get_width(area);
     uint32_t h = lv_area_get_height(area);
-    lv_draw_sw_rgb565_swap(px_map, w * h);
-    lgfx->pushImage(area->x1, area->y1, w, h, (uint16_t *)px_map);
+    lv_draw_sw_rgb565_swap(px_map, w * h); // CPU only - deliberately outside the guard
+    {
+        ISpiLock::Guard bus;
+        lgfx->pushImage(area->x1, area->y1, w, h, (uint16_t *)px_map);
+    }
     lv_display_flush_ready(disp);
 }
 #else
@@ -220,9 +238,14 @@ template <class LGFX> void LGFXDriver<LGFX>::touchpad_read(lv_indev_t *indev_dri
 {
     uint16_t touchX = 0, touchY = 0;
 #ifdef CUSTOM_TOUCH_DRIVER
-    bool touched = lgfx->getTouchXY(&touchX, &touchY);
+    bool touched = lgfx->getTouchXY(&touchX, &touchY); // I2C, no bus guard needed
 #else
-    bool touched = lgfx->getTouch(&touchX, &touchY);
+    // XPT2046/STMPE610 sit on SPI, and on several boards on the panel's own host.
+    bool touched;
+    {
+        ISpiLock::Guard bus;
+        touched = lgfx->getTouch(&touchX, &touchY);
+    }
 #endif
     if (!touched) {
         data->state = LV_INDEV_STATE_REL;
@@ -343,9 +366,12 @@ template <class LGFX> void LGFXDriver<LGFX>::init_lgfx(void)
 {
     // Initialize LovyanGFX
     ILOG_DEBUG("LGFX init...");
-    lgfx->init();
-    lgfx->setBrightness(defaultBrightness);
-    lgfx->fillScreen(LGFX::color565(0x3D, 0xDA, 0x83));
+    {
+        ISpiLock::Guard bus;
+        lgfx->init();
+        lgfx->setBrightness(defaultBrightness);
+        lgfx->fillScreen(LGFX::color565(0x3D, 0xDA, 0x83));
+    }
 
     if (hasTouch()) {
 #ifndef CUSTOM_TOUCH_DRIVER
@@ -393,6 +419,15 @@ template <class LGFX> bool LGFXDriver<LGFX>::calibrate(uint16_t parameters[8])
         calibrating = true;
         std::uint16_t fg = TFT_BLUE;
         std::uint16_t bg = LGFX::color565(0x67, 0xEA, 0x94);
+        // calibrateTouch() blocks until the user has tapped every marker, and the bus
+        // stays held throughout. Deliberate: LovyanGFX runs its own draw/read loop with
+        // no hook to release in the middle, and dropping the guard would leave this the
+        // one unprotected SPI path now that the host no longer holds a coarse lock. It
+        // is also not a regression - the host used to hold that lock across this same
+        // call. Note the branch is only reached when a board ships no stored calibration
+        // parameters; every board in-tree supplies them, so in practice we take the
+        // non-blocking setTouchCalibrate() path above.
+        ISpiLock::Guard bus;
         lgfx->clearDisplay();
         lgfx->fillScreen(LGFX::color565(0x67, 0xEA, 0x94));
         lgfx->setTextSize(1);
@@ -422,7 +457,11 @@ template <class LGFX> void LGFXDriver<LGFX>::printConfig(void)
     if (lgfx->panel()) {
         auto p = lgfx->panel();
         auto cfg = p->config();
-        uint32_t id = p->readCommand(0x04, 0, 4);
+        uint32_t id;
+        {
+            ISpiLock::Guard bus; // readCommand is a real panel transaction
+            id = p->readCommand(0x04, 0, 4);
+        }
         ILOG_DEBUG("Panel id=0x%08x (%dx%d): rst:%d, busy:%d, offX:%d, offY:%d invert:%d, RGB:%d, rotation:%d, offR:%d, read:%d, "
                    "readP:%d, readB:%d, dlen:%d, colordepth:%d",
                    id, p->width(), p->height(), cfg.pin_rst, cfg.pin_busy, cfg.offset_x, cfg.offset_y, p->getInvert(),

include/util/ISpiLock.h

@@ -0,0 +1,71 @@
+#pragma once
+
+/**
+ * @brief Host-provided mutual exclusion for an SPI bus shared with other peripherals.
+ *
+ * On boards where the display and/or SD card sit on the same SPI bus as something the
+ * host drives itself - a LoRa radio, say - the host owns the arbitration. Installing an
+ * implementation here lets the UI take that lock around its own bus traffic, so the host
+ * does not have to serialize whole UI cycles to stay safe.
+ *
+ * Hold time is what matters: the UI takes the lock only around actual transfers, never
+ * across rendering, so the bus stays free during the CPU-only majority of a redraw.
+ *
+ * @note Implementations MUST be reentrant. UI call sites nest (a tile draw takes the
+ * lock, and the SD read that feeds it takes it again), so a plain non-recursive mutex
+ * will self-deadlock. Wrap one if that is all the host has - see the firmware's
+ * tftSetup.cpp for a task-tracking example.
+ *
+ * When no implementation is installed the guards below are no-ops, which is correct for
+ * hosts that do not share the bus.
+ */
+class ISpiLock
+{
+  public:
+    virtual ~ISpiLock() = default;
+    virtual void lock(void) = 0;
+    virtual void unlock(void) = 0;
+
+    /**
+     * Let a waiter in without giving up the enclosing critical section.
+     *
+     * Provided for an async-DMA flush: once the display driver can signal completion
+     * asynchronously, the UI can release the bus while the transfer runs. Nothing calls
+     * it yet, so the default - release and immediately re-take - is sufficient.
+     */
+    virtual void yield(void)
+    {
+        unlock();
+        lock();
+    }
+
+    /**
+     * Normally set through DeviceScreen::create(). Kept as a static holder because two
+     * classes of call site cannot reach an instance member: the LVGL flush and touch
+     * callbacks are static and have no `this`, and the SD layer (`sdCard`, `SDFs`) is
+     * file-scope globals with no route to a DeviceGUI.
+     */
+    static void install(ISpiLock *lock);
+    static ISpiLock *installed(void);
+
+    /// RAII: hold the bus for the enclosing scope. Safe when nothing is installed.
+    class Guard
+    {
+      public:
+        Guard(void) : held(ISpiLock::installed())
+        {
+            if (held)
+                held->lock();
+        }
+        ~Guard()
+        {
+            if (held)
+                held->unlock();
+        }
+        Guard(const Guard &) = delete;
+        Guard &operator=(const Guard &) = delete;
+
+      private:
+        ISpiLock *held;
+    };
+};

source/graphics/DeviceScreen.cpp

@@ -15,14 +15,23 @@ DeviceScreen &DeviceScreen::create(void)
     return *new DeviceScreen(nullptr);
 }
 
-DeviceScreen &DeviceScreen::create(const DisplayDriverConfig *cfg)
+DeviceScreen &DeviceScreen::create(ISpiLock &spiLock)
+{
+    // Installed before anything is constructed, so even panel init is guarded.
+    ISpiLock::install(&spiLock);
+    return *new DeviceScreen(nullptr);
+}
+
+DeviceScreen &DeviceScreen::create(const DisplayDriverConfig *cfg, ISpiLock *spiLock)
 {
     ILOG_DEBUG("creating DeviceScreen %dx%d ...", cfg ? cfg->width() : 0, cfg ? cfg->height() : 0);
+    ISpiLock::install(spiLock);
     return *new DeviceScreen(cfg);
 }
 
-DeviceScreen &DeviceScreen::create(DisplayDriverConfig &&cfg)
+DeviceScreen &DeviceScreen::create(DisplayDriverConfig &&cfg, ISpiLock *spiLock)
 {
+    ISpiLock::install(spiLock);
     return *new DeviceScreen(std::move(cfg));
 }
 

source/graphics/TFT/TFTView_320x240.cpp

@@ -21,6 +21,7 @@
 #include "ui.h"
 #include "util/FileLoader.h"
 #include "util/ILog.h"
+#include "util/ISpiLock.h"
 #include <algorithm>
 #include <cmath>
 #include <cstdio>
@@ -6160,26 +6161,36 @@ void TFTView_320x240::backup(uint32_t option)
 
     std::stringstream path;
     path << "/keys/" << std::hex << std::setw(8) << std::setfill('0') << ownNode << ".yml";
+
+    // The bus is held for the card access only - messageAlert() below is LVGL work.
+    bool written = false;
+    {
+        ISpiLock::Guard bus;
 #if defined(ARCH_PORTDUINO) || defined(HAS_SD_MMC)
-    SDFs.mkdir("/keys");
-    File sd = SDFs.open(path.str().c_str(), FILE_WRITE);
+        SDFs.mkdir("/keys");
+        File sd = SDFs.open(path.str().c_str(), FILE_WRITE);
 #else
-    SDFs.mkdir("/keys");
-    FsFile sd = SDFs.open(path.str().c_str(), O_RDWR | O_CREAT);
+        SDFs.mkdir("/keys");
+        FsFile sd = SDFs.open(path.str().c_str(), O_RDWR | O_CREAT);
 #endif
-    if (sd) {
-        sd.println("config:");
-        sd.println("  security:");
-        sd.print("      privateKey: base64:");
-        sd.println(pskToBase64(privkey.bytes, privkey.size).c_str());
-        sd.print("      publicKey: base64:");
-        sd.println(pskToBase64(pubkey.bytes, pubkey.size).c_str());
+        if (sd) {
+            sd.println("config:");
+            sd.println("  security:");
+            sd.print("      privateKey: base64:");
+            sd.println(pskToBase64(privkey.bytes, privkey.size).c_str());
+            sd.print("      publicKey: base64:");
+            sd.println(pskToBase64(pubkey.bytes, pubkey.size).c_str());
+            written = true;
+        }
+        sd.close();
+    }
+
+    if (written) {
         ILOG_INFO("backup pub/priv keys done.");
     } else {
         ILOG_ERROR("open file %s for backup failed", path.str().c_str());
         messageAlert(_("Failed to write keys!"), true);
     }
-    sd.close();
 #endif
 }
 
@@ -6192,39 +6203,47 @@ void TFTView_320x240::restore(uint32_t option)
     std::stringstream path;
     path << "/keys/" << std::hex << std::setw(8) << std::setfill('0') << ownNode << ".yml";
 
+    // Read the file out under the bus guard, then release it: sendConfig() goes to the
+    // radio - which needs this same bus from another task - and messageAlert() is LVGL.
+    bool opened = false;
+    String privKey, pubKey;
+    {
+        ISpiLock::Guard bus;
 #if defined(ARCH_PORTDUINO) || defined(HAS_SD_MMC)
-    File sd = SDFs.open(path.str().c_str(), FILE_READ);
+        File sd = SDFs.open(path.str().c_str(), FILE_READ);
 #else
-    FsFile sd = SDFs.open(path.str().c_str(), O_RDONLY);
+        FsFile sd = SDFs.open(path.str().c_str(), O_RDONLY);
 #endif
-    if (sd) {
-        // TODO: improve parsing file contents
-        sd.readStringUntil('\n');                  // config:
-        sd.readStringUntil('\n');                  // security:
-        String privKey = sd.readStringUntil('\n'); // privateKey: base64:
-        String pubKey = sd.readStringUntil('\n');  // publicKey: base64:
-        if (privKey.indexOf("privateKey:") > 0 && pubKey.indexOf("publicKey:") > 0) {
-            String b64priv = privKey.substring(privKey.lastIndexOf(":") + 1);
-            String b64pub = pubKey.substring(pubKey.lastIndexOf(":") + 1);
-            b64priv.trim();
-            b64pub.trim();
-            if (base64ToPsk(b64priv.c_str(), privkey.bytes, privkey.size) &&
-                base64ToPsk(b64pub.c_str(), pubkey.bytes, pubkey.size) &&
-                controller->sendConfig(meshtastic_Config_SecurityConfig{db.config.security})) {
-                ILOG_INFO("restore pub/priv keys sent to radio");
-            } else {
-                ILOG_ERROR("decoding keys failed");
-                messageAlert(_("Failed to restore keys!"), true);
-            }
-        } else {
-            ILOG_ERROR("file %s contents don't match backup", path.str().c_str());
-            messageAlert(_("Failed to parse keys!"), true);
+        if (sd) {
+            opened = true;
+            // TODO: improve parsing file contents
+            sd.readStringUntil('\n');           // config:
+            sd.readStringUntil('\n');           // security:
+            privKey = sd.readStringUntil('\n'); // privateKey: base64:
+            pubKey = sd.readStringUntil('\n');  // publicKey: base64:
         }
-    } else {
+        sd.close();
+    }
+
+    if (!opened) {
         ILOG_ERROR("open file %s failed", path.str().c_str());
         messageAlert(_("Failed to retrieve keys!"), true);
+    } else if (privKey.indexOf("privateKey:") > 0 && pubKey.indexOf("publicKey:") > 0) {
+        String b64priv = privKey.substring(privKey.lastIndexOf(":") + 1);
+        String b64pub = pubKey.substring(pubKey.lastIndexOf(":") + 1);
+        b64priv.trim();
+        b64pub.trim();
+        if (base64ToPsk(b64priv.c_str(), privkey.bytes, privkey.size) && base64ToPsk(b64pub.c_str(), pubkey.bytes, pubkey.size) &&
+            controller->sendConfig(meshtastic_Config_SecurityConfig{db.config.security})) {
+            ILOG_INFO("restore pub/priv keys sent to radio");
+        } else {
+            ILOG_ERROR("decoding keys failed");
+            messageAlert(_("Failed to restore keys!"), true);
+        }
+    } else {
+        ILOG_ERROR("file %s contents don't match backup", path.str().c_str());
+        messageAlert(_("Failed to parse keys!"), true);
     }
-    sd.close();
 #endif
 }