fix(stm32wl,nrf52,fs): flash hardening, FS platform unification, write-behind LFS cache (FORMAT BREAK)#10171
fix(stm32wl,nrf52,fs): flash hardening, FS platform unification, write-behind LFS cache (FORMAT BREAK)#10171ndoo wants to merge 16 commits intomeshtastic:developfrom
Conversation
thebentern
requested review from
Stary2001,
caveman99 and
Copilot
and removed request for
caveman99
There was a problem hiding this comment.
Pull request overview
This PR hardens flash/filesystem behavior across STM32WL and nRF52 to match ESP32-style atomic writes, removes several arch-specific filesystem conditionals, and introduces a STM32WL LittleFS write-behind cache to support smaller virtual block sizes (format-breaking).
Changes:
- Harden STM32WL flash/LittleFS handling (error-flag clearing, stricter write behavior) and reduce reserved FS space for wio-e5.
- Unify FS behavior across platforms (atomic SafeFile rename path, universal format-on-retry, shared FSCommon/xmodem code).
- Add default-constructible
Fileon STM32WL (and switch nRF52 framework to a fork pending upstream support).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/stm32/wio-e5/platformio.ini | Adjust upload maximum size to reflect reduced FS reservation (20KB). |
| variants/stm32/stm32.ini | Disable backup preferences on flash-constrained STM32 builds. |
| variants/nrf52840/nrf52.ini | Temporarily point nRF52 framework to fork to obtain File() default constructor. |
| src/xmodem.h | Remove arch-conditional File initialization now that File() exists. |
| src/platform/stm32wl/STM32_LittleFS_File.h | Add File() default constructor declaration. |
| src/platform/stm32wl/STM32_LittleFS_File.cpp | Implement File() default constructor binding to InternalFS. |
| src/platform/stm32wl/LittleFS.cpp | Introduce write-behind page cache and change LFS tunables (format break). |
| src/modules/AdminModule.cpp | Gate UI config persistence to screen-capable builds; gate backup admin messages. |
| src/mesh/NodeDB.h | Compile-time exclusion of backup APIs/paths when backup is disabled. |
| src/mesh/NodeDB.cpp | Enable format-on-retry across all platforms; compile-time exclusion of backup impl. |
| src/SafeFile.cpp | Remove nRF52 direct-write bypass; use temp+readback+rename path. |
| src/FSCommon.cpp | Remove arch-specific FS behavior branches; unify path handling and recursive delete. |
ndoo
force-pushed
the
feat/stm32-lfs-cleanup
branch
from
ece4cb2 to
ee115ed
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens and unifies filesystem behavior across STM32WL/NRF52/other platforms, and introduces a format-breaking STM32WL LittleFS optimization (write-behind page cache + smaller virtual blocks) along with variant config updates to reclaim flash space.
Changes:
- Updates STM32WL LittleFS to use a write-behind page cache, reducing LittleFS virtual block size and FS reservation (format break).
- Removes architecture-specific filesystem divergences (SafeFile atomic write path, FSCommon cleanup, xmodem File default construction) and enables format-on-retry broadly.
- Adds build/config adjustments: STM32 maximum_size bump, optional exclusion of backup preferences, and a temporary NRF52 framework fork for File() default ctor support.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/stm32/wio-e5/platformio.ini | Bumps upload maximum_size to reflect reduced FS reservation. |
| variants/stm32/stm32.ini | Excludes backup preferences to save flash on STM32 targets. |
| variants/stm32/russell/platformio.ini | Bumps upload maximum_size to reflect reduced FS reservation. |
| variants/stm32/rak3172/platformio.ini | Bumps upload maximum_size to reflect reduced FS reservation. |
| variants/stm32/milesight_gs301/platformio.ini | Bumps upload maximum_size to reflect reduced FS reservation. |
| variants/stm32/CDEBYTE_E77-MBL/platformio.ini | Bumps upload maximum_size to reflect reduced FS reservation. |
| variants/nrf52840/nrf52.ini | Temporarily points NRF52 framework to a fork for File() default constructor support. |
| src/xmodem.h | Removes arch-specific File initialization by relying on default constructor. |
| src/platform/stm32wl/STM32_LittleFS_File.h | Declares a default File() constructor for STM32WL LittleFS File. |
| src/platform/stm32wl/STM32_LittleFS_File.cpp | Implements default File() constructor binding to InternalFS. |
| src/platform/stm32wl/LittleFS.cpp | Implements write-behind page cache and new LittleFS tunables (format break). |
| src/modules/AdminModule.cpp | Gates UI config persistence behind HAS_SCREEN; gates backup admin commands behind MESHTASTIC_EXCLUDE_BACKUP. |
| src/mesh/NodeDB.h | Compiles out backup-related APIs/state when MESHTASTIC_EXCLUDE_BACKUP is set. |
| src/mesh/NodeDB.cpp | Enables format-on-retry path universally; compiles out backup implementation when excluded. |
| src/SafeFile.cpp | Removes NRF52 direct-write bypass so all platforms use tmp+readback+rename path. |
| src/FSCommon.cpp | Simplifies rename/list/delete logic and reduces arch-specific handling. |
|
Part 1 flash driver changes are fine with a couple comments. |
Stary2001
left a comment
Stary2001
left a comment
There was a problem hiding this comment.
Add the HAL_FLASH_Lock fix and that should be it.
|
CI fails for native: |
I will look at this later - needs fixing |
Wait, which branch was I supposed to target? |
Geez, I hate when Claude decides to post something without checking with me first. |
_internal_flash_prog already checks HAL_FLASH_Unlock() and returns LFS_ERR_IO on failure. _internal_flash_erase discarded the return value, proceeding to erase even if the flash was not unlocked. Apply the same check for consistency and safety. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously the programming loop continued to the next doubleword after HAL_FLASH_Program() failed, potentially writing to invalid addresses and returning a misleading error code only at the end (last iteration). HAL_FLASH_Lock() was also skipped on the mid-loop early return path. - Move bounds check before the loop (validate full range at once) - Break on first HAL error so subsequent doublewords are not written - Move HAL_FLASH_Lock() after the loop so it always runs Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Stale error flags in FLASH->SR from a previous failed operation can cause HAL_FLASH_Program() or HAL_FLASHEx_Erase() to return HAL_ERROR immediately without attempting the operation. Add __HAL_FLASH_CLEAR_FLAG(FLASH_FLAG_ALL_ERRORS) after each HAL_FLASH_Unlock() in both _internal_flash_prog and _internal_flash_erase to ensure a clean state before each operation. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The STM32WL HAL minimum write unit is one 64-bit doubleword (8 bytes). _internal_flash_prog silently truncated any trailing bytes when size % 8 != 0 because dw_count = size / 8 drops the remainder. Return LFS_ERR_INVAL early so LittleFS sees the error rather than a silent short write. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Ok, here comes a real human reply (KILL ALL HUMANS…)
I think these will be resolved shortly…
Actually, now that we have smaller virtual blocks, we actually have space for backup, so I will drop the commit entirely.
I’ve been testing the integration branch on a T-Echo, but yes it does need more testing.
Probably… we should probably watch the utilization and consider reducing the allocation more… |
NRF52 was bypassing the .tmp/readback/rename path entirely — openFile() deleted the target file and wrote directly to it, and close() returned true without verifying the write or renaming anything. Adafruit_LittleFS::rename() calls lfs_rename() directly (confirmed at Adafruit_LittleFS.cpp:205). Remove both ARCH_NRF52 guards so NRF52 follows the same write-to-.tmp → readback-hash → rename path used by all other platforms. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
handleStoreDeviceUIConfig() was writing /prefs/uiconfig.proto unconditionally. MenuHandler.cpp is already gated behind #if HAS_SCREEN, so there is no path that populates UI config on screen-less platforms. Guard the save with #if HAS_SCREEN to avoid wasting a flash block on devices that will never use it. The read path (handleGetDeviceUIConfig) does not touch the filesystem and needs no change. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The FSCom.format() call on save failure was guarded to ARCH_NRF52 with a comment that other platforms were not ready (bug meshtastic#4184). STM32WL was added to the guard in a prior commit. All platforms now expose format semantics and the retry logic is identical — remove the guard. To keep NodeDB.cpp platform-agnostic and fix a CI failure on native-tft (portduino's fs::FS has no format() method), introduce fsFormat() in FSCommon as the single call-site for all callers: - Embedded (ESP32, NRF52, STM32WL, RP2040): delegates to FSCom.format() - Portduino: rmDir("/prefs") + FSBegin() (a no-op on portduino). rmDir("/prefs") is already called unconditionally by factoryReset() (NodeDB.cpp:504), so both primitives are proven on portduino. Replace both direct FSCom.format() calls in NodeDB.cpp with fsFormat(). Note: we do not run portduino locally — portduino/native build testers please verify the format-on-retry path. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…rnalFS Adds File() to the Adafruit LittleFS File class (in the Meshtastic Adafruit_nRF52_Arduino fork), delegating to File(InternalFS). This matches the default-constructible File API on all other platforms. The constructor is implemented in Adafruit_LittleFS_File.cpp rather than inline in the header to avoid a circular include between Adafruit_LittleFS_File.h and InternalFileSystem.h. FOLLOW-UP REQUIRED: nrf52.ini points to a commit SHA on the mesh-malaysia/Adafruit_nRF52_Arduino fork instead of the upstream meshtastic framework. Once meshtastic/Adafruit_nRF52_Arduino#5 is merged, revert nrf52.ini to point back to the upstream meshtastic framework URL. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds File() to STM32_LittleFS_Namespace::File, delegating to File(InternalFS). Implemented in the .cpp to avoid a circular include between STM32_LittleFS_File.h (which cannot include LittleFS.h) and the InternalFS extern declaration. This matches the File API on ESP32/RP2040/Portduino and is a prerequisite for removing the ARCH_STM32WL guard in xmodem.h. No behavior change — the constructor leaves the file in the same closed/unattached state as File(InternalFS) would. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Now that NRF52 and STM32WL have File() default constructors and NRF52 has working atomic SafeFile rename, the capability gaps are closed. Remove all per-arch guards across the shared FS layer: FSCommon.cpp — renameFile(): Use FSCom.rename() on all platforms. Adafruit_LittleFS::rename() calls lfs_rename() directly (Adafruit_LittleFS.cpp:205). The copy+delete fallback on NRF52/RP2040 was never necessary. FSCommon.cpp — getFiles(): Replace four ARCH_ESP32 guards with a single filepath pointer at the top of the loop (file.path() on ESP32, file.name() elsewhere). Fix strcpy(fileInfo.file_name, filepath): bounded to sizeof(fileInfo.file_name)-1 with explicit NUL termination to prevent overflow of the 228-byte meshtastic_FileInfo::file_name array. FSCommon.cpp — listDir(): Same filepath pointer approach. NRF52/STM32WL were in an else-branch that only logged but never deleted — now all platforms follow the unified del path. 12 guards → 2. Fix three strncpy(buffer, ..., sizeof(buffer)) calls that did not NUL-terminate when source length >= sizeof(buffer) (255 bytes). Add explicit buffer[sizeof(buffer)-1] = '\0' after each. FSCommon.cpp — rmDir(): Use listDir(del=true) everywhere. The ARCH_NRF52 rmdir_r() path and the ARCH_ESP32|RP2040|PORTDUINO listDir() path collapse to one line. SafeFile.cpp: ARCH_NRF52 bypass removed (handled in preceding commit). xmodem.h: File file; now works on all platforms via default constructors added in the two preceding commits. Remaining #ifdef ARCH_ESP32 in FSCommon.cpp: exactly 4, all for the file.path() vs file.name() API difference (ESP32 Arduino LittleFS returns the full path; all others return only the name). That difference lives in the framework and cannot be closed here. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd FS reservation (FORMAT BREAK)
Adds a write-behind (RMW) page cache to the STM32WL LittleFS driver,
modelled after the NRF52 Adafruit approach (flash_cache.c). This allows
LFS to use 256-byte virtual blocks backed by 2048-byte physical pages:
the erase/prog callbacks accumulate changes in a 2 KB RAM buffer; the
sync callback (and page eviction on page-change) flushes with a single
HAL physical-erase + doubleword-program pass.
LFS tunables changed (FORMAT BREAK — superblock parameters):
block_size: 2048 B → 256 B (8 virtual blocks per physical page)
read_size: 2048 B → 256 B (= block_size)
prog_size: 2048 B → 256 B (= block_size; hardware min is 8 B)
block_count: 112 → 80 (14 phys pages → 10 phys pages = 20 KiB)
Benefits:
- Internal fragmentation: max 2047 B/file → max 255 B/file
- Heap per open LFS file: ~4 KB → 512 B (prog + read buffers)
- Code flash headroom: 6.7 KB → ~14.1 KB (+7.4 KB)
- Block budget: 80 virtual blocks, worst-case peak ~20, ~60 free
Updates board_upload.maximum_size in wio-e5/platformio.ini from 233472
(256 KB − 28 KB) to 241664 (256 KB − 20 KB) to match the reduced FS
reservation.
Justification for the format break: the prior STM32WL firmware had
several flash write bugs fixed earlier in this series (missing error
flag clearing, no abort on first write failure, unaligned write
acceptance). These bugs very likely caused silent config corruption on
deployed devices. The format break should be treated as an enhancement:
it provides a clean, reliably-written starting point. Users will need
to reconfigure their device once after this update.
Correctness fixes applied to the cache implementation:
- alignas(8) on _page_cache: the buffer was uint8_t[] (alignment 1)
but _flash_cache_flush casts it to const uint64_t* — undefined
behaviour per C++ standard, potential Cortex-M hardfault. alignas(8)
guarantees the required alignment for the doubleword cast.
- HAL_FLASH_Lock() return value: was discarded. Now assigned to
lock_rc and propagated into rc if prior writes succeeded, so LFS
sees the error rather than a false success.
Signed-off-by: Andrew Yong <me@ndoo.sg>
Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
ndoo
force-pushed
the
feat/stm32-lfs-cleanup
branch
from
cac0908 to
f5f29f3
Compare
I had CC implement this for Portduino in 5623be6 |
|
Will update the PR text later, but now is a good time to request a Copilot re-review. |
|
PR text updated. |
There was a problem hiding this comment.
Pull request overview
This PR hardens STM32WL flash/LittleFS behavior, unifies filesystem feature parity across architectures (reducing arch-specific #ifdef paths), and introduces a format-breaking STM32WL LittleFS optimization using a write-behind page cache plus smaller virtual blocks/reservation.
Changes:
- Reduce STM32WL filesystem reservation and adjust LittleFS tunables to use a write-behind page cache with 256-byte virtual blocks (format-breaking).
- Unify FS behaviors across platforms: atomic
SafeFileusage on nRF52, sharedfsFormat()helper, and simplified FSCommon implementations. - Add default
File()constructor support for STM32WL and simplify XModem adapter file handling; gate UI config persistence onHAS_SCREEN.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| variants/stm32/wio-e5/platformio.ini | Adjust max firmware size to reserve 20KB for FS (down from 28KB). |
| variants/stm32/russell/platformio.ini | Same FS reservation adjustment. |
| variants/stm32/rak3172/platformio.ini | Same FS reservation adjustment. |
| variants/stm32/milesight_gs301/platformio.ini | Same FS reservation adjustment. |
| variants/stm32/CDEBYTE_E77-MBL/platformio.ini | Same FS reservation adjustment. |
| variants/nrf52840/nrf52.ini | Pin Adafruit nRF52 framework to a specific fork SHA for File() default ctor dependency. |
| src/xmodem.h | Remove arch-specific File construction; rely on default ctor. |
| src/platform/stm32wl/STM32_LittleFS_File.h | Declare STM32WL File() default constructor binding to InternalFS. |
| src/platform/stm32wl/STM32_LittleFS_File.cpp | Implement STM32WL File() default constructor. |
| src/platform/stm32wl/LittleFS.cpp | Implement STM32WL write-behind page cache + smaller virtual block size and updated mount/format behavior. |
| src/modules/AdminModule.cpp | Avoid saving UI config on builds without screens (#if HAS_SCREEN). |
| src/mesh/NodeDB.cpp | Use new fsFormat() helper for reset/retry formatting paths. |
| src/SafeFile.cpp | Remove nRF52 direct-write bypass; use standard atomic temp+readback+rename path. |
| src/FSCommon.h | Add fsFormat() declaration. |
| src/FSCommon.cpp | Simplify renameFile(), add fsFormat(), and unify list/remove logic across architectures. |
…REAK) Reduces LFS_FLASH_TOTAL_SIZE from 10 × 2 KiB pages (20 KiB) to 7 × 2 KiB pages (14 KiB), freeing 6 KiB for firmware. board_upload.maximum_size updated accordingly across all STM32WL variants: 241664 (256 KiB - 20 KiB) → 247808 (256 KiB - 14 KiB) This is a FORMAT BREAK: existing filesystems must be erased before use. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
Avoids undefined behavior and -Wreturn-type warnings in configurations that compile FSCommon.cpp without a filesystem backend. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
STM32WL LittleFS: 7-page (14 KiB) reservation safety analysis
This gives us a little bit of space back for code.
|
ndoo
changed the title
|
PR title updated. Also added a FLASH usage summary:
|
|
meshtastic/Adafruit_nRF52_Arduino#4 is merged but meshtastic/Adafruit_nRF52_Arduino#5 needs to be merged too; edit: because the lib_deps pulls from the cpp17 branch |
|
@copilot resolve the merge conflicts in this pull request |
I can do this as part of a bigger rebase on head of develop if there's any merge conflicts. But first please help to merge that the cpp17 branch of the other PR (sorry, on my phone so a bit of a hassle to copy the link) |
|
It's merged already |
|
Copilot is bad about not obeying me on other folk's remotes anyway 😅 |
Summary
Three parts, ordered from least to most disruptive:
#ifdefchainsEach part is independently reviewable. Builds verified after each commit.
Part 1 — FS platform unification
What was different
SafeFileuses.tmp/readback/renamesaveToDiskformat-on-retryrenameFileusesFSCom.rename()Filedefault constructorlistDir(del=true)deletes filesChanges
src/SafeFile.cpp.tmp/readback/rename and writing directly to the final filenamesrc/mesh/NodeDB.cppfsFormat()helper; remove#if ARCH_NRF52guardsrc/FSCommon.cppfsFormat()(portableFSCom.format()wrapper, Portduino-aware); unifyrenameFile()to useFSCom.rename()on all platforms; consolidatelistDir(),getFiles(),rmDir()with a singlefilepathpointer — ~18 arch-specific guards removedsrc/modules/AdminModule.cpphandleStoreDeviceUIConfigbehind#if HAS_SCREEN— no benefit persisting UI config on screen-less devicessrc/platform/stm32wl/STM32_LittleFS_File.h/.cppFile()default constructor — prerequisite for unifiedxmodem.hsrc/xmodem.hFile file;— no arch guard neededvariants/nrf52840/nrf52.inimesh-malaysia/Adafruit_nRF52_ArduinoSHA with NRF52File()default constructor (pending meshtastic/Adafruit_nRF52_Arduino#5)Behavior unchanged for ESP32, RP2040, Portduino — only NRF52/STM32WL code paths change to match what other platforms already do.
Part 2 — STM32WL flash driver hardening
Four bugs in the original driver caused silent flash corruption. All are fixed by design in the write-behind cache rewrite (Part 3), which concentrates all HAL I/O in
_flash_cache_flush():_flash_cache_flush()HAL_FLASH_Unlock()return unchecked in erase — always proceeded even if unlock failedLFS_ERR_IOif unlock failsHAL_FLASH_Programerrors ignored — loop continued through remaining doublewords__HAL_FLASH_CLEAR_FLAG(FLASH_FLAG_ALL_ERRORS)before eraseuint64_t*Part 3 — STM32WL write-behind page cache (FORMAT BREAK)
Adds a RMW page cache (same approach as NRF52 Adafruit
flash_cache.c, adapted for the 2048B STM32WL physical page). LFS uses 256-byte virtual blocks; erase/prog calls accumulate in the RAM cache and the physical page is flushed on sync or when a different page is addressed.block_sizeblock_countFS reservation applied to all STM32WL variants via
board_upload.maximum_size:wio-e5,rak3172,russell,CDEBYTE_E77-MBL,milesight_gs301.This is a LFS superblock parameter change — existing devices will reformat on first boot and lose stored config. The flash driver bugs fixed in Part 2 very likely caused silent corruption on deployed devices; the format break is justified and should be documented in release notes.
Build verification
01bd4cfb)3dc3948a)wio-e5rak3172rak4631heltec-v3picoDependencies
File()default constructor for NRF52. Once merged, revertvariants/nrf52840/nrf52.inito upstream.🤖 Generated with Claude Code