Files
Crosspoint/lib/hal/HalStorage.cpp
T
Jeremy Klein 88b10c82a3 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
2026-05-24 21:48:33 -04:00

174 lines
7.4 KiB
C++

#define HAL_STORAGE_IMPL
#include "HalStorage.h"
#include <FS.h> // need to be included before SdFat.h for compatibility with FS.h's File class
#include <Logging.h>
#include <SDCardManager.h>
#include <cassert>
#define SDCard SDCardManager::getInstance()
HalStorage HalStorage::instance;
HalStorage::HalStorage() {
// 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);
}
// begin() and ready() are only called from setup, no need to acquire mutex for them
bool HalStorage::begin() { return SDCard.begin(); }
bool HalStorage::ready() const { return SDCard.ready(); }
// For the rest of the methods, we acquire the mutex to ensure thread safety
class HalStorage::StorageLock {
public:
StorageLock() { xSemaphoreTakeRecursive(HalStorage::getInstance().storageMutex, portMAX_DELAY); }
~StorageLock() { xSemaphoreGiveRecursive(HalStorage::getInstance().storageMutex); }
};
#define HAL_STORAGE_WRAPPED_CALL(method, ...) \
HalStorage::StorageLock lock; \
return SDCard.method(__VA_ARGS__);
std::vector<String> HalStorage::listFiles(const char* path, int maxFiles) {
HAL_STORAGE_WRAPPED_CALL(listFiles, path, maxFiles);
}
String HalStorage::readFile(const char* path) { HAL_STORAGE_WRAPPED_CALL(readFile, path); }
bool HalStorage::readFileToStream(const char* path, Print& out, size_t chunkSize) {
HAL_STORAGE_WRAPPED_CALL(readFileToStream, path, out, chunkSize);
}
size_t HalStorage::readFileToBuffer(const char* path, char* buffer, size_t bufferSize, size_t maxBytes) {
HAL_STORAGE_WRAPPED_CALL(readFileToBuffer, path, buffer, bufferSize, maxBytes);
}
bool HalStorage::writeFile(const char* path, const String& content) {
HAL_STORAGE_WRAPPED_CALL(writeFile, path, content);
}
bool HalStorage::ensureDirectoryExists(const char* path) { HAL_STORAGE_WRAPPED_CALL(ensureDirectoryExists, path); }
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) : 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) {
StorageLock lock; // ensure thread safety for the duration of this function
return HalFile(std::make_unique<HalFile::Impl>(SDCard.open(path, oflag)));
}
bool HalStorage::mkdir(const char* path, const bool pFlag) { HAL_STORAGE_WRAPPED_CALL(mkdir, path, pFlag); }
bool HalStorage::exists(const char* path) { HAL_STORAGE_WRAPPED_CALL(exists, path); }
bool HalStorage::remove(const char* path) { HAL_STORAGE_WRAPPED_CALL(remove, path); }
bool HalStorage::rename(const char* oldPath, const char* newPath) {
HAL_STORAGE_WRAPPED_CALL(rename, oldPath, newPath);
}
bool HalStorage::rmdir(const char* path) { HAL_STORAGE_WRAPPED_CALL(rmdir, path); }
bool HalStorage::openFileForRead(const char* moduleName, const char* path, HalFile& file) {
StorageLock lock; // ensure thread safety for the duration of this function
FsFile fsFile;
bool ok = SDCard.openFileForRead(moduleName, path, fsFile);
file = HalFile(std::make_unique<HalFile::Impl>(std::move(fsFile)));
return ok;
}
bool HalStorage::openFileForRead(const char* moduleName, const std::string& path, HalFile& file) {
return openFileForRead(moduleName, path.c_str(), file);
}
bool HalStorage::openFileForRead(const char* moduleName, const String& path, HalFile& file) {
return openFileForRead(moduleName, path.c_str(), file);
}
bool HalStorage::openFileForWrite(const char* moduleName, const char* path, HalFile& file) {
StorageLock lock; // ensure thread safety for the duration of this function
FsFile fsFile;
bool ok = SDCard.openFileForWrite(moduleName, path, fsFile);
file = HalFile(std::make_unique<HalFile::Impl>(std::move(fsFile)));
return ok;
}
bool HalStorage::openFileForWrite(const char* moduleName, const std::string& path, HalFile& file) {
return openFileForWrite(moduleName, path.c_str(), file);
}
bool HalStorage::openFileForWrite(const char* moduleName, const String& path, HalFile& file) {
return openFileForWrite(moduleName, path.c_str(), file);
}
bool HalStorage::removeDir(const char* path) { HAL_STORAGE_WRAPPED_CALL(removeDir, path); }
// HalFile implementation
// Allow doing file operations while ensuring thread safety via HalStorage's mutex.
// Please keep the list below in sync with the HalFile.h header
#define HAL_FILE_WRAPPED_CALL(method, ...) \
HalStorage::StorageLock lock; \
assert(impl != nullptr); \
return impl->file.method(__VA_ARGS__);
#define HAL_FILE_FORWARD_CALL(method, ...) \
assert(impl != nullptr); \
return impl->file.method(__VA_ARGS__);
void HalFile::flush() { HAL_FILE_WRAPPED_CALL(flush, ); }
size_t HalFile::getName(char* name, size_t len) { HAL_FILE_WRAPPED_CALL(getName, name, len); }
size_t HalFile::size() { HAL_FILE_FORWARD_CALL(size, ); } // already thread-safe, no need to wrap
size_t HalFile::fileSize() { HAL_FILE_FORWARD_CALL(fileSize, ); } // already thread-safe, no need to wrap
uint64_t HalFile::fileSize64() { HAL_FILE_FORWARD_CALL(fileSize, ); } // already thread-safe, no need to wrap
bool HalFile::seek(size_t pos) { HAL_FILE_WRAPPED_CALL(seekSet, pos); }
bool HalFile::seek64(uint64_t pos) { HAL_FILE_WRAPPED_CALL(seekSet, pos); }
bool HalFile::seekCur(int64_t offset) { HAL_FILE_WRAPPED_CALL(seekCur, offset); }
bool HalFile::seekSet(size_t offset) { HAL_FILE_WRAPPED_CALL(seekSet, offset); }
int HalFile::available() const { HAL_FILE_WRAPPED_CALL(available, ); }
size_t HalFile::position() const { HAL_FILE_WRAPPED_CALL(position, ); }
int HalFile::read(void* buf, size_t count) { HAL_FILE_WRAPPED_CALL(read, buf, count); }
int HalFile::read() { HAL_FILE_WRAPPED_CALL(read, ); }
size_t HalFile::write(const void* buf, size_t count) { HAL_FILE_WRAPPED_CALL(write, buf, count); }
size_t HalFile::write(uint8_t b) { HAL_FILE_WRAPPED_CALL(write, b); }
bool HalFile::rename(const char* newPath) { HAL_FILE_WRAPPED_CALL(rename, newPath); }
bool HalFile::isDirectory() const { HAL_FILE_FORWARD_CALL(isDirectory, ); } // already thread-safe, no need to wrap
void HalFile::rewindDirectory() { HAL_FILE_WRAPPED_CALL(rewindDirectory, ); }
bool HalFile::close() { HAL_FILE_WRAPPED_CALL(close, ); }
HalFile HalFile::openNextFile() {
HalStorage::StorageLock lock;
assert(impl != nullptr);
return HalFile(std::make_unique<Impl>(impl->file.openNextFile()));
}
bool HalFile::isOpen() const { return impl != nullptr && impl->file.isOpen(); } // already thread-safe, no need to wrap
HalFile::operator bool() const { return isOpen(); }