From a99b4a7c53f61e16a53daed6cd98e28ce5d542ea Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:36:16 -0700 Subject: [PATCH 1/3] fix(startup): harden boot count persistence against hard-reset corruption HWIL evidence (integration-uart runs 29894110572 / 29979002023, PR #460): the boot count file was read back as 0x02FE191005000001 after a sequence of watchdog-commanded hard resets, and the next boot persisted exactly garbage+1 - the increment machinery works, but nothing guards against a torn flash write poisoning the count forever. Four changes to StartupManager: - Corruption guard: values > 1,000,000 are treated as a failed read (mirrors the existing quiescence-file 0xFF-fill guard) and reported via new WARNING_HI BootCountCorrupted with the raw value. - GET_BOOT_COUNT is now read-only: the unconditional write-back on every query was the largest source of boot-count write traffic, and each write is a window for a reset to tear the file. - Increment retry: if the first-tick persist fails (e.g. filesystem not ready), run_handler retries each 1Hz tick until it sticks - the increment is delayed, not lost. Failure warning de-duplicated per streak. - Atomic persist: write temp file then Os::FileSystem::rename over the target; littlefs renames are atomic so a mid-update reset leaves either the old or new file, never a torn one. Regression seam: test_safe_09_command_loss_triggers_safe_mode_and_reboot asserts boot count == initial+1 across a watchdog reset (with the retry poll from PR #460). Co-Authored-By: Claude Fable 5 --- .../StartupManager/StartupManager.cpp | 54 ++++++++++++++++--- .../StartupManager/StartupManager.fpp | 4 ++ .../StartupManager/StartupManager.hpp | 32 +++++++---- 3 files changed, 72 insertions(+), 18 deletions(-) diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp index acf6b154..9084730d 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp @@ -7,6 +7,7 @@ #include "PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp" #include "Os/File.hpp" +#include "Os/FileSystem.hpp" #include namespace Components { @@ -101,6 +102,11 @@ StartupManager::Status write(const Fw::StringBase& file_path, const T& value) { return return_status; } +// Boot counts beyond this are treated as file corruption rather than real history: a hard reset +// (e.g. the watchdog power cycle used for command-loss recovery) can tear the flash write and leave +// a well-formed file full of junk, which would otherwise be incremented and persisted forever. +static constexpr FwSizeType MAX_PLAUSIBLE_BOOT_COUNT = 1000000; + FwSizeType StartupManager ::get_boot_count(bool increment) { // Read the boot count file path from parameter and assert that it is either valid or the default value FwSizeType boot_count = 0; @@ -108,20 +114,42 @@ FwSizeType StartupManager ::get_boot_count(bool increment) { auto boot_count_file = this->paramGet_BOOT_COUNT_FILE(is_valid); FW_ASSERT(is_valid == Fw::ParamValid::VALID || is_valid == Fw::ParamValid::DEFAULT); - // Open the boot count file and add one to the current boot count ensuring a minimum of 1 in the case - // of read failure. Since read will retain the `0` initial value on read failure, we can ignore the error - // status returned by the read. + // Read the current count ensuring a minimum of 1 after increment in the case of read failure. + // Since read will retain the `0` initial value on read failure, we can ignore the error status. (void)read(boot_count_file, boot_count); + if (boot_count > MAX_PLAUSIBLE_BOOT_COUNT) { + this->log_WARNING_HI_BootCountCorrupted(static_cast(boot_count)); + boot_count = 0; + } boot_count = FW_MAX(1, increment ? boot_count + 1 : boot_count); - // Rewrite the updated boot count back to the file, and on failure emit a warning about the inability to - // persist the boot count. - StartupManager::Status status = write(boot_count_file, boot_count); - if (status != StartupManager::SUCCESS) { - this->log_WARNING_LO_BootCountUpdateFailure(); + + // Only the once-per-boot increment writes the file; plain reads (GET_BOOT_COUNT) must not. + // Every write is a window for a reset to tear the file, and the count is queried far more + // often than it changes. On failure, run_handler retries on subsequent ticks. + if (increment) { + StartupManager::Status status = this->persist_boot_count(boot_count_file, boot_count); + this->m_boot_count_persisted = (status == StartupManager::SUCCESS); + if (!this->m_boot_count_persisted && !this->m_boot_count_write_logged) { + this->log_WARNING_LO_BootCountUpdateFailure(); + this->m_boot_count_write_logged = true; + } } return boot_count; } +StartupManager::Status StartupManager ::persist_boot_count(const Fw::StringBase& file_path, FwSizeType value) { + // Write to a temp file, then rename over the target: littlefs renames are atomic, so a reset + // landing mid-update leaves either the old or the new file - never a torn one. + Fw::String temp_path(file_path); + temp_path += ".tmp"; + StartupManager::Status status = write(temp_path, value); + if (status == StartupManager::SUCCESS && + Os::FileSystem::rename(temp_path.toChar(), file_path.toChar()) != Os::FileSystem::OP_OK) { + status = StartupManager::FAILURE; + } + return status; +} + Fw::Time StartupManager ::update_quiescence_start() { Fw::ParamValid is_valid; auto time_file = this->paramGet_QUIESCENCE_START_FILE(is_valid); @@ -186,6 +214,16 @@ void StartupManager ::run_handler(FwIndexType portNum, U32 context) { Fw::ParamString first_sequence = this->paramGet_STARTUP_SEQUENCE_FILE(is_valid); FW_ASSERT(is_valid == Fw::ParamValid::VALID || is_valid == Fw::ParamValid::DEFAULT); this->runSequence_out(0, first_sequence); + } else if (!this->m_boot_count_persisted) { + // The first-tick write can fail transiently (e.g. filesystem not ready yet). Re-attempt each + // tick until the count is durably stored, so the increment is delayed rather than lost. + auto boot_count_file = this->paramGet_BOOT_COUNT_FILE(is_valid); + FW_ASSERT(is_valid == Fw::ParamValid::VALID || is_valid == Fw::ParamValid::DEFAULT); + this->m_boot_count_persisted = + (this->persist_boot_count(boot_count_file, this->m_boot_count) == StartupManager::SUCCESS); + if (this->m_boot_count_persisted) { + this->m_boot_count_write_logged = false; + } } // Calculate the quiescence end time based on the quiescence period parameter diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.fpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.fpp index ee6c634b..23f31359 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.fpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.fpp @@ -33,6 +33,10 @@ module Components { event BootCountUpdateFailure() severity warning low \ format "Failed to update boot count file" + @ Event emitted when the boot count file holds an implausible value (torn or corrupt write) + event BootCountCorrupted(raw: I64) severity warning high \ + format "Boot count file corrupt (raw value {}) - treating as unreadable" + @ Event emitted when the quiescence file was not updated event QuiescenceFileInitFailure() severity warning low \ format "Failed to initialize quiescence start time file" diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp index c6cad995..1a216327 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp @@ -30,16 +30,26 @@ class StartupManager final : public StartupManagerComponentBase { //! Destroy StartupManager object ~StartupManager(); - //! \brief read and increment the boot count + //! \brief read and optionally increment the boot count //! - //! Reads the boot count from the boot count file, increments it, and writes it back to the file. If the read - //! fails, the boot count will be initialized to 1. If the write fails, a warning will be emitted. + //! Reads the boot count from the boot count file. If the read fails or the stored value is implausible + //! (torn/corrupt write), the count is treated as unread and initializes to 1 on increment. Only the + //! increment path writes the file; plain reads leave it untouched. If the write fails, a warning is + //! emitted and the write is retried on subsequent run ticks. //! - //! \warning this function will modify the boot count file on disk. + //! \warning this function will modify the boot count file on disk when increment is true. //! //! \return The updated boot count FwSizeType get_boot_count(bool increment); + //! \brief durably persist the boot count via write-to-temp + atomic rename + //! + //! littlefs renames are atomic, so a reset landing mid-update leaves either the old or the new + //! file — never a torn one. + //! + //! \return Status of the persist operation + Status persist_boot_count(const Fw::StringBase& file_path, FwSizeType value); + //! \brief get and possibly initialize the quiescence start time //! //! Reads the quiescence start time from the quiescence start time file. If the read fails, the current time is @@ -98,12 +108,14 @@ class StartupManager final : public StartupManagerComponentBase { ) override; private: - Fw::Time m_quiescence_start; //!< Time of the start of the quiescence wait - FwOpcodeType m_stored_opcode; //!< Stored opcode for delayed response - FwSizeType m_boot_count; //!< Current boot count - U32 m_stored_sequence; //!< Stored sequence number for delayed response - std::atomic m_waiting; //!< Indicates if waiting for quiescence - Fw::String m_sequence_file; //!< The filepath for the sequence last initiated + Fw::Time m_quiescence_start; //!< Time of the start of the quiescence wait + FwOpcodeType m_stored_opcode; //!< Stored opcode for delayed response + FwSizeType m_boot_count; //!< Current boot count + bool m_boot_count_persisted = false; //!< Whether the incremented boot count has reached the file + bool m_boot_count_write_logged = false; //!< Warning already emitted for the current persist-failure streak + U32 m_stored_sequence; //!< Stored sequence number for delayed response + std::atomic m_waiting; //!< Indicates if waiting for quiescence + Fw::String m_sequence_file; //!< The filepath for the sequence last initiated }; } // namespace Components From f426f7cbd08e16cf5e5085e1fa39a43b6a247dc4 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:49:22 -0700 Subject: [PATCH 2/3] fix(startup): flush before close in file write helper; zero-init m_boot_count Addresses CodeRabbit review on #470: Os::File::close() is void and cannot report a deferred flush failure, so the write helper now requires file.flush() == OP_OK before reporting SUCCESS - persist_boot_count no longer renames a temp file whose data may not have reached storage. Also gives m_boot_count an in-class zero initializer; run_handler's first-tick guard reads it before any assignment. Co-Authored-By: Claude Fable 5 --- .../Components/StartupManager/StartupManager.cpp | 5 ++++- .../Components/StartupManager/StartupManager.hpp | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp index 9084730d..6360b333 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp @@ -94,7 +94,10 @@ StartupManager::Status write(const Fw::StringBase& file_path, const T& value) { if (status == Os::File::OP_OK) { FwSizeType size = sizeof(data_buffer); status = file.write(data_buffer, size); - if (status == Os::File::OP_OK && size == sizeof(data_buffer)) { + // Flush before close so the data has reached storage before callers (e.g. the atomic + // rename in persist_boot_count) treat the write as durable. close() returns void and + // cannot report a flush failure. + if (status == Os::File::OP_OK && size == sizeof(data_buffer) && file.flush() == Os::File::OP_OK) { return_status = StartupManager::SUCCESS; } } diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp index 1a216327..46430d57 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp @@ -110,7 +110,7 @@ class StartupManager final : public StartupManagerComponentBase { private: Fw::Time m_quiescence_start; //!< Time of the start of the quiescence wait FwOpcodeType m_stored_opcode; //!< Stored opcode for delayed response - FwSizeType m_boot_count; //!< Current boot count + FwSizeType m_boot_count = 0; //!< Current boot count (0 = first run tick not yet processed) bool m_boot_count_persisted = false; //!< Whether the incremented boot count has reached the file bool m_boot_count_write_logged = false; //!< Warning already emitted for the current persist-failure streak U32 m_stored_sequence; //!< Stored sequence number for delayed response From 92beea07d578c600022434684e5257d78ba93854 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:16:27 -0700 Subject: [PATCH 3/3] =?UTF-8?q?docs:=20correct=20filesystem=20=E2=80=94=20?= =?UTF-8?q?flight=20FS=20is=20FAT=20(ELM=20FatFs),=20not=20littlefs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The persist design claimed littlefs-atomic renames; the flight filesystem is FAT (CONFIG_FAT_FILESYSTEM_ELM, zephyr,fstab,fatfs). FAT renames are not guaranteed power-cut atomic. The write-then-rename design still closes the observed torn-in-place-write window (data fully flushed before it replaces the old file); the residual worst case is a missing file, which reads as a failed read and re-initializes visibly instead of propagating silent garbage. Comments and SDD updated to state this accurately. Co-Authored-By: Claude Fable 5 --- .../Components/StartupManager/StartupManager.cpp | 7 +++++-- .../Components/StartupManager/StartupManager.hpp | 7 ++++--- .../Components/StartupManager/docs/sdd.md | 4 ++-- docs-site/components/StartupManager.md | 4 ++-- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp index 80cf07d0..128f92bd 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp @@ -142,8 +142,11 @@ FwSizeType StartupManager ::get_boot_count(bool increment) { } StartupManager::Status StartupManager ::persist_boot_count(const Fw::StringBase& file_path, FwSizeType value) { - // Write to a temp file, then rename over the target: littlefs renames are atomic, so a reset - // landing mid-update leaves either the old or the new file - never a torn one. + // Write to a temp file, then rename over the target. The flight FS is FAT (ELM FatFs), whose + // rename is not guaranteed power-cut atomic - but the new data is fully written and flushed + // before it replaces the old file, so a reset can no longer tear the value mid-write (the + // observed failure). Worst case during the rename window is a missing file, which reads as a + // failed read and re-initializes the count - detectable, unlike silent garbage. Fw::String temp_path(file_path); temp_path += ".tmp"; StartupManager::Status status = write(temp_path, value); diff --git a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp index 6a780ce5..e0185cbc 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp +++ b/PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp @@ -42,10 +42,11 @@ class StartupManager final : public StartupManagerComponentBase { //! \return The updated boot count FwSizeType get_boot_count(bool increment); - //! \brief durably persist the boot count via write-to-temp + atomic rename + //! \brief durably persist the boot count via write-to-temp + rename //! - //! littlefs renames are atomic, so a reset landing mid-update leaves either the old or the new - //! file — never a torn one. + //! The new value is fully written and flushed before it replaces the old file, so a reset + //! cannot tear the value mid-write. FAT's rename is not power-cut atomic; the residual worst + //! case is a missing file, which reads as a failed read rather than silent garbage. //! //! \return Status of the persist operation Status persist_boot_count(const Fw::StringBase& file_path, FwSizeType value); diff --git a/PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md b/PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md index 63de86ee..fa87289a 100644 --- a/PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md +++ b/PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md @@ -50,12 +50,12 @@ On each 1 Hz `run`, when `DEFAULT_STARTUP_VALUE == 1` and `m_transmit_enable_tic ### Boot count persistence -The boot count lives in `BOOT_COUNT_FILE` on littlefs and is incremented lazily on the first 1 Hz `run` tick of each boot. Hard resets (e.g. the watchdog power cycle used for command-loss recovery) can land while a write is in flight, so the persistence path is hardened in four ways: +The boot count lives in `BOOT_COUNT_FILE` on the flight filesystem (FAT — ELM FatFs, see `prj.conf`) and is incremented lazily on the first 1 Hz `run` tick of each boot. Hard resets (e.g. the watchdog power cycle used for command-loss recovery) can land while a write is in flight, so the persistence path is hardened in four ways: 1. **Corruption guard**: a read that returns a value above 1,000,000 is treated as a failed read (the file contained torn/junk data) and reported via `BootCountCorrupted` with the raw value. The count then re-initializes on the next increment instead of propagating garbage. Observed on HWIL: a torn write left the file reading `0x02FE191005000001`, and the next boot persisted exactly garbage+1. 2. **Read-only queries**: `GET_BOOT_COUNT` never writes the file. Only the once-per-boot increment does, minimizing the window in which a reset can tear a write. 3. **Increment retry**: if the first-tick persist fails (e.g. filesystem not ready), `run` re-attempts it each tick until it succeeds — the increment is delayed, not lost. `BootCountUpdateFailure` is emitted once per failure streak. -4. **Atomic persist**: the value is written to `.tmp`, flushed, and renamed over the target. littlefs renames are atomic, so a reset mid-update leaves either the old or the new file, never a torn one. +4. **Write-then-rename persist**: the value is written to `.tmp`, flushed, and renamed over the target. FAT's rename is not guaranteed power-cut atomic, but the new data is fully on storage before it replaces the old file, closing the torn-in-place-write window that produced the observed garbage. The residual worst case during the rename window is a missing file, which reads as a failed read (count re-initializes, visibly) rather than silent corruption. ## Port Descriptions diff --git a/docs-site/components/StartupManager.md b/docs-site/components/StartupManager.md index 63de86ee..fa87289a 100644 --- a/docs-site/components/StartupManager.md +++ b/docs-site/components/StartupManager.md @@ -50,12 +50,12 @@ On each 1 Hz `run`, when `DEFAULT_STARTUP_VALUE == 1` and `m_transmit_enable_tic ### Boot count persistence -The boot count lives in `BOOT_COUNT_FILE` on littlefs and is incremented lazily on the first 1 Hz `run` tick of each boot. Hard resets (e.g. the watchdog power cycle used for command-loss recovery) can land while a write is in flight, so the persistence path is hardened in four ways: +The boot count lives in `BOOT_COUNT_FILE` on the flight filesystem (FAT — ELM FatFs, see `prj.conf`) and is incremented lazily on the first 1 Hz `run` tick of each boot. Hard resets (e.g. the watchdog power cycle used for command-loss recovery) can land while a write is in flight, so the persistence path is hardened in four ways: 1. **Corruption guard**: a read that returns a value above 1,000,000 is treated as a failed read (the file contained torn/junk data) and reported via `BootCountCorrupted` with the raw value. The count then re-initializes on the next increment instead of propagating garbage. Observed on HWIL: a torn write left the file reading `0x02FE191005000001`, and the next boot persisted exactly garbage+1. 2. **Read-only queries**: `GET_BOOT_COUNT` never writes the file. Only the once-per-boot increment does, minimizing the window in which a reset can tear a write. 3. **Increment retry**: if the first-tick persist fails (e.g. filesystem not ready), `run` re-attempts it each tick until it succeeds — the increment is delayed, not lost. `BootCountUpdateFailure` is emitted once per failure streak. -4. **Atomic persist**: the value is written to `.tmp`, flushed, and renamed over the target. littlefs renames are atomic, so a reset mid-update leaves either the old or the new file, never a torn one. +4. **Write-then-rename persist**: the value is written to `.tmp`, flushed, and renamed over the target. FAT's rename is not guaranteed power-cut atomic, but the new data is fully on storage before it replaces the old file, closing the torn-in-place-write window that produced the observed garbage. The residual worst case during the rename window is a missing file, which reads as a failed read (count re-initializes, visibly) rather than silent corruption. ## Port Descriptions