Remove copyFile/renameFile reimplementations, adjust STM32 SafeFile to match NRF52#10072
Remove copyFile/renameFile reimplementations, adjust STM32 SafeFile to match NRF52#10072Stary2001 wants to merge 1 commit intomeshtastic:developfrom
Conversation
…p to date enough LittleFS to use the builtin ones, and NRF52/STM32 don't use it.
github-actions
bot
added
the
hardware-support
|
Between "develop" and this commit there is a hardfault. Edit: copying the three files limited to this commit over the develop (2f19a1d) config save seems like working on STM32WL |
There was a problem hiding this comment.
Pull request overview
Removes previously added filesystem copy/rename helper reimplementations and updates SafeFile to use the platform FS rename directly (or skip atomic behavior on NRF52/STM32WL), aligning STM32WL behavior with NRF52 to avoid prior LittleFS-related issues.
Changes:
- Removed
copyFile()/renameFile()helpers fromFSCommon. - Updated
SafeFileto bypass atomic tmp/rename flow onARCH_STM32WL(matching NRF52) and to callFSCom.rename()directly elsewhere. - Added explicit SPI locking around the
FSCom.rename()call inSafeFile::close().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/SafeFile.cpp | Adjusts STM32WL to follow NRF52 “write-in-place” path; switches rename to FSCom.rename() with explicit locking. |
| src/FSCommon.h | Removes declarations for copyFile() / renameFile(). |
| src/FSCommon.cpp | Removes implementations of copyFile() / renameFile(). |
Comments suppressed due to low confidence (1)
src/FSCommon.cpp:40
- The file header comment still says FSCommon provides "copying" and "renaming" filesystem operations, but the copy/rename helpers were removed in this PR. Update the top-of-file documentation to match the current responsibilities (listing/deleting/etc.) to avoid misleading readers.
#include <vector>
/**
* @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.
*
| // Only way to work on both esp32 and nrf52 | ||
| static File openFile(const char *filename, bool fullAtomic) | ||
| { | ||
| concurrency::LockGuard g(spiLock); | ||
| LOG_DEBUG("Opening %s, fullAtomic=%d", filename, fullAtomic); | ||
| #ifdef ARCH_NRF52 | ||
| #if defined(ARCH_NRF52) || defined(ARCH_STM32WL) | ||
| FSCom.remove(filename); | ||
| return FSCom.open(filename, FILE_O_WRITE); |
There was a problem hiding this comment.
openFile() comment says this is the only way to work on esp32 and nrf52, but the conditional now also applies to ARCH_STM32WL. Update the comment to reflect the actual supported architectures (or reword it to be architecture-agnostic) so future readers don’t misinterpret why STM32WL is in this fast-path.
| /** | ||
| * Atomically close the file (overwriting any old version) and readback the contents to confirm the hash matches | ||
| * | ||
| * @return false for failure | ||
| */ | ||
| bool SafeFile::close() | ||
| { | ||
| if (!f) | ||
| return false; | ||
|
|
||
| spiLock->lock(); | ||
| f.close(); | ||
| spiLock->unlock(); | ||
|
|
||
| #ifdef ARCH_NRF52 | ||
| #if defined(ARCH_NRF52) || defined(ARCH_STM32WL) | ||
| return true; | ||
| #endif |
There was a problem hiding this comment.
SafeFile::close() is documented as doing an atomic replace and readback hash verification, but for ARCH_NRF52 and now ARCH_STM32WL it returns early and skips testReadback() and the tmp-file rename step. Please adjust the function/docstring to accurately describe the behavior on these platforms (and consider whether callers relying on fullAtomic=true should be able to detect that atomic mode isn’t available).
| spiLock->lock(); | ||
| if (!FSCom.rename(filenameTmp.c_str(), filename.c_str())) { | ||
| spiLock->unlock(); | ||
| LOG_ERROR("Error: can't rename new pref file"); |
There was a problem hiding this comment.
The rename failure log says "new pref file", but SafeFile is used for more than prefs (e.g., sensor state / NicheGraphics data). Make this error message generic and/or include the target path so the log is accurate and useful outside prefs writes.
| LOG_ERROR("Error: can't rename new pref file"); | |
| LOG_ERROR("Error: can't rename tmp file %s to %s", filenameTmp.c_str(), filename.c_str()); |
Use the built-in rename, or none instead.
Temporarily added for NRF52 in #1594, but we're not using it on NRF52 any more, and it caused problems for STM32.
Needs testing on RP2040.
Will supersede #9927 .
🤝 Attestations