add more locking for shared SPI devices

platformio.ini

@@ -123,7 +123,8 @@ lib_deps =
 
 [radiolib_base]
 lib_deps =
-	https://github.com/jgromes/RadioLib.git#b2b4c9e0f73bd9d5a435b291ee27574ee9c2f69f
+	; jgromes/[email protected]
+	https://github.com/jgromes/RadioLib.git#92b687821ff4e6c358d866f84566f66672ab02b8
 
 ; Common libs for environmental measurements in telemetry module
 ; (not included in native / portduino)

src/FSCommon.cpp

@@ -9,6 +9,7 @@
  *
  */
 #include "FSCommon.h"
+#include "SPILock.h"
 #include "configuration.h"
 
 #ifdef HAS_SDCARD
@@ -102,6 +103,8 @@ bool copyFile(const char *from, const char *to)
     return true;
 
 #elif defined(FSCom)
+    // take SPI Lock
+    concurrency::LockGuard g(spiLock);
     unsigned char cbuffer[16];
 
     File f1 = FSCom.open(from, FILE_O_READ);
@@ -145,16 +148,23 @@ bool renameFile(const char *pathFrom, const char *pathTo)
         return false;
     }
 #elif defined(FSCom)
+
 #ifdef ARCH_ESP32
+    // take SPI Lock
+    spiLock->lock();
     // rename was fixed for ESP32 IDF LittleFS in April
-    return FSCom.rename(pathFrom, pathTo);
+    bool result = FSCom.rename(pathFrom, pathTo);
+    spiLock->unlock();
+    return result;
 #else
+    // copyFile does its own locking.
     if (copyFile(pathFrom, pathTo) && FSCom.remove(pathFrom)) {
         return true;
     } else {
         return false;
     }
 #endif
+
 #endif
 }
 
@@ -164,6 +174,7 @@ bool renameFile(const char *pathFrom, const char *pathTo)
  * @brief Get the list of files in a directory.
  *
  * This function returns a list of files in a directory. The list includes the full path of each file.
+ * We can't use SPILOCK here because of recursion. Callers of this function should use SPILOCK.
  *
  * @param dirname The name of the directory.
  * @param levels The number of levels of subdirectories to list.
@@ -212,6 +223,7 @@ std::vector<meshtastic_FileInfo> getFiles(const char *dirname, uint8_t levels)
 
 /**
  * Lists the contents of a directory.
+ * We can't use SPILOCK here because of recursion. Callers of this function should use SPILOCK.
  *
  * @param dirname The name of the directory to list.
  * @param levels The number of levels of subdirectories to list.
@@ -325,18 +337,21 @@ void listDir(const char *dirname, uint8_t levels, bool del)
 void rmDir(const char *dirname)
 {
 #ifdef FSCom
+
 #if (defined(ARCH_ESP32) || defined(ARCH_RP2040) || defined(ARCH_PORTDUINO))
     listDir(dirname, 10, true);
 #elif defined(ARCH_NRF52)
     // nRF52 implementation of LittleFS has a recursive delete function
     FSCom.rmdir_r(dirname);
 #endif
+
 #endif
 }
 
 void fsInit()
 {
 #ifdef FSCom
+    spiLock->lock();
     if (!FSBegin()) {
         LOG_ERROR("Filesystem mount failed");
         // assert(0); This auto-formats the partition, so no need to fail here.
@@ -347,6 +362,7 @@ void fsInit()
     LOG_DEBUG("Filesystem files:");
 #endif
     listDir("/", 10);
+    spiLock->unlock();
 #endif
 }
 
@@ -356,6 +372,7 @@ void fsInit()
 void setupSDCard()
 {
 #ifdef HAS_SDCARD
+    concurrency::LockGuard g(spiLock);
     SDHandler.begin(SPI_SCK, SPI_MISO, SPI_MOSI);
 
     if (!SD.begin(SDCARD_CS, SDHandler)) {
@@ -383,4 +400,4 @@ void setupSDCard()
     LOG_DEBUG("Total space: %lu MB", (uint32_t)(SD.totalBytes() / (1024 * 1024)));
     LOG_DEBUG("Used space: %lu MB", (uint32_t)(SD.usedBytes() / (1024 * 1024)));
 #endif
-}
+}

src/SafeFile.cpp

@@ -5,6 +5,7 @@
 // Only way to work on both esp32 and nrf52
 static File openFile(const char *filename, bool fullAtomic)
 {
+    concurrency::LockGuard g(spiLock);
     if (!fullAtomic)
         FSCom.remove(filename); // Nuke the old file to make space (ignore if it !exists)
 
@@ -53,14 +54,19 @@ bool SafeFile::close()
     if (!f)
         return false;
 
+    spiLock->lock();
     f.close();
+    spiLock->unlock();
     if (!testReadback())
         return false;
 
-    // brief window of risk here ;-)
-    if (fullAtomic && FSCom.exists(filename.c_str()) && !FSCom.remove(filename.c_str())) {
-        LOG_ERROR("Can't remove old pref file");
-        return false;
+    { // Scope for lock
+        concurrency::LockGuard g(spiLock);
+        // brief window of risk here ;-)
+        if (fullAtomic && FSCom.exists(filename.c_str()) && !FSCom.remove(filename.c_str())) {
+            LOG_ERROR("Can't remove old pref file");
+            return false;
+        }
     }
 
     String filenameTmp = filename;
@@ -76,6 +82,7 @@ bool SafeFile::close()
 /// Read our (closed) tempfile back in and compare the hash
 bool SafeFile::testReadback()
 {
+    concurrency::LockGuard g(spiLock);
     bool lfs_failed = lfs_assert_failed;
     lfs_assert_failed = false;
 

src/SafeFile.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "FSCommon.h"
+#include "SPILock.h"
 #include "configuration.h"
 
 #ifdef FSCom

src/main.cpp

@@ -376,6 +376,8 @@ void setup()
 #endif
 #endif
 
+    initSPI();
+
     OSThread::setup();
 
     ledPeriodic = new Periodic("Blink", ledBlinker);
@@ -652,8 +654,6 @@ void setup()
     rp2040Setup();
 #endif
 
-    initSPI(); // needed here before reading from littleFS
-
     // We do this as early as possible because this loads preferences from flash
     // but we need to do this after main cpu init (esp32setup), because we need the random seed set
     nodeDB = new NodeDB;

src/mesh/NodeDB.cpp

@@ -13,6 +13,7 @@
 #include "PowerFSM.h"
 #include "RTC.h"
 #include "Router.h"
+#include "SPILock.h"
 #include "SafeFile.h"
 #include "TypeConversions.h"
 #include "error.h"
@@ -423,12 +424,15 @@ bool NodeDB::factoryReset(bool eraseBleBonds)
 {
     LOG_INFO("Perform factory reset!");
     // first, remove the "/prefs" (this removes most prefs)
-    rmDir("/prefs");
+    spiLock->lock();
+    rmDir("/prefs"); // this uses spilock internally...
+
 #ifdef FSCom
     if (FSCom.exists("/static/rangetest.csv") && !FSCom.remove("/static/rangetest.csv")) {
         LOG_ERROR("Could not remove rangetest.csv file");
     }
 #endif
+    spiLock->unlock();
     // second, install default state (this will deal with the duplicate mac address issue)
     installDefaultDeviceState();
     installDefaultConfig(!eraseBleBonds); // Also preserve the private key if we're not erasing BLE bonds
@@ -913,6 +917,7 @@ LoadFileResult NodeDB::loadProto(const char *filename, size_t protoSize, size_t
 {
     LoadFileResult state = LoadFileResult::OTHER_FAILURE;
 #ifdef FSCom
+    concurrency::LockGuard g(spiLock);
 
     auto f = FSCom.open(filename, FILE_O_READ);
 
@@ -946,8 +951,10 @@ void NodeDB::loadFromDisk()
     // disk we will still factoryReset to restore things.
 
 #ifdef ARCH_ESP32
+    spiLock->lock();
     if (FSCom.exists("/static/static"))
         rmDir("/static/static"); // Remove bad static web files bundle from initial 2.5.13 release
+    spiLock->unlock();
 #endif
 
     // static DeviceState scratch; We no longer read into a tempbuf because this structure is 15KB of valuable RAM
@@ -1094,9 +1101,6 @@ void NodeDB::loadFromDisk()
 bool NodeDB::saveProto(const char *filename, size_t protoSize, const pb_msgdesc_t *fields, const void *dest_struct,
                        bool fullAtomic)
 {
-#ifdef ARCH_ESP32
-    concurrency::LockGuard g(spiLock);
-#endif
     bool okay = false;
 #ifdef FSCom
     auto f = SafeFile(filename, fullAtomic);
@@ -1124,15 +1128,19 @@ bool NodeDB::saveProto(const char *filename, size_t protoSize, const pb_msgdesc_
 bool NodeDB::saveChannelsToDisk()
 {
 #ifdef FSCom
+    spiLock->lock();
     FSCom.mkdir("/prefs");
+    spiLock->unlock();
 #endif
     return saveProto(channelFileName, meshtastic_ChannelFile_size, &meshtastic_ChannelFile_msg, &channelFile);
 }
 
 bool NodeDB::saveDeviceStateToDisk()
 {
 #ifdef FSCom
+    spiLock->lock();
     FSCom.mkdir("/prefs");
+    spiLock->unlock();
 #endif
     // Note: if MAX_NUM_NODES=100 and meshtastic_NodeInfoLite_size=166, so will be approximately 17KB
     // Because so huge we _must_ not use fullAtomic, because the filesystem is probably too small to hold two copies of this
@@ -1145,7 +1153,9 @@ bool NodeDB::saveToDiskNoRetry(int saveWhat)
     bool success = true;
 
 #ifdef FSCom
+    spiLock->lock();
     FSCom.mkdir("/prefs");
+    spiLock->unlock();
 #endif
     if (saveWhat & SEGMENT_CONFIG) {
         config.has_device = true;
@@ -1196,7 +1206,9 @@ bool NodeDB::saveToDisk(int saveWhat)
     if (!success) {
         LOG_ERROR("Failed to save to disk, retrying");
 #ifdef ARCH_NRF52 // @geeksville is not ready yet to say we should do this on other platforms.  See bug #4184 discussion
+        spiLock->lock();
         FSCom.format();
+        spiLock->unlock();
 
 #endif
         success = saveToDiskNoRetry(saveWhat);
@@ -1515,4 +1527,4 @@ void recordCriticalError(meshtastic_CriticalErrorCode code, uint32_t address, co
     LOG_ERROR("A critical failure occurred, portduino is exiting");
     exit(2);
 #endif
-}
+}

src/mesh/PhoneAPI.cpp

@@ -12,6 +12,7 @@
 #include "PhoneAPI.h"
 #include "PowerFSM.h"
 #include "RadioInterface.h"
+#include "SPILock.h"
 #include "TypeConversions.h"
 #include "main.h"
 #include "xmodem.h"
@@ -54,7 +55,9 @@ void PhoneAPI::handleStartConfig()
     // even if we were already connected - restart our state machine
     state = STATE_SEND_MY_INFO;
     pauseBluetoothLogging = true;
+    spiLock->lock();
     filesManifest = getFiles("/", 10);
+    spiLock->unlock();
     LOG_DEBUG("Got %d files in manifest", filesManifest.size());
 
     LOG_INFO("Start API client config");

src/mesh/http/ContentHandler.cpp

@@ -10,6 +10,7 @@
 #include "mesh/wifi/WiFiAPClient.h"
 #endif
 #include "Led.h"
+#include "SPILock.h"
 #include "power.h"
 #include "serialization/JSON.h"
 #include <FSCommon.h>
@@ -236,6 +237,7 @@ void handleAPIv1ToRadio(HTTPRequest *req, HTTPResponse *res)
 
 void htmlDeleteDir(const char *dirname)
 {
+
     File root = FSCom.open(dirname);
     if (!root) {
         return;
@@ -318,6 +320,7 @@ void handleFsBrowseStatic(HTTPRequest *req, HTTPResponse *res)
     res->setHeader("Access-Control-Allow-Origin", "*");
     res->setHeader("Access-Control-Allow-Methods", "GET");
 
+    concurrency::LockGuard g(spiLock);
     auto fileList = htmlListDir("/static", 10);
 
     // create json output structure
@@ -349,9 +352,12 @@ void handleFsDeleteStatic(HTTPRequest *req, HTTPResponse *res)
     res->setHeader("Content-Type", "application/json");
     res->setHeader("Access-Control-Allow-Origin", "*");
     res->setHeader("Access-Control-Allow-Methods", "DELETE");
+
     if (params->getQueryParameter("delete", paramValDelete)) {
         std::string pathDelete = "/" + paramValDelete;
+        concurrency::LockGuard g(spiLock);
         if (FSCom.remove(pathDelete.c_str())) {
+
             LOG_INFO("%s", pathDelete.c_str());
             JSONObject jsonObjOuter;
             jsonObjOuter["status"] = new JSONValue("ok");
@@ -360,6 +366,7 @@ void handleFsDeleteStatic(HTTPRequest *req, HTTPResponse *res)
             delete value;
             return;
         } else {
+
             LOG_INFO("%s", pathDelete.c_str());
             JSONObject jsonObjOuter;
             jsonObjOuter["status"] = new JSONValue("Error");
@@ -393,6 +400,8 @@ void handleStatic(HTTPRequest *req, HTTPResponse *res)
             filenameGzip = "/static/index.html.gz";
         }
 
+        concurrency::LockGuard g(spiLock);
+
         if (FSCom.exists(filename.c_str())) {
             file = FSCom.open(filename.c_str());
             if (!file.available()) {
@@ -410,6 +419,7 @@ void handleStatic(HTTPRequest *req, HTTPResponse *res)
             file = FSCom.open(filenameGzip.c_str());
             res->setHeader("Content-Type", "text/html");
             if (!file.available()) {
+
                 LOG_WARN("File not available - %s", filenameGzip.c_str());
                 res->println("Web server is running.<br><br>The content you are looking for can't be found. Please see: <a "
                              "href=https://meshtastic.org/docs/software/web-client/>FAQ</a>.<br><br><a "
@@ -535,6 +545,7 @@ void handleFormUpload(HTTPRequest *req, HTTPResponse *res)
         // concepts of the body parser functionality easier to understand.
         std::string pathname = "/static/" + filename;
 
+        concurrency::LockGuard g(spiLock);
         // Create a new file to stream the data into
         File file = FSCom.open(pathname.c_str(), FILE_O_WRITE);
         size_t fileLength = 0;
@@ -571,6 +582,7 @@ void handleFormUpload(HTTPRequest *req, HTTPResponse *res)
 
         file.flush();
         file.close();
+
         res->printf("<p>Saved %d bytes to %s</p>", (int)fileLength, pathname.c_str());
     }
     if (!didwrite) {
@@ -642,9 +654,11 @@ void handleReport(HTTPRequest *req, HTTPResponse *res)
     jsonObjMemory["heap_free"] = new JSONValue((int)memGet.getFreeHeap());
     jsonObjMemory["psram_total"] = new JSONValue((int)memGet.getPsramSize());
     jsonObjMemory["psram_free"] = new JSONValue((int)memGet.getFreePsram());
+    spiLock->lock();
     jsonObjMemory["fs_total"] = new JSONValue((int)FSCom.totalBytes());
     jsonObjMemory["fs_used"] = new JSONValue((int)FSCom.usedBytes());
     jsonObjMemory["fs_free"] = new JSONValue(int(FSCom.totalBytes() - FSCom.usedBytes()));
+    spiLock->unlock();
 
     // data->power
     JSONObject jsonObjPower;
@@ -786,6 +800,7 @@ void handleDeleteFsContent(HTTPRequest *req, HTTPResponse *res)
 
     LOG_INFO("Delete files from /static/* : ");
 
+    concurrency::LockGuard g(spiLock);
     htmlDeleteDir("/static");
 
     res->println("<p><hr><p><a href=/admin>Back to admin</a>");