mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-20 05:18:54 -04:00
Ben Meadors
585ce17f59
* fix(nrf52): serialise the warm-node ring against LittleFS on the shared flash cache On nRF52840 the warm-node store writes its 3-page record ring straight through flash_nrf5x_write/erase/flush, holding only spiLock. Every LittleFS writer instead holds Adafruit_LittleFS's own mutex, and two of them run on other tasks entirely: Bluefruit's bond saves on the callback task, and - since phone config writes moved into BLE context - a whole saveToDisk on the BLE task. Neither takes spiLock. Both writers share one 4 KB page cache, one SoftDevice flash semaphore and one result word. flash_cache_write repoints that cache when the requested page differs from the cached one, so a second writer arriving mid-write flushes the first writer's page and re-points the buffer; the first writer's remaining memcpy then lands in the wrong page's image. Ring records end up inside LittleFS metadata, or the reverse. The collision also exhausts the flash layer's 20 x 1 ms busy-retry budget against an 85 ms page erase, and flash_cache_flush discards the failure, so 32 LittleFS blocks vanish with no error reaching the filesystem. What the user sees is a torn directory pair on the next mount, a format, and critical error 13. Take the filesystem mutex in the five ring entry points that reach flash, after spiLock and never before - the order every existing path already uses. The ring touches no LittleFS call itself, so the non-recursive mutex is never re-entered. Longest new hold is a page rotation at roughly half a second, against a 2 s supervision timeout and a 90 s watchdog. Non-nRF52840 backends are untouched. * fix(nodedb): make saveProto report a failed readback or rename SafeFile::close() already verifies the .tmp by hash and renames it over the live file, and saveProto captured that result, logged it, and then returned the pb_encode status alone. A torn or half-programmed page therefore counted as a successful save: for the fullAtomic files the old contents silently survived, for nodes.proto (written in place) the file was simply gone, and saveToDisk's recovery path never fired for the one failure it exists for. * fix(nodedb): retry a failed save before formatting, and never format on a low rail saveToDisk answered any failed write with an immediate fsFormat(), which is where most "critical error 12/13" reports and the total config wipe behind them come from. A write that fails once is far more often a busy SoftDevice or a VDD dip mid-save than a corrupt filesystem, so: - retry twice, 150 ms apart, re-checking powerHAL_isPowerLevelSafe() before each attempt and before the format; on a low rail return false and leave the filesystem alone (the next save lands once the rail recovers, and boot already waits for a safe level) - check fsFormat()'s result instead of assuming it worked - after a successful format rewrite every segment, not only the ones this call asked for: the format took config.proto and the node identity with it, so a nodes-only save that ended in a format used to come back up as a new node - with encrypted storage a format also destroys the DEK; skip the resave rather than land the private key and PSKs on flash in plaintext RP2040 feeds its watchdog across the delays, as the neighbouring code does. * fix(nrf52): quiesce flash before every software reset and power-off The Adafruit flash layer keeps one 4 KB page image and one SoftDevice flash semaphore for the whole chip. Every reset path we own - Power::reboot(), enterDfuMode() (admin enter_dfu_mode_request, which arrives on the BLE task since #10967), cpuDeepSleep()'s reset and system-off arms, and the wio-t1000-s secure DFU handler - went straight to NVIC_SystemReset or sd_power_system_off while another task could be half-way through a page program or erase. A reset in that window leaves the page erased or partly programmed; LittleFS finds the torn metadata on the next mount and the corruption handler formats the filesystem. nrf52FlashQuiesce() takes spiLock and the LittleFS mutex, waits out whatever write is in flight, flushes the page cache, and keeps both locks because the caller resets next. The corruption-reboot handler and __assert_func are left alone: they run inside the filesystem call stack or a fault, where taking the mutex would deadlock. nRF54L is a second copy of these paths since #11867 and still defines ARCH_NRF52, so it gets the same function on the same core flash layer. * fix(nrf52): quiesce flash before the library BLE DFU handler jumps to the bootloader On every board except wio-t1000-s the Nordic DFU service is the framework's BLEDfu, whose START_DFU handler runs on the callback task and jumps to the bootloader with no regard for a flash write in progress on the loop task. That is the OTA path the Apple app and nRF Connect use (Android sends enter_dfu_mode_request instead, which the previous commit covers). QuiescingBLEDfu re-installs the control-point write callback after BLEDfu::begin() and wraps the library's: flush under both locks, then drop the LittleFS mutex before handing over, because the library reloads the bond keys through LittleFS on its way to the jump and the mutex is not recursive. spiLock stays held across the handler: every LittleFS writer on the BLE task takes it first, the loop task cannot preempt the callback task, and the handler never blocks after the flush, so nothing can dirty flash before bootloader_util_app_start(). If the handler returns, nothing jumped, and the lock is released. The library callback is a file-static, so it is read back out of the characteristic through a pointer-to-member obtained via a using-declaration; that is well-formed C++ and compiles under the pinned GCC 9.3 with LTO. * test(nodedb): pin the save-failure contract of saveProto and saveToDisk A failed rename must come back as false from saveProto, a one-off unsafe rail reading during a write must be retried and land, and a rail still unsafe at the retry gate must make saveToDisk return false with the filesystem untouched. The rail is scripted through a strong powerHAL_isPowerLevelSafe() over the weak native default; on Windows the default is strong, so only the rename case runs there. The format branch itself is unreachable natively (a FLASH_CORRUPTION critical error exits the portduino process), which is what the survival assertions pin. * fix(nodedb): only format when the filesystem itself is unreadable Making saveProto honest about write failures gave the recovery path a new way in: any persistent write failure now reached fsFormat(), which takes every file with it. A busy or lock-protected nRF52 flash fails every write for as long as it lasts, so two retries are not enough to tell that apart from a corrupt filesystem, and guessing wrong costs the node its config, keys and bonds. Reads settle it. They never touch the SoftDevice write path that a busy flash fails on, so if /prefs still walks and a stored proto still opens and reads, the metadata chain is intact and the write failure was transient - return false and let the caller try again later. Genuine corruption is not silently tolerated: lfs asserts on it, and the nRF52 handler reboots and formats on the way back up. Covered by a test that fails without this: a save whose rename cannot succeed, against an otherwise healthy filesystem, must leave devicestate untouched. * trunk: exempt Unity test entry points from trufflehog trufflehog's Lob detector matches "test_" followed by alphanumerics, which describes every Unity test function name. It fired on a new test in test_nodedb_save_retry and will fire again on the next suite added. Scoped to test/**/test_main.cpp, alongside the existing gitleaks exemption for the synthetic node-DB fixtures. * fix(nodedb): feed the RP2040 watchdog around the format and the resave saveToDisk() only feeds the watchdog at the top of each retry. The last retry, the readable probe, fsFormat() and the five-segment resave then share one 8 s budget (watchdog_enable in main-rp2xx0.cpp) with no loop left to feed it. A timeout during the resave leaves the filesystem empty and the node boots on defaults with a new identity - the exact outcome this PR exists to prevent, reached by a different road. Feed once before the probe and again before the resave. Both feeds sit outside any lock: filesystemStillReadable() takes spiLock itself, and the format has already released it. ARCH_RP2040 covers rp2040 and rp2350 alike, and the blocks compile out everywhere else, so no other platform and no native test changes. Raised by @caveman99 in review. * fix(nodedb): narrow the save-probe comment and name the full-filesystem case The comment on filesystemStillReadable() claimed "real corruption asserts in lfs and formats on reboot". That does hold on nRF52 - nrf52.ini builds with -DLFS_NO_ASSERT and force-includes cpp_overrides/lfs_util.h, whose LFS_NO_ASSERT arm routes LFS_ASSERT to the lfs_assert() in main-nrf52.cpp, which stamps NRF52_MAGIC_LFS_IS_CORRUPT and resets into the format - but NodeDB.cpp compiles for ESP32, RP2040 and portduino too, where nothing of the sort is wired up. It is also not true on nRF52 under POFWARN, where lfs_assert() deliberately skips the stamp. Drop the claim rather than qualify it three ways. The log line now names what a field log actually needs to tell apart: a filesystem that still reads but cannot be written is either busy or full. Raised by @caveman99 in review. * fix(sx128x): quiesce flash before the 2.4GHz region reset reinitChip() saves the region, waits 2 s and resets. On nRF52 that was the last software reset still going straight to NVIC_SystemReset with a page program possibly in flight, so "every software reset" in the earlier commit did not quite hold. The quiesce stays inside the ARCH_NRF52 arm on purpose. The #else arm logs and falls through to lora.setCRC() further down, which re-enters spiBeginTransaction(); a quiesce hoisted above the #if would take spiLock and never give it back, self-deadlocking portduino and stm32wl. Routing this through Power::reboot() is wrong for the same class of reason: setupModules() runs before initLoRa, so its notifyReboot observers and waypointStore.saveToFlash() are live and would add a flash write to an aborted radio init. Raised by @caveman99 in review. * fix(nrf52): only quiesce on the DFU control write that actually resets QuiescingBLEDfu wrapped every control-point write, so a write that was never going to reset still blocked the Bluefruit callback task on spiLock, forced an early page-cache commit and held back the GATT authorize reply. Only START_DFU resets; gate on that. Deliberately no "request->len &&" term. The library's own test is `request->data[0] == START_DFU` with no length check (BLEDfu.cpp:110 in both the nRF52 and nRF54 cores), and Bluefruit hands the callback a copy of a reused event buffer, so a zero-length write carrying a stale 0x01 still resets inside the library. A len term here would let exactly that reset run unquiesced, which is the case this wrapper exists for. Reading data[0] is always in bounds: ble_gatts_evt_write_t declares uint8_t data[1] and the copy covers it. Raised by @caveman99 in review.