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) {