Make Safe File actually safe

src/MessageStore.cpp

@@ -291,23 +291,16 @@ static inline bool readMessageRecord(File &f, StoredMessage &m)
 void MessageStore::saveToFlash()
 {
 #ifdef FSCom
-    // Ensure root exists
-    spiLock->lock();
-    FSCom.mkdir("/");
-    spiLock->unlock();
-
     SafeFile f(filename.c_str(), false);
 
-    spiLock->lock();
     uint8_t count = static_cast<uint8_t>(liveMessages.size());
     if (count > MAX_MESSAGES_SAVED)
         count = MAX_MESSAGES_SAVED;
-    f.write(&count, 1);
+    f.write(&count, 1); // SafeFile::write takes spiLock internally
 
     for (uint8_t i = 0; i < count; ++i) {
         writeMessageRecord(f, liveMessages[i]);
     }
-    spiLock->unlock();
 
     f.close();
 #endif
@@ -367,12 +360,7 @@ void MessageStore::clearAllMessages()
     SafeFile f(filename.c_str(), false);
     uint8_t count = 0;
 
-    // SafeFile already does its own spiLock in its constructor and close().
-    // Avoid nesting spiLocks, as this will hang until watchdog reset!
-    {
-        concurrency::LockGuard guard(spiLock);
-        f.write(&count, 1); // write "0 messages"
-    }
+    f.write(&count, 1); // write "0 messages"; SafeFile::write takes spiLock internally
 
     f.close();
 #endif

src/SafeFile.cpp

@@ -5,15 +5,23 @@
 // Only way to work on both esp32 and nrf52
 static File openFile(const char *filename, bool fullAtomic)
 {
+    std::string filenameTmp = filename;
+    filenameTmp += ".tmp";
+
     concurrency::LockGuard g(spiLock);
+
+    // Create the containing directory if it doesn't already exist
+    size_t slashIndex = filenameTmp.find_last_of('/');
+    if (slashIndex != std::string::npos && slashIndex > 0) {
+        std::string dir = filenameTmp.substr(0, slashIndex);
+        FSCom.mkdir(dir.c_str());
+    }
+
     LOG_DEBUG("Opening %s, fullAtomic=%d", filename, fullAtomic);
     if (!fullAtomic) {
         FSCom.remove(filename); // Nuke the old file to make space (ignore if it !exists)
     }
 
-    String filenameTmp = filename;
-    filenameTmp += ".tmp";
-
     // FIXME: If we are doing a full atomic write, we may need to remove the old tmp file now
     // if (fullAtomic) {
     //     FSCom.remove(filename);
@@ -33,6 +41,7 @@ size_t SafeFile::write(uint8_t ch)
     if (!f)
         return 0;
 
+    concurrency::LockGuard g(spiLock);
     hash ^= ch;
     return f.write(ch);
 }
@@ -42,6 +51,7 @@ size_t SafeFile::write(const uint8_t *buffer, size_t size)
     if (!f)
         return 0;
 
+    concurrency::LockGuard g(spiLock);
     for (size_t i = 0; i < size; i++) {
         hash ^= buffer[i];
     }

src/gps/GPS.cpp

@@ -666,17 +666,13 @@ bool GPS::saveProbeCache() const
         return false;
     }
 
-    spiLock->lock();
-    FSCom.mkdir("/prefs");
-    spiLock->unlock();
     GPSProbeCacheRecord record = {
         GPS_PROBE_CACHE_MAGIC, GPS_PROBE_CACHE_VERSION, 0, static_cast<uint32_t>(detectedBaud), static_cast<uint8_t>(gnssModel),
     };
 
     auto file = SafeFile(GPS_PROBE_CACHE_FILE, true);
-    spiLock->lock();
+    // SafeFile::write takes spiLock internally; don't hold it here (non-recursive lock).
     const size_t written = file.write(reinterpret_cast<const uint8_t *>(&record), sizeof(record));
-    spiLock->unlock();
     return (written == sizeof(record)) && file.close();
 #else
     return false;

src/graphics/niche/Utils/CannedMessageStore.cpp

@@ -121,14 +121,7 @@ void CannedMessageStore::handleSet(const meshtastic_AdminMessage *request)
     strncpy(cannedMessageModuleConfig.messages, request->set_canned_message_module_messages,
             sizeof(cannedMessageModuleConfig.messages));
 
-    // Ensure the directory exists
-#ifdef FSCom
-    spiLock->lock();
-    FSCom.mkdir("/prefs");
-    spiLock->unlock();
-#endif
-
-    // Write to flash
+    // Write to flash ("/prefs" is created by SafeFile (openFile) inside saveProto)
     nodeDB->saveProto(cannedMessagesConfigFile, meshtastic_CannedMessageModuleConfig_size,
                       &meshtastic_CannedMessageModuleConfig_msg, &cannedMessageModuleConfig);
 

src/graphics/niche/Utils/FlashData.h

@@ -107,30 +107,25 @@ template <typename T> class FlashData
         return okay;
     }
 
-    // Save module's custom data (settings?) to flash. Doesn't use protobufs
-    // Takes the firmware's SPI lock, in case the files are stored on SD card
-    // Need to lock and unlock around specific FS methods, as the SafeFile class takes the lock for itself internally.
+    // Save module's custom data (settings?) to flash. Doesn't use protobufs.
+    // SafeFile (constructor/write/close) takes the firmware's SPI lock for itself internally
+    // (relevant when files are on an SD card), so we must not hold it across those calls.
     static void save(T *data, const char *label)
     {
         // Get a filename based on the label
         std::string filename = getFilename(label);
 
 #ifdef FSCom
-        spiLock->lock();
-        FSCom.mkdir("/NicheGraphics");
-        spiLock->unlock();
-
+        // "/NicheGraphics" is created by SafeFile (openFile) below.
         auto f = SafeFile(filename.c_str(), true); // "true": full atomic. Write new data to temp file, then rename.
 
         LOG_INFO("Saving %s", filename.c_str());
 
         // Calculate a hash of the data
         uint32_t hash = getHash(data);
 
-        spiLock->lock();
-        f.write((uint8_t *)data, sizeof(T));     // Write the actual data
+        f.write((uint8_t *)data, sizeof(T));     // Write the actual data (SafeFile::write takes spiLock internally)
         f.write((uint8_t *)&hash, sizeof(hash)); // Append the hash
-        spiLock->unlock();
 
         bool writeSucceeded = f.close();
 

src/mesh/NodeDB.cpp

@@ -2787,12 +2787,7 @@ bool NodeDB::saveChannelsToDisk()
         return false;
     }
 
-#ifdef FSCom
-    spiLock->lock();
-    FSCom.mkdir("/prefs");
-    spiLock->unlock();
-#endif
-
+    // Directory is created by SafeFile (openFile) inside saveProto.
     return saveProto(channelFileName, meshtastic_ChannelFile_size, &meshtastic_ChannelFile_msg, &channelFile);
 }
 
@@ -2806,11 +2801,6 @@ bool NodeDB::saveDeviceStateToDisk()
         return false;
     }
 
-#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
     return saveProto(deviceStateFileName, meshtastic_DeviceState_size, &meshtastic_DeviceState_msg, &devicestate, true);
@@ -2843,12 +2833,6 @@ bool NodeDB::saveNodeDatabaseToDisk()
     }
 #endif
 
-#ifdef FSCom
-    spiLock->lock();
-    FSCom.mkdir("/prefs");
-    spiLock->unlock();
-#endif
-
     // Project the maps into the on-disk vectors just before encoding; cleared
     // again on the way out so we don't carry duplicate state.
     concurrency::LockGuard guard(&satelliteMutex);
@@ -2960,11 +2944,6 @@ bool NodeDB::saveToDiskNoRetry(int saveWhat)
 #endif
 
     bool success = true;
-#ifdef FSCom
-    spiLock->lock();
-    FSCom.mkdir("/prefs");
-    spiLock->unlock();
-#endif
 
     if (saveWhat & SEGMENT_CONFIG) {
         config.has_device = true;
@@ -3980,9 +3959,7 @@ bool NodeDB::backupPreferences(meshtastic_AdminMessage_BackupLocation location)
         size_t backupSize;
         pb_get_encoded_size(&backupSize, meshtastic_BackupPreferences_fields, &backup);
 
-        spiLock->lock();
-        FSCom.mkdir("/backups");
-        spiLock->unlock();
+        // "/backups" is created by SafeFile (openFile) inside saveProto.
         success = saveProto(backupFileName, backupSize, &meshtastic_BackupPreferences_msg, &backup);
 
         if (success) {

src/mesh/WarmNodeStore.cpp

@@ -592,21 +592,12 @@ bool WarmNodeStore::save()
     h.entrySize = sizeof(WarmNodeEntry);
     h.crc = crc32Buffer(packed.data(), h.count * sizeof(WarmNodeEntry));
 
-    // SafeFile already does its own spiLock in its constructor and close().
-    // Avoid nesting spiLocks, as this will hang until watchdog reset!
-
-    {
-        concurrency::LockGuard g(spiLock);
-        FSCom.mkdir("/prefs");
-    }
-
+    // SafeFile takes spiLock itself in its constructor, write() and close();
+    // we must not hold it across those or the non-recursive lock deadlocks (watchdog reset).
     auto f = SafeFile(warmFileName, false);
 
-    {
-        concurrency::LockGuard g(spiLock);
-        f.write((const uint8_t *)&h, sizeof(h));
-        f.write((const uint8_t *)packed.data(), h.count * sizeof(WarmNodeEntry));
-    }
+    f.write((const uint8_t *)&h, sizeof(h));
+    f.write((const uint8_t *)packed.data(), h.count * sizeof(WarmNodeEntry));
 
     bool ok = f.close();
     if (!ok)

src/mesh/mesh-pb-constants.cpp

@@ -56,12 +56,11 @@ bool readcb(pb_istream_t *stream, uint8_t *buf, size_t count)
 /// Write to an arduino file
 bool writecb(pb_ostream_t *stream, const uint8_t *buf, size_t count)
 {
-    spiLock->lock();
+    // Locking is delegated to the Print's write() (SafeFile::write takes spiLock itself).
+    // All current callers pass a SafeFile; a non-SafeFile Print would not be bus-locked here.
     auto file = (Print *)stream->state;
     // LOG_DEBUG("writing %d bytes to protobuf file", count);
-    bool status = file->write(buf, count) == count;
-    spiLock->unlock();
-    return status;
+    return file->write(buf, count) == count;
 }
 #endif
 

src/modules/CannedMessageModule.cpp

@@ -2292,12 +2292,7 @@ bool CannedMessageModule::saveProtoForModule()
 {
     bool okay = true;
 
-#ifdef FSCom
-    spiLock->lock();
-    FSCom.mkdir("/prefs");
-    spiLock->unlock();
-#endif
-
+    // "/prefs" is created by SafeFile (openFile) inside saveProto.
     okay &= nodeDB->saveProto(cannedMessagesConfigFile, meshtastic_CannedMessageModuleConfig_size,
                               &meshtastic_CannedMessageModuleConfig_msg, &cannedMessageModuleConfig);
 

src/modules/HopScalingModule.cpp

@@ -80,7 +80,6 @@ void HopScalingModule::clear()
 void HopScalingModule::saveToDisk() const
 {
 #ifdef FSCom
-    FSCom.mkdir("/prefs");
     PersistedHistogram state{};
     state.magic = HISTOGRAM_STATE_MAGIC;
     state.version = HISTOGRAM_STATE_VERSION;

src/motion/MotionSensor.cpp

@@ -79,7 +79,7 @@ bool MotionSensor::saveMagnetometerCalibration(const char *filePath, float highe
         return false;
     }
 
-    FSCom.mkdir("/prefs");
+    // "/prefs" is created by SafeFile (openFile) below.
     CompassCalibrationRecord record = {
         COMPASS_CALIBRATION_MAGIC, COMPASS_CALIBRATION_VERSION, 0, highestX, lowestX, highestY, lowestY, highestZ, lowestZ};