From 88b10c82a3ce8f5477525aec3f5519084bc5652f Mon Sep 17 00:00:00 2001 From: Jeremy Klein Date: Sun, 24 May 2026 18:48:33 -0700 Subject: [PATCH] fix: serialize SdFat FsFile close through HalStorage mutex (#2135) SdFat's SdSpiCard tracks SPI bus state with an unsynchronized m_spiActive bool. When two tasks call into SdFat concurrently they can confuse that state machine, ending with one task calling SPIClass::endTransaction() against a paramLock the other task holds. That trips FreeRTOS's xTaskPriorityDisinherit assert (tasks.c:5156, pxTCB == pxCurrentTCBs[0]) and panics the system. HalStorage already serialized every explicit method call via storageMutex, but HalFile's destructor was `= default`, which let the underlying SdFat FsFile destructor run close() outside any lock (DESTRUCTOR_CLOSES_FILE=1). Any task that destructed a HalFile while another task was mid-SD-op would race the unsynchronized state. Move the locking discipline into HalFile::Impl::~Impl: an explicit close() under StorageLock, then the FsFile member destructor's redundant close() is a no-op. HalFile's special members can stay = default. Switch storageMutex to xSemaphoreCreateRecursiveMutex so openFileForRead and openFileForWrite can hold the lock while assigning to a HalFile& out-param whose prior Impl needs locked teardown. Priority inheritance still applies to recursive mutexes. Also documented the no-bypass rule in CLAUDE.md: never call SdFat / SdSpiCard / FsBaseFile / SDCardManager directly, never define HAL_STORAGE_IMPL outside HalStorage.cpp. Addresses my admittedly synthetic repro for #2047 Did you use AI tools to help write this code? partial --- .skills/SKILL.md | 6 ++++++ lib/hal/HalStorage.cpp | 26 +++++++++++++++++++------- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/.skills/SKILL.md b/.skills/SKILL.md index b7b59dc7..848ea462 100644 --- a/.skills/SKILL.md +++ b/.skills/SKILL.md @@ -161,6 +161,12 @@ if (Storage.openFileForRead("MODULE", "/path/to/file.bin", file)) { **Usage**: See example above. Uses `FsFile` (SdFat), NOT Arduino `File`. Do NOT add `file.close()` for local variables (see DESTRUCTOR_CLOSES_FILE above). +**SdFat is not thread-safe; all SD access MUST go through HalStorage**: +- SdFat's `SdSpiCard` tracks SPI bus state with an unsynchronized `m_spiActive` bool. Two tasks calling SdFat concurrently can confuse that state machine and end with one task calling `SPIClass::endTransaction()` against a paramLock the *other* task is holding. That trips FreeRTOS's `xTaskPriorityDisinherit` assert (`tasks.c:5156, pxTCB == pxCurrentTCBs[0]`) and panics the system. See SdFat issue #518. +- `HalStorage` serializes everything via `storageMutex`. Downstream code includes ``, which transparently `using FsFile = HalFile;`; every method call (read, write, seek, close) takes the mutex. `HalFile`'s destructor also takes the mutex before letting the underlying SdFat `FsFile` close. +- **Never** call into `SdFat` / `SdSpiCard` / `FsBaseFile` / `SDCardManager` directly. **Never** define `HAL_STORAGE_IMPL` outside `HalStorage.cpp`; that disables the `FsFile -> HalFile` typedef and you'll get a raw SdFat handle that bypasses the mutex. +- If you're storing a raw `FsFile` in a place that won't transitively include `` (rare), include the header explicitly so the typedef applies. + --- ## Coding Standards diff --git a/lib/hal/HalStorage.cpp b/lib/hal/HalStorage.cpp index 154e2c34..095e389c 100644 --- a/lib/hal/HalStorage.cpp +++ b/lib/hal/HalStorage.cpp @@ -12,7 +12,13 @@ HalStorage HalStorage::instance; HalStorage::HalStorage() { - storageMutex = xSemaphoreCreateMutex(); + // Recursive so the same task can re-enter StorageLock without self-deadlock. + // openFileForRead/Write take the lock and then assign to a HalFile& + // out-param; if that out-param already held an Impl, its destructor takes + // the lock again to close the prior FsFile under serialization (see + // HalFile::Impl::~Impl below). Priority inheritance still applies to + // recursive mutexes. + storageMutex = xSemaphoreCreateRecursiveMutex(); assert(storageMutex != nullptr); } @@ -26,8 +32,8 @@ bool HalStorage::ready() const { return SDCard.ready(); } class HalStorage::StorageLock { public: - StorageLock() { xSemaphoreTake(HalStorage::getInstance().storageMutex, portMAX_DELAY); } - ~StorageLock() { xSemaphoreGive(HalStorage::getInstance().storageMutex); } + StorageLock() { xSemaphoreTakeRecursive(HalStorage::getInstance().storageMutex, portMAX_DELAY); } + ~StorageLock() { xSemaphoreGiveRecursive(HalStorage::getInstance().storageMutex); } }; #define HAL_STORAGE_WRAPPED_CALL(method, ...) \ @@ -57,17 +63,23 @@ bool HalStorage::ensureDirectoryExists(const char* path) { HAL_STORAGE_WRAPPED_C class HalFile::Impl { public: Impl(FsFile&& fsFile) : file(std::move(fsFile)) {} + // SdFat is not thread-safe; FsFile::close() touches SD/SPI and must run + // under StorageLock or it races SdSpiCard::m_spiActive across tasks and + // trips FreeRTOS's xTaskPriorityDisinherit assert. The FsFile member + // destructor (DESTRUCTOR_CLOSES_FILE=1) will close() again after the lock + // releases, but close() on an already-closed FsFile is a no-op. See SdFat + // issue #518 and the HAL note in CLAUDE.md. + ~Impl() { + HalStorage::StorageLock lock; + file.close(); + } FsFile file; }; HalFile::HalFile() = default; - HalFile::HalFile(std::unique_ptr impl) : impl(std::move(impl)) {} - HalFile::~HalFile() = default; - HalFile::HalFile(HalFile&&) = default; - HalFile& HalFile::operator=(HalFile&&) = default; HalFile HalStorage::open(const char* path, const oflag_t oflag) {