From ad61687e5178016983ace0c5ac09fa907702a31c Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:24:04 -0700 Subject: [PATCH 1/9] docs: add S-Band reintegration TDD plan Approved 3-PR plan to bring S-Band back into main with every fix pinned by a test written first: PR 1 (infra: StackMonitor + Kconfig flips), PR 2 (component rework + UTs, still not in topology), PR 3 (topology re-enable, soak-gated). Root causes, decisions (D1-D7), and per-slice acceptance criteria for issues #122/#299 and PR #256 (S-Band removal). Co-Authored-By: Claude Fable 5 --- S-BAND-REINTEGRATION-PLAN.md | 166 +++++++++++++++++++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 S-BAND-REINTEGRATION-PLAN.md diff --git a/S-BAND-REINTEGRATION-PLAN.md b/S-BAND-REINTEGRATION-PLAN.md new file mode 100644 index 00000000..b242e6be --- /dev/null +++ b/S-BAND-REINTEGRATION-PLAN.md @@ -0,0 +1,166 @@ +# S-Band Reintegration TDD Plan + +**Goal:** Bring the S-Band (SX1280/RadioLib) radio back into `main`, fixing the defects +that got it removed in PR #256 ("Remove SBand to protect RAM"), with every fix pinned +by a test written first. + +**Related:** issue #122 (library can lock up / RAM concerns), issue #299 (stack +overflow analysis), PR #175 (original MVP), PR #256 (removal), PR #109 (radio fault +management / nRST reset), branch `s-band-speedup` (post-removal correctness fixes). + +> Naming note: #122 says "RadioHead" but the flight code uses **RadioLib** +> (`lib/RadioLib`, jgromes). Correct this when updating the issues. + +--- + +## Root causes (as-investigated, 2026-07-13) + +1. **Stack overflow on RX** (#299): `deferredRxHandler` runs on the SBand thread with a + 256-byte local array, then calls `dataOut_out` — a *synchronous* call chain through + `ComCcsdsSband` (frameAccumulator → deframer → router) — overflowing the 4096-byte + stack. +2. **Why "just raise the stack" failed:** `CONFIG_DYNAMIC_THREAD_STACK_SIZE=4096` is a + single global for the 25-slot Zephyr thread pool (`DYNAMIC_THREAD_ALLOC=n`). Raising + it costs ~100 KB across all threads on a 520 KB part → no boot. + **Never tried:** `CONFIG_DYNAMIC_THREAD_ALLOC=y` lets one oversized request fall back + to a single boot-time `k_malloc` (~8 KB total cost); all other threads keep pool stacks. +3. **In-flight heap churn** (#122 comment): RadioLib defaults to dynamic allocation; + `RADIOLIB_STATIC_ONLY` is not set. +4. **Unbounded waits** (#122): RadioLib internal BUSY/status polling can spin forever if + SPI/BUSY wedges (the BUSY line wasn't even wired until `s-band-speedup` 7c473f4). +5. **No tests:** the SBand UT block is commented out; the component has no seam + (`FprimeHal` → `Module` → `SX1280` held by value). + +## Decisions (grilled 2026-07-12/13) + +| # | Decision | +|---|----------| +| D1 | Resurrect the existing RadioLib driver in place (no Zephyr-native rewrite, no com-chain async rework). Cherry-pick only *correctness* commits from `s-band-speedup`. | +| D2 | SBand thread gets a dedicated 8 KB stack via `CONFIG_DYNAMIC_THREAD_ALLOC=y` fallback; boot-failure is loud (assert/FATAL); stack usage is *measured*, not assumed. | +| D3 | Failure semantics: bounded call → N consecutive errors/timeouts → auto nRST reset (reuse #109 path) → M failed resets → **FAULTED, latched until ground command**. Invariant: S-Band failure degrades to "no S-Band", never to backpressure, hang, or spacecraft reset. UHF unaffected. | +| D4 | Test seam: new `SBandRadioIf` abstract interface over the 13 SX1280 calls the component uses; production impl wraps SX1280 and owns the bounded-timeout logic; F´ UTs drive the component against a scripted fake. | +| D5 | Three staged PRs (infra → component+UTs → topology re-enable). PR 3 gated on ≥24 h dual-radio HWIL soak + end-to-end functional pass. | +| D6 | Program-wide `StackMonitor` component (all threads, 1 Hz, `k_thread_foreach` + `k_thread_stack_space_get`, needs `CONFIG_THREAD_MONITOR=y`). | +| D7 | Out of scope: throughput tuning (passthrough chunk/cooldown), async com-chain rework (documented fallback only), static stack-analysis tooling, stale `s-band-*` branch cleanup, fprime-zephyr changes (already merged via #326). | + +**Fallback trigger:** if soak shows SBand stack high-water > 70 % of 8 KB, revisit +option C from #299 (async hop in the com chain) before flying. + +--- + +## PR 1 — Infrastructure (SBand stays commented out; zero flight-behavior change) + +### Slice 1.1 — StackMonitor component (tracer bullet) +New passive component `Components/StackMonitor`, seam: `ThreadInfoProviderIf` +(Zephyr impl trivial; UTs use a fake). One RED→GREEN cycle per behavior, in order: + +1. On each `run` tick, reports minimum-free-stack for every thread the provider + exposes (telemetry observable via the component tester). +2. Emits a WARNING_HI EVR when any thread's free stack drops below a threshold + (percentage of its own size), throttled; clears on recovery. +3. Handles thread count changing between ticks without asserting. + +### Slice 1.2 — Loud boot failure on task-start error +Verify what `ActiveComponentBase`/`Os::Task` does today when +`k_thread_stack_alloc` returns null (`ERROR_RESOURCES`). If it can silently limp, +add an assert/FATAL. Behavior to pin (host UT where the Os layer permits, else +HWIL check in PR 3): a deployment whose thread can't get its stack never runs +half-alive. + +### Slice 1.3 — Kconfig flips +`CONFIG_DYNAMIC_THREAD_ALLOC=y`, `CONFIG_THREAD_MONITOR=y`. No new tests (config +only); gate = full existing suite + CI build for v5c/v5d/v5e + a short UHF bench +sanity to prove no regression while SBand is still off. + +**PR 1 exit:** main builds and soaks exactly as before, now with per-thread stack +telemetry visible in YAMCS. + +--- + +## PR 2 — Component rework + UTs (built, tested, still not in the topology) + +Un-comment `add_fprime_subdirectory(.../SBand/)` and the UT registration; the +library and its tests build without any topology change. + +### Slice 2.1 — Seam + first light (tracer bullet) +RED: component UT constructs SBand with a fake `SBandRadioIf`; asserts that startup +configures the radio and arms receive. GREEN: introduce `SBandRadioIf`, refactor +SBand to hold a reference (production default = RadioLib-backed impl), pass. +This cycle proves the whole UT harness path works. + +### Slice 2.2 — RX happy path +Fake presents a received packet → component emits `dataOut` with the exact bytes, +re-arms receive, updates RSSI/SNR telemetry. (Implementation note, not test +subject: read directly into the allocated `Fw::Buffer` — the 256-byte stack array +goes away here.) + +### Slice 2.3 — RX allocation failure +Buffer manager returns invalid buffer → WARNING EVR, receive re-armed, no leak, +no crash. + +### Slice 2.4 — TX path + transmit gate +`dataIn` while ENABLED → radio transmit called with the frame, buffer returned, +comStatus emitted. While DISABLED → no radio call, buffer still returned, +comStatus still emitted (the com queue must never starve). + +### Slice 2.5 — Errors are bounded and counted +Fake returns an error / simulated timeout → call completes promptly, EVR emitted, +consecutive-error counter advances; one success resets the counter. + +### Slice 2.6 — Auto-reset escalation +N consecutive failures → exactly one nRST reset request via the #109 path, EVR, +counters observable in telemetry. + +### Slice 2.7 — FAULTED latch +M resets without recovery → FAULTED: telemetry flag set; no further radio-interface +calls; `dataIn` buffers returned immediately; `run` ticks are no-ops. Latched — +further errors can't re-trigger resets. + +### Slice 2.8 — Ground recovery +`RESET_RADIO` (existing command) while FAULTED → full re-init through the fake, +FAULTED cleared on success, re-latched if re-init fails. + +### Slice 2.9 — Cherry-pick `s-band-speedup` correctness fixes +BUSY-GPIO wiring into RadioLib (7c473f6/363101e), `dataOut_out` after all SPI ops +(8aeee2f), atomic inter-thread flag (9b240d0), enableRx/enableTx return types +(17df3ec). Where a fix has observable behavior (ordering, flag races), pin it with +a UT first; pure wiring is covered by PR 3 bench work. + +### Slice 2.10 — `RADIOLIB_STATIC_ONLY=1` +Set in SBand's CMake (where RadioLib is added). Gate = build + full UT suite; +grep the map file to confirm no RadioLib heap symbols remain on hot paths. + +**PR 2 exit:** SBand component fully unit-tested against the D3 invariant; zero +topology/flight change. + +--- + +## PR 3 — Topology re-enable (soak-gated) + +### Slice 3.1 — Wire it back +Un-comment instances, `ComCcsdsSband` subtopology, connections, rate-group slots. +New `SBAND_STACK_SIZE = 8 * 1024` for the sband instance only (comment explaining +the pool-fallback mechanism). CI builds for v5c/v5d/v5e; existing integration + +day-in-the-life suites green. + +### Slice 3.2 — HWIL functional pass (bench, v5e) +- Downlink: TM frames received over the S-Band link end-to-end. +- Uplink: command through `ComCcsdsSband.authenticationRouter` → `cmdDisp` executes. +- StackMonitor shows the sband thread running with an 8 KB stack. + +### Slice 3.3 — HWIL fault injection (the #122 repro, now with an expected answer) +Physically disconnect SPI/BUSY mid-operation → expect: bounded EVRs → auto-reset +attempts → FAULTED. **No watchdog reset, no queue overflow, UHF link still up.** +Then `RESET_RADIO` with hardware restored → link recovers. + +### Slice 3.4 — Soak gate (merge gate for PR 3) +≥24 h continuous, both radios active with periodic S-Band TX/RX traffic: +- zero watchdog/unexpected resets; +- sband stack high-water < 70 % of 8 KB (else trigger the option-C fallback review); +- no other thread's watermark regresses vs the PR 1 baseline; +- heap watermark flat after init (STATIC_ONLY doing its job); +- RX/TX counters advance the whole window. + +**Post-merge:** update #122 and #299 with results (and the RadioLib naming +correction); file the follow-up issue for throughput tuning (deferred +`s-band-speedup` perf commits + passthrough work). From 4c339d5f38bddf6b9311ecb96061e98750df1aee Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:24:23 -0700 Subject: [PATCH 2/9] feat(stack-monitor): add StackMonitorCore pure-logic class, TDD Host-testable core for the S-Band reintegration plan's StackMonitor component (PR 1 / Slice 1.1, decision D6): no Zephyr or F Prime autocode dependencies, matching the pattern used by DetumbleManager::BDot. Given a per-tick snapshot of thread stack samples (name, size, free bytes), computes a summary of the thread under the most stack pressure and warn/clear decisions per thread, with hysteresis (a thread warns once on crossing below its threshold and doesn't re-warn until it recovers above it). One RED->GREEN cycle per behavior, in order: summary from a sample set, warning on crossing below threshold, no re-warn while still below, clearing on recovery, then edge cases actually hit (empty sample set, a thread disappearing between ticks). All 6 cases green under make test-unit. Co-Authored-By: Claude Fable 5 --- .../StackMonitor/StackMonitorCore.cpp | 65 +++++++++ .../StackMonitor/StackMonitorCore.hpp | 75 +++++++++++ .../test/unit-tests/CMakeLists.txt | 9 ++ .../unit-tests/test_StackMonitor_Core.cpp | 126 ++++++++++++++++++ 4 files changed, 275 insertions(+) create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp create mode 100644 PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp new file mode 100644 index 00000000..8257a5f1 --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp @@ -0,0 +1,65 @@ +// ====================================================================== +// \title StackMonitorCore.cpp +// \brief cpp file for StackMonitorCore pure-logic class +// ====================================================================== + +#include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp" + +namespace Components { + +namespace { + +//! Free bytes, clamped so a corrupt/odd sample can never read as more free +//! than the thread's total stack size. +std::uint32_t clampedFreeBytes(const ThreadStackSample& sample) { + return (sample.freeBytes > sample.sizeBytes) ? sample.sizeBytes : sample.freeBytes; +} + +//! Percent of the thread's stack currently free, 0-100. +std::uint32_t freePercent(const ThreadStackSample& sample) { + if (sample.sizeBytes == 0) { + return 0; + } + return (clampedFreeBytes(sample) * 100) / sample.sizeBytes; +} + +} // namespace + +StackMonitorCore::StackMonitorCore(std::uint32_t warnThresholdPercent) : m_warnThresholdPercent(warnThresholdPercent) {} + +StackMonitorTickResult StackMonitorCore::tick(const std::vector& samples) { + StackMonitorTickResult result; + + bool haveWorst = false; + std::uint32_t worstFreePercent = 0; + + for (const auto& sample : samples) { + std::uint32_t fPercent = freePercent(sample); + + if (!haveWorst || fPercent < worstFreePercent) { + haveWorst = true; + worstFreePercent = fPercent; + result.summary.worstThreadName = sample.name; + result.summary.worstThreadFreeBytes = sample.freeBytes; + result.summary.worstThreadUsedPercent = 100 - fPercent; + } + + bool isBelowThreshold = fPercent < m_warnThresholdPercent; + if (isBelowThreshold) { + result.summary.threadsBelowThreshold++; + } + + bool wasWarned = m_warned[sample.name]; + if (isBelowThreshold && !wasWarned) { + result.newWarnings.push_back({sample.name, sample.freeBytes, sample.sizeBytes}); + m_warned[sample.name] = true; + } else if (!isBelowThreshold && wasWarned) { + result.newRecoveries.push_back({sample.name}); + m_warned[sample.name] = false; + } + } + + return result; +} + +} // namespace Components diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp new file mode 100644 index 00000000..1af6bd97 --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp @@ -0,0 +1,75 @@ +// ====================================================================== +// \title StackMonitorCore.hpp +// \brief hpp file for StackMonitorCore pure-logic class +// ====================================================================== + +#pragma once + +#include +#include +#include +#include + +namespace Components { + +//! One thread's stack usage as sampled for a single tick. +struct ThreadStackSample { + std::string name; + std::uint32_t sizeBytes; + std::uint32_t freeBytes; +}; + +//! Per-tick summary across all sampled threads. +struct StackMonitorSummary { + std::string worstThreadName; + std::uint32_t worstThreadFreeBytes = 0; + std::uint32_t worstThreadUsedPercent = 0; + std::uint32_t threadsBelowThreshold = 0; +}; + +//! A thread that just crossed below its warn threshold this tick. +struct StackWarning { + std::string name; + std::uint32_t freeBytes; + std::uint32_t sizeBytes; +}; + +//! A thread that just crossed back above its warn threshold this tick. +struct StackRecovery { + std::string name; +}; + +//! Result of processing one tick of thread stack samples. +struct StackMonitorTickResult { + StackMonitorSummary summary; + std::vector newWarnings; + std::vector newRecoveries; +}; + +//! Pure logic core for the StackMonitor component. +//! +//! Given a snapshot of per-thread stack usage taken once per tick, computes a +//! summary of the thread under the most stack pressure and (in a later slice) +//! warn/clear decisions. Host-compilable: no Zephyr or F Prime autocode +//! dependencies, matching the pattern used by DetumbleManager::BDot. +class StackMonitorCore { + public: + //! \param warnThresholdPercent warn when a thread's free stack drops + //! below this percent of its own size. + explicit StackMonitorCore(std::uint32_t warnThresholdPercent); + + //! Process one tick's worth of thread stack samples. + StackMonitorTickResult tick(const std::vector& samples); + + private: + std::uint32_t m_warnThresholdPercent; + + //! Latched warn state per thread name; true while a thread is below + //! threshold and hasn't yet recovered. Kept across ticks so a thread + //! that stays below threshold isn't re-warned every tick, and a thread + //! that disappears from the sample set simply stops being updated + //! (no assert, no crash) until it reappears. + std::unordered_map m_warned; +}; + +} // namespace Components diff --git a/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt b/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt index 0a047550..4302a604 100644 --- a/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt +++ b/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt @@ -39,6 +39,14 @@ target_include_directories(detumble_manager_bdot PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/../../.. ) +# StackMonitor Core +add_library(stack_monitor_core STATIC + ${CMAKE_CURRENT_SOURCE_DIR}/../../../PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp +) +target_include_directories(stack_monitor_core PUBLIC + ${CMAKE_CURRENT_SOURCE_DIR}/../../.. +) + # --- Auto-discover and build tests --- file(GLOB TEST_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/test_*.cpp") @@ -53,6 +61,7 @@ foreach(test_src ${TEST_SOURCES}) detumble_manager_strategy_selector detumble_manager_bdot rtc_manager_rtc_helper + stack_monitor_core ) add_test(NAME ${test_name} COMMAND ${test_name}) diff --git a/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp b/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp new file mode 100644 index 00000000..dd23f470 --- /dev/null +++ b/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp @@ -0,0 +1,126 @@ +#include + +#include + +#include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp" + +using Components::StackMonitorCore; +using Components::ThreadStackSample; + +// Warn when a thread's free stack drops below 20% of its own size. +const std::uint32_t WARN_THRESHOLD_PERCENT = 20; + +TEST(StackMonitorCoreTest, SummarizesWorstThreadFromSampleSet) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + std::vector samples = { + {"idle", 1024, 900}, // 12% used, healthy + {"sband", 4096, 1024}, // 75% used, worst of the three + {"watchdog", 2048, 1800}, // ~12% used, healthy + }; + + auto result = core.tick(samples); + + EXPECT_EQ(result.summary.worstThreadName, "sband"); + EXPECT_EQ(result.summary.worstThreadFreeBytes, 1024u); + EXPECT_EQ(result.summary.worstThreadUsedPercent, 75u); + EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); +} + +TEST(StackMonitorCoreTest, WarnsWhenThreadCrossesBelowThreshold) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + // sband: 4096 total, 700 free => ~17% free, below the 20% threshold. + std::vector samples = { + {"sband", 4096, 700}, + }; + + auto result = core.tick(samples); + + EXPECT_EQ(result.summary.threadsBelowThreshold, 1u); + ASSERT_EQ(result.newWarnings.size(), 1u); + EXPECT_EQ(result.newWarnings[0].name, "sband"); + EXPECT_EQ(result.newWarnings[0].freeBytes, 700u); + EXPECT_EQ(result.newWarnings[0].sizeBytes, 4096u); + EXPECT_TRUE(result.newRecoveries.empty()); +} + +TEST(StackMonitorCoreTest, DoesNotReWarnWhileStillBelowThreshold) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + std::vector samples = { + {"sband", 4096, 700}, // below threshold + }; + + auto first = core.tick(samples); + ASSERT_EQ(first.newWarnings.size(), 1u); + + // Still below threshold on the next tick: no new warning (hysteresis). + auto second = core.tick(samples); + EXPECT_TRUE(second.newWarnings.empty()); + EXPECT_EQ(second.summary.threadsBelowThreshold, 1u); +} + +TEST(StackMonitorCoreTest, ClearsWhenThreadRecoversAboveThreshold) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + std::vector low = { + {"sband", 4096, 700}, // ~17% free, below threshold + }; + auto warned = core.tick(low); + ASSERT_EQ(warned.newWarnings.size(), 1u); + + std::vector recovered = { + {"sband", 4096, 2048}, // 50% free, back above threshold + }; + auto result = core.tick(recovered); + + EXPECT_TRUE(result.newWarnings.empty()); + ASSERT_EQ(result.newRecoveries.size(), 1u); + EXPECT_EQ(result.newRecoveries[0].name, "sband"); + EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); + + // Crossing below threshold again after recovery re-arms the warning. + auto rewarned = core.tick(low); + ASSERT_EQ(rewarned.newWarnings.size(), 1u); + EXPECT_EQ(rewarned.newWarnings[0].name, "sband"); +} + +TEST(StackMonitorCoreTest, EmptySampleSetProducesNoCrashAndNeutralSummary) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + auto result = core.tick({}); + + EXPECT_EQ(result.summary.worstThreadName, ""); + EXPECT_EQ(result.summary.worstThreadFreeBytes, 0u); + EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); + EXPECT_TRUE(result.newWarnings.empty()); + EXPECT_TRUE(result.newRecoveries.empty()); +} + +TEST(StackMonitorCoreTest, ThreadDisappearingBetweenTicksDoesNotAssert) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + std::vector withSband = { + {"sband", 4096, 700}, // below threshold, warns + {"idle", 1024, 900}, + }; + auto first = core.tick(withSband); + ASSERT_EQ(first.newWarnings.size(), 1u); + + // sband thread has exited; only idle remains this tick. + std::vector withoutSband = { + {"idle", 1024, 900}, + }; + auto second = core.tick(withoutSband); + + EXPECT_TRUE(second.newWarnings.empty()); + EXPECT_TRUE(second.newRecoveries.empty()); + EXPECT_EQ(second.summary.worstThreadName, "idle"); + EXPECT_EQ(second.summary.threadsBelowThreshold, 0u); + + // sband reappears still below threshold: no re-warn (hysteresis survives the gap). + auto third = core.tick(withSband); + EXPECT_TRUE(third.newWarnings.empty()); + EXPECT_EQ(third.summary.threadsBelowThreshold, 1u); +} From ecae2c58a0958fec9c37c68f4796c0f6e8784827 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:24:37 -0700 Subject: [PATCH 3/9] feat(stack-monitor): add StackMonitor F Prime component Thin passive component wrapping StackMonitorCore for the S-Band reintegration plan (PR 1 / Slice 1.1, decision D6): once per run tick, walks live Zephyr threads (k_thread_foreach + k_thread_stack_space_get, name via k_thread_name_get guarded by CONFIG_THREAD_NAME) and feeds the samples to the core. All Zephyr calls are isolated in StackMonitor.cpp so the core stays pure and host-testable. Telemetry: MinFreeBytes, WorstThread, ThreadsBelowThreshold. Events: StackLow (WARNING_HI), StackRecovered (ACTIVITY_HI). Not yet wired into the topology in this commit (see the next commit). Co-Authored-By: Claude Fable 5 --- .../Components/CMakeLists.txt | 1 + .../Components/StackMonitor/CMakeLists.txt | 25 ++++++ .../Components/StackMonitor/StackMonitor.cpp | 77 +++++++++++++++++++ .../Components/StackMonitor/StackMonitor.fpp | 62 +++++++++++++++ .../Components/StackMonitor/StackMonitor.hpp | 50 ++++++++++++ .../Components/StackMonitor/docs/sdd.md | 34 ++++++++ 6 files changed, 249 insertions(+) create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/CMakeLists.txt create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp create mode 100644 PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md diff --git a/PROVESFlightControllerReference/Components/CMakeLists.txt b/PROVESFlightControllerReference/Components/CMakeLists.txt index 8c7565f1..f2ddc660 100644 --- a/PROVESFlightControllerReference/Components/CMakeLists.txt +++ b/PROVESFlightControllerReference/Components/CMakeLists.txt @@ -23,6 +23,7 @@ add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/PayloadCom/") add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/PowerMonitor/") add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/ResetManager/") #add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/SBand/") +add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/StackMonitor/") add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/StartupManager/") add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/ThermalManager/") add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Watchdog") diff --git a/PROVESFlightControllerReference/Components/StackMonitor/CMakeLists.txt b/PROVESFlightControllerReference/Components/StackMonitor/CMakeLists.txt new file mode 100644 index 00000000..a4476451 --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/CMakeLists.txt @@ -0,0 +1,25 @@ +#### +# F Prime CMakeLists.txt: +# +# SOURCES: list of source files (to be compiled) +# AUTOCODER_INPUTS: list of files to be passed to the autocoders +# DEPENDS: list of libraries that this module depends on +# +# More information in the F´ CMake API documentation: +# https://fprime.jpl.nasa.gov/latest/docs/reference/api/cmake/API/ +# +#### + +# Module names are derived from the path from the nearest project/library/framework +# root when not specifically overridden by the developer. i.e. The module defined by +# `Ref/SignalGen/CMakeLists.txt` will be named `Ref_SignalGen`. + +register_fprime_library( + AUTOCODER_INPUTS + "${CMAKE_CURRENT_LIST_DIR}/StackMonitor.fpp" + SOURCES + "${CMAKE_CURRENT_LIST_DIR}/StackMonitor.cpp" + "${CMAKE_CURRENT_LIST_DIR}/StackMonitorCore.cpp" +# DEPENDS +# MyPackage_MyOtherModule +) diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp new file mode 100644 index 00000000..0c23e4c4 --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp @@ -0,0 +1,77 @@ +// ====================================================================== +// \title StackMonitor.cpp +// \brief cpp file for StackMonitor component implementation class +// ====================================================================== + +#include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp" + +#include + +namespace Components { + +namespace { + +//! k_thread_foreach callback: append this thread's stack usage to the +//! std::vector pointed to by userData. Keeps all Zephyr +//! calls (and their quirks) out of StackMonitorCore, which stays pure. +void appendThreadSample(const struct k_thread* thread, void* userData) { + auto* samples = static_cast*>(userData); + + // thread is logically read-only here, but the Zephyr stack-space API + // takes a non-const k_tid_t; the callback signature is fixed by + // k_thread_foreach and always hands us a live, non-const thread object. + k_tid_t tid = const_cast(thread); + + std::size_t unusedBytes = 0; + if (k_thread_stack_space_get(thread, &unusedBytes) != 0) { + // Couldn't read this thread's stack info (e.g. mid-teardown); skip + // it rather than report a bogus sample. The next tick will pick it + // back up if it's still alive. + return; + } + + ThreadStackSample sample; +#if defined(CONFIG_THREAD_NAME) + const char* name = k_thread_name_get(tid); + sample.name = (name != nullptr) ? name : ""; +#endif + sample.sizeBytes = static_cast(thread->stack_info.size); + sample.freeBytes = static_cast(unusedBytes); + + samples->push_back(sample); +} + +} // namespace + +// ---------------------------------------------------------------------- +// Component construction and destruction +// ---------------------------------------------------------------------- + +StackMonitor ::StackMonitor(const char* const compName) + : StackMonitorComponentBase(compName), m_core(WARN_THRESHOLD_PERCENT) {} + +StackMonitor ::~StackMonitor() {} + +// ---------------------------------------------------------------------- +// Handler implementations for user-defined typed input ports +// ---------------------------------------------------------------------- + +void StackMonitor ::run_handler(FwIndexType portNum, U32 context) { + std::vector samples; + k_thread_foreach(&appendThreadSample, &samples); + + StackMonitorTickResult result = this->m_core.tick(samples); + + this->tlmWrite_MinFreeBytes(result.summary.worstThreadFreeBytes); + this->tlmWrite_WorstThread(Fw::TlmString(result.summary.worstThreadName.c_str())); + this->tlmWrite_ThreadsBelowThreshold(result.summary.threadsBelowThreshold); + + for (const auto& warning : result.newWarnings) { + this->log_WARNING_HI_StackLow(Fw::LogStringArg(warning.name.c_str()), warning.freeBytes, warning.sizeBytes); + } + for (const auto& recovery : result.newRecoveries) { + this->log_ACTIVITY_HI_StackRecovered(Fw::LogStringArg(recovery.name.c_str())); + } +} + +} // namespace Components diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp new file mode 100644 index 00000000..bcfa572d --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp @@ -0,0 +1,62 @@ +module Components { + @ Program-wide stack-usage watchdog. Walks all live Zephyr threads once per + @ tick (via ThreadInfoProviderIf) and reports per-thread stack high-water + @ telemetry, warning when any thread's free stack drops below a configurable + @ percent of its own size and clearing on recovery. + @ S-Band reintegration plan, PR 1 / Slice 1.1 (D6). + passive component StackMonitor { + + @ Port receiving calls from the rate group + sync input port run: Svc.Sched + + @ Free bytes remaining on the thread under the most stack pressure this tick + telemetry MinFreeBytes: U32 + + @ Name of the thread under the most stack pressure this tick + telemetry WorstThread: string size 32 + + @ Count of threads currently below the warn threshold + telemetry ThreadsBelowThreshold: U32 + + @ Event logged when a thread's free stack drops below its warn threshold + event StackLow( + thread: string size 32 @< Name of the thread + freeBytes: U32 @< Free bytes remaining on the thread's stack + sizeBytes: U32 @< Total size of the thread's stack + ) \ + severity warning high \ + format "Thread {} stack low: {} of {} bytes free" + + @ Event logged when a previously-low thread recovers above its warn threshold + event StackRecovered( + thread: string size 32 @< Name of the thread + ) \ + severity activity high \ + format "Thread {} stack recovered" + + ############################################################################### + # Standard AC Ports: Required for Channels, Events, Commands, and Parameters # + ############################################################################### + @ Port for requesting the current time + time get port timeCaller + + @ Port for sending command registrations + command reg port cmdRegOut + + @ Port for receiving commands + command recv port cmdIn + + @ Port for sending command responses + command resp port cmdResponseOut + + @ Port for sending textual representation of events + text event port logTextOut + + @ Port for sending events to downlink + event port logOut + + @ Port for sending telemetry channels to downlink + telemetry port tlmOut + + } +} diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp new file mode 100644 index 00000000..7854a6f3 --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp @@ -0,0 +1,50 @@ +// ====================================================================== +// \title StackMonitor.hpp +// \brief hpp file for StackMonitor component implementation class +// ====================================================================== + +#ifndef Components_StackMonitor_HPP +#define Components_StackMonitor_HPP + +#include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorComponentAc.hpp" +#include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp" + +namespace Components { + +class StackMonitor final : public StackMonitorComponentBase { + public: + // ---------------------------------------------------------------------- + // Component construction and destruction + // ---------------------------------------------------------------------- + + //! Construct StackMonitor object + StackMonitor(const char* const compName //!< The component name + ); + + //! Destroy StackMonitor object + ~StackMonitor(); + + private: + // ---------------------------------------------------------------------- + // Handler implementations for user-defined typed input ports + // ---------------------------------------------------------------------- + + //! Handler implementation for run + //! + //! Port receiving calls from the rate group + void run_handler(FwIndexType portNum, //!< The port number + U32 context //!< The call order + ) override; + + //! Warn when a thread's free stack drops below this percent of its own size. + static constexpr std::uint32_t WARN_THRESHOLD_PERCENT = 20; + + //! Pure-logic core: turns a snapshot of per-thread stack usage into a + //! summary and warn/clear decisions. Host-testable in isolation; see + //! test/unit-tests/test_StackMonitor_Core.cpp. + StackMonitorCore m_core; +}; + +} // namespace Components + +#endif diff --git a/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md b/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md new file mode 100644 index 00000000..f5b5d3ad --- /dev/null +++ b/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md @@ -0,0 +1,34 @@ +# Components::StackMonitor + +Program-wide stack-usage watchdog. Once per tick, walks every live Zephyr +thread (`k_thread_foreach`) and reports per-thread stack high-water telemetry, +warning when any thread's free stack drops below a configurable percent of its +own size and clearing on recovery. + +Part of the S-Band reintegration plan, PR 1 / Slice 1.1 (decision D6): a +prerequisite for safely re-enabling the S-Band radio thread with a larger, +dedicated stack, by making per-thread stack pressure observable in YAMCS. + +The stack-usage decision logic lives in `StackMonitorCore`, a pure C++ class +with no Zephyr or F Prime dependencies, so it is host-testable in isolation +(see `test/unit-tests/test_StackMonitor_Core.cpp`). `StackMonitor.cpp` is a +thin adapter: it walks Zephyr threads, builds a sample list, and feeds it to +the core each tick. + +## Telemetry +| Name | Description | +|---|---| +| MinFreeBytes | Free bytes remaining on the thread under the most stack pressure this tick | +| WorstThread | Name of the thread under the most stack pressure this tick | +| ThreadsBelowThreshold | Count of threads currently below the warn threshold | + +## Events +| Name | Description | +|---|---| +| StackLow | A thread's free stack dropped below its warn threshold | +| StackRecovered | A previously-low thread recovered above its warn threshold | + +## Change Log +| Date | Description | +|---|---| +|---| Initial Draft | From 8ac1d58a511186e6d60a52e507f3a1141a17996f Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:24:44 -0700 Subject: [PATCH 4/9] feat(topology): wire StackMonitor into the 1Hz rate group Adds the stackMonitor passive instance (base id 0x1007A000, next free slot after picoTempManager) and connects it to the previously-unused rateGroup1Hz.RateGroupMemberOut[12], following the existing base-id and rate-group wiring conventions. Zero behavior change for any existing component. Co-Authored-By: Claude Fable 5 --- .../ReferenceDeployment/Top/instances.fpp | 2 ++ .../ReferenceDeployment/Top/topology.fpp | 2 ++ 2 files changed, 4 insertions(+) diff --git a/PROVESFlightControllerReference/ReferenceDeployment/Top/instances.fpp b/PROVESFlightControllerReference/ReferenceDeployment/Top/instances.fpp index c1a541eb..7fbfddd8 100644 --- a/PROVESFlightControllerReference/ReferenceDeployment/Top/instances.fpp +++ b/PROVESFlightControllerReference/ReferenceDeployment/Top/instances.fpp @@ -242,4 +242,6 @@ module ReferenceDeployment { instance picoTempManager: Drv.PicoTempManager base id 0x10079000 + instance stackMonitor: Components.StackMonitor base id 0x1007A000 + } diff --git a/PROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpp b/PROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpp index d7ab78d9..0e475adb 100644 --- a/PROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpp +++ b/PROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpp @@ -117,6 +117,7 @@ module ReferenceDeployment { instance dropDetector instance picoTempManager + instance stackMonitor # ---------------------------------------------------------------------- # Pattern graph specifiers @@ -284,6 +285,7 @@ module ReferenceDeployment { rateGroup1Hz.RateGroupMemberOut[17] -> adcs.run rateGroup1Hz.RateGroupMemberOut[18] -> thermalManager.run rateGroup1Hz.RateGroupMemberOut[19] -> ComCcsdsLora.authenticationRouter.run + rateGroup1Hz.RateGroupMemberOut[12] -> stackMonitor.run } From d305746d438b8851be77abbbb5f1e12a3bb5f362 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:24:54 -0700 Subject: [PATCH 5/9] chore(kconfig): enable dynamic thread alloc fallback + thread monitor S-Band reintegration plan (PR 1 / Slice 1.3): - CONFIG_DYNAMIC_THREAD_ALLOC=y lets a single oversized thread-stack request fall back to a boot-time k_malloc() instead of failing, needed for the SBand thread's future dedicated 8 KB stack (PR 3). CONFIG_DYNAMIC_THREAD_PREFER_POOL=y is unchanged, so every other thread (<=4096 bytes) still gets a pool stack exactly as before. - CONFIG_THREAD_MONITOR=y and CONFIG_THREAD_NAME=y, needed by the new StackMonitor component for k_thread_foreach and per-thread names. Neither was previously set anywhere in the tree. Config-only change; no new tests. SBand stays commented out, so this is a no-op for current flight behavior. Co-Authored-By: Claude Fable 5 --- prj.conf | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/prj.conf b/prj.conf index a375729a..4082975f 100644 --- a/prj.conf +++ b/prj.conf @@ -43,7 +43,12 @@ CONFIG_I2C_TCA954X_CHANNEL_INIT_PRIO=71 CONFIG_DYNAMIC_THREAD=y CONFIG_KERNEL_MEM_POOL=y -CONFIG_DYNAMIC_THREAD_ALLOC=n +# S-Band reintegration plan (PR 1, Slice 1.3): the SBand thread needs a dedicated +# 8 KB stack (see PR 3), which is larger than the shared 4096-byte pool slot size. +# CONFIG_DYNAMIC_THREAD_ALLOC=y lets a single oversized k_thread_create() request +# fall back to a boot-time k_malloc() instead of failing; CONFIG_DYNAMIC_THREAD_PREFER_POOL=y +# is unchanged, so every other thread (<=4096 bytes) still gets a pool stack as before. +CONFIG_DYNAMIC_THREAD_ALLOC=y CONFIG_DYNAMIC_THREAD_PREFER_POOL=y CONFIG_DYNAMIC_THREAD_POOL_SIZE=25 # Num threads in the thread pool @@ -53,6 +58,12 @@ CONFIG_DYNAMIC_THREAD_STACK_SIZE=4096 CONFIG_THREAD_STACK_INFO=y CONFIG_RING_BUFFER=y +# S-Band reintegration plan (PR 1, Slice 1.1 / D6): needed by the StackMonitor +# component to enumerate live threads (k_thread_foreach) and read their names +# (k_thread_name_get) for per-thread stack telemetry. +CONFIG_THREAD_MONITOR=y +CONFIG_THREAD_NAME=y + CONFIG_COMMON_LIBC_MALLOC=y CONFIG_SENSOR=y From 7886aaab2b307733cc81a20da2b9bae162c19c31 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:30:20 -0700 Subject: [PATCH 6/9] fix(topology): assign StackMonitor telemetry to the Health packet The topology dictionary generator (fpp-to-dict) rejects any telemetry channel that isn't in a packet or explicitly marked omit. Discovered by make build after wiring in stackMonitor: MinFreeBytes, WorstThread, and ThreadsBelowThreshold are system-health telemetry, so add them to the existing Health packet (id 2, group 5) alongside rate-group timing and mode/safe-mode status. Co-Authored-By: Claude Fable 5 --- .../ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi | 3 +++ 1 file changed, 3 insertions(+) diff --git a/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi b/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi index f107909f..7a556ae3 100644 --- a/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi +++ b/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi @@ -130,6 +130,9 @@ telemetry packets ReferenceDeploymentPackets { ReferenceDeployment.modeManager.CurrentMode ReferenceDeployment.modeManager.SafeModeEntryCount ReferenceDeployment.modeManager.CurrentSafeModeReason + ReferenceDeployment.stackMonitor.MinFreeBytes + ReferenceDeployment.stackMonitor.WorstThread + ReferenceDeployment.stackMonitor.ThreadsBelowThreshold } packet HealthWarnings id 3 group 5 { From adfb109042d5a3407b786878918216c4cf2f6111 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:30:29 -0700 Subject: [PATCH 7/9] fix(kconfig): add CONFIG_INIT_STACKS, required by k_thread_stack_space_get Zephyr only compiles z_impl_k_thread_stack_space_get() when both CONFIG_INIT_STACKS and CONFIG_THREAD_STACK_INFO are set (it relies on stacks being pre-filled with a known 0xaa pattern at creation time so the unused portion can be measured later). CONFIG_THREAD_STACK_INFO was already on, but CONFIG_INIT_STACKS was not, which linked but failed at final link time: undefined reference to `z_impl_k_thread_stack_space_get' Caught by make build (full firmware link), not make test-unit (host core tests use a fake sample list, not the real Zephyr call). Co-Authored-By: Claude Fable 5 --- prj.conf | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/prj.conf b/prj.conf index 4082975f..bcce1385 100644 --- a/prj.conf +++ b/prj.conf @@ -63,6 +63,11 @@ CONFIG_RING_BUFFER=y # (k_thread_name_get) for per-thread stack telemetry. CONFIG_THREAD_MONITOR=y CONFIG_THREAD_NAME=y +# k_thread_stack_space_get() (used to read each thread's free-stack high-water +# mark) is only compiled in when CONFIG_INIT_STACKS is also set: it relies on +# stacks being pre-filled with a known 0xaa pattern at creation time so the +# unused portion can be measured later. +CONFIG_INIT_STACKS=y CONFIG_COMMON_LIBC_MALLOC=y From 24354b83342ca0eeb64593a3b837b78846d010e6 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:43:49 -0700 Subject: [PATCH 8/9] fix(stack-monitor): remove all heap allocation from the monitoring path Design-review finding: the k_thread_foreach callback called std::vector::push_back, and Zephyr runs that callback with the thread-monitor spinlock held and IRQs locked -- an allocation that reallocs under a spinlock is a deadlock/latency hazard. Beyond that, the core's std::string/std::vector/std::unordered_map state allocated and freed heap on every 1 Hz tick, the same in-flight heap-churn defect class that got the S-Band radio removed (issue #122). Rework to fixed-capacity POD storage throughout: - ThreadStackSample: char name[32] (matches Zephyr's default CONFIG_THREAD_MAX_NAME_LEN) + integer sizes. - ThreadStackSampleSet: caller-owned fixed array of 32 samples (25-slot pool + statics + margin) with count. add() does only a bounds check, a bounded strncpy, and integer stores -- safe under the spinlock. When full, extras are dropped and an overflowed flag is set (no silent truncation), surfaced as the new SampleOverflow telemetry channel (added to the Health packet). - StackMonitorCore: results written into a caller-provided fixed-size StackMonitorTickResult; the unordered_map hysteresis state replaced by a fixed 32-entry table keyed by the fixed-size name (linear scan at 1 Hz). All member state fixed-size at construction; zero heap after boot. - StackMonitor component: sample set and tick result moved to member state (together a few KB -- too much for run_handler locals on the 4 KB rate-group thread). Same TDD discipline: the 6 existing behaviors were ported to the new interface one at a time and kept green; the overflow flag was added as a genuine RED->GREEN cycle (MoreThreadsThanCapacitySetsOverflowFlag). 7/7 core tests green; full v5e firmware build and lint clean. Co-Authored-By: Claude Fable 5 --- .../Components/StackMonitor/StackMonitor.cpp | 52 +++--- .../Components/StackMonitor/StackMonitor.fpp | 9 +- .../Components/StackMonitor/StackMonitor.hpp | 7 + .../StackMonitor/StackMonitorCore.cpp | 100 +++++++++-- .../StackMonitor/StackMonitorCore.hpp | 95 +++++++--- .../Components/StackMonitor/docs/sdd.md | 10 ++ .../Top/ReferenceDeploymentPackets.fppi | 1 + .../unit-tests/test_StackMonitor_Core.cpp | 167 +++++++++++------- 8 files changed, 319 insertions(+), 122 deletions(-) diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp index 0c23e4c4..9fab981e 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp @@ -12,15 +12,14 @@ namespace Components { namespace { //! k_thread_foreach callback: append this thread's stack usage to the -//! std::vector pointed to by userData. Keeps all Zephyr -//! calls (and their quirks) out of StackMonitorCore, which stays pure. +//! ThreadStackSampleSet pointed to by userData. Zephyr runs this callback +//! with the thread-monitor spinlock held and IRQs locked, so it must not +//! allocate or block: it only reads the stack high-water mark, does a +//! bounded string copy of the name, and stores integers into a fixed slot +//! (ThreadStackSampleSet::add). Keeps all Zephyr calls out of +//! StackMonitorCore, which stays pure. void appendThreadSample(const struct k_thread* thread, void* userData) { - auto* samples = static_cast*>(userData); - - // thread is logically read-only here, but the Zephyr stack-space API - // takes a non-const k_tid_t; the callback signature is fixed by - // k_thread_foreach and always hands us a live, non-const thread object. - k_tid_t tid = const_cast(thread); + auto* samples = static_cast(userData); std::size_t unusedBytes = 0; if (k_thread_stack_space_get(thread, &unusedBytes) != 0) { @@ -30,15 +29,16 @@ void appendThreadSample(const struct k_thread* thread, void* userData) { return; } - ThreadStackSample sample; + const char* name = ""; #if defined(CONFIG_THREAD_NAME) - const char* name = k_thread_name_get(tid); - sample.name = (name != nullptr) ? name : ""; + // thread is logically read-only here, but k_thread_name_get takes a + // non-const k_tid_t; the callback signature is fixed by + // k_thread_foreach and always hands us a live thread object. + name = k_thread_name_get(const_cast(thread)); #endif - sample.sizeBytes = static_cast(thread->stack_info.size); - sample.freeBytes = static_cast(unusedBytes); - samples->push_back(sample); + (void)samples->add(name, static_cast(thread->stack_info.size), + static_cast(unusedBytes)); } } // namespace @@ -48,7 +48,7 @@ void appendThreadSample(const struct k_thread* thread, void* userData) { // ---------------------------------------------------------------------- StackMonitor ::StackMonitor(const char* const compName) - : StackMonitorComponentBase(compName), m_core(WARN_THRESHOLD_PERCENT) {} + : StackMonitorComponentBase(compName), m_core(WARN_THRESHOLD_PERCENT), m_samples(), m_result() {} StackMonitor ::~StackMonitor() {} @@ -57,20 +57,22 @@ StackMonitor ::~StackMonitor() {} // ---------------------------------------------------------------------- void StackMonitor ::run_handler(FwIndexType portNum, U32 context) { - std::vector samples; - k_thread_foreach(&appendThreadSample, &samples); + this->m_samples.clear(); + k_thread_foreach(&appendThreadSample, &this->m_samples); - StackMonitorTickResult result = this->m_core.tick(samples); + this->m_core.tick(this->m_samples, this->m_result); - this->tlmWrite_MinFreeBytes(result.summary.worstThreadFreeBytes); - this->tlmWrite_WorstThread(Fw::TlmString(result.summary.worstThreadName.c_str())); - this->tlmWrite_ThreadsBelowThreshold(result.summary.threadsBelowThreshold); + this->tlmWrite_MinFreeBytes(this->m_result.summary.worstThreadFreeBytes); + this->tlmWrite_WorstThread(Fw::TlmString(this->m_result.summary.worstThreadName)); + this->tlmWrite_ThreadsBelowThreshold(this->m_result.summary.threadsBelowThreshold); + this->tlmWrite_SampleOverflow(this->m_result.summary.overflowed); - for (const auto& warning : result.newWarnings) { - this->log_WARNING_HI_StackLow(Fw::LogStringArg(warning.name.c_str()), warning.freeBytes, warning.sizeBytes); + for (std::uint32_t i = 0; i < this->m_result.warningCount; i++) { + const StackWarning& warning = this->m_result.newWarnings[i]; + this->log_WARNING_HI_StackLow(Fw::LogStringArg(warning.name), warning.freeBytes, warning.sizeBytes); } - for (const auto& recovery : result.newRecoveries) { - this->log_ACTIVITY_HI_StackRecovered(Fw::LogStringArg(recovery.name.c_str())); + for (std::uint32_t i = 0; i < this->m_result.recoveryCount; i++) { + this->log_ACTIVITY_HI_StackRecovered(Fw::LogStringArg(this->m_result.newRecoveries[i].name)); } } diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp index bcfa572d..8b68e608 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fpp @@ -1,8 +1,9 @@ module Components { @ Program-wide stack-usage watchdog. Walks all live Zephyr threads once per - @ tick (via ThreadInfoProviderIf) and reports per-thread stack high-water + @ tick (k_thread_foreach) and reports per-thread stack high-water @ telemetry, warning when any thread's free stack drops below a configurable - @ percent of its own size and clearing on recovery. + @ percent of its own size and clearing on recovery. Fixed-capacity storage + @ throughout: zero heap allocation after construction. @ S-Band reintegration plan, PR 1 / Slice 1.1 (D6). passive component StackMonitor { @@ -18,6 +19,10 @@ module Components { @ Count of threads currently below the warn threshold telemetry ThreadsBelowThreshold: U32 + @ True when a tick saw more live threads than the monitor's fixed + @ capacity (extra threads went unsampled that tick -- not silent) + telemetry SampleOverflow: bool + @ Event logged when a thread's free stack drops below its warn threshold event StackLow( thread: string size 32 @< Name of the thread diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp index 7854a6f3..73e6bd4c 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hpp @@ -43,6 +43,13 @@ class StackMonitor final : public StackMonitorComponentBase { //! summary and warn/clear decisions. Host-testable in isolation; see //! test/unit-tests/test_StackMonitor_Core.cpp. StackMonitorCore m_core; + + //! Per-tick sample and result storage. Fixed-size and kept as member + //! state (not run_handler locals): together they are a few KB, which + //! would not be comfortable on the 4 KB rate-group thread stack, and + //! keeping them here guarantees zero heap allocation after boot. + ThreadStackSampleSet m_samples; + StackMonitorTickResult m_result; }; } // namespace Components diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp index 8257a5f1..60b5ad5e 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cpp @@ -5,10 +5,19 @@ #include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp" +#include + namespace Components { namespace { +//! Bounded copy of a thread name into a fixed-size slot, always +//! null-terminated. No allocation. +void copyName(char (&dst)[STACK_MONITOR_MAX_NAME_LEN], const char* src) { + (void)std::strncpy(dst, (src != nullptr) ? src : "", STACK_MONITOR_MAX_NAME_LEN - 1); + dst[STACK_MONITOR_MAX_NAME_LEN - 1] = '\0'; +} + //! Free bytes, clamped so a corrupt/odd sample can never read as more free //! than the thread's total stack size. std::uint32_t clampedFreeBytes(const ThreadStackSample& sample) { @@ -25,21 +34,52 @@ std::uint32_t freePercent(const ThreadStackSample& sample) { } // namespace -StackMonitorCore::StackMonitorCore(std::uint32_t warnThresholdPercent) : m_warnThresholdPercent(warnThresholdPercent) {} +// ---------------------------------------------------------------------- +// ThreadStackSampleSet +// ---------------------------------------------------------------------- -StackMonitorTickResult StackMonitorCore::tick(const std::vector& samples) { - StackMonitorTickResult result; +void ThreadStackSampleSet::clear() { + this->count = 0; + this->overflowed = false; +} + +bool ThreadStackSampleSet::add(const char* name, std::uint32_t sizeBytes, std::uint32_t freeBytes) { + if (this->count >= STACK_MONITOR_MAX_THREADS) { + this->overflowed = true; + return false; + } + ThreadStackSample& slot = this->samples[this->count]; + copyName(slot.name, name); + slot.sizeBytes = sizeBytes; + slot.freeBytes = freeBytes; + this->count++; + return true; +} + +// ---------------------------------------------------------------------- +// StackMonitorCore +// ---------------------------------------------------------------------- + +StackMonitorCore::StackMonitorCore(std::uint32_t warnThresholdPercent) + : m_warnThresholdPercent(warnThresholdPercent), m_warned() {} + +void StackMonitorCore::tick(const ThreadStackSampleSet& sampleSet, StackMonitorTickResult& result) { + result.summary = StackMonitorSummary(); + result.summary.overflowed = sampleSet.overflowed; + result.warningCount = 0; + result.recoveryCount = 0; bool haveWorst = false; std::uint32_t worstFreePercent = 0; - for (const auto& sample : samples) { + for (std::uint32_t i = 0; i < sampleSet.count; i++) { + const ThreadStackSample& sample = sampleSet.samples[i]; std::uint32_t fPercent = freePercent(sample); if (!haveWorst || fPercent < worstFreePercent) { haveWorst = true; worstFreePercent = fPercent; - result.summary.worstThreadName = sample.name; + copyName(result.summary.worstThreadName, sample.name); result.summary.worstThreadFreeBytes = sample.freeBytes; result.summary.worstThreadUsedPercent = 100 - fPercent; } @@ -49,17 +89,55 @@ StackMonitorTickResult StackMonitorCore::tick(const std::vectorfindWarned(sample.name) >= 0); if (isBelowThreshold && !wasWarned) { - result.newWarnings.push_back({sample.name, sample.freeBytes, sample.sizeBytes}); - m_warned[sample.name] = true; + if (result.warningCount < STACK_MONITOR_MAX_THREADS) { + StackWarning& warning = result.newWarnings[result.warningCount]; + copyName(warning.name, sample.name); + warning.freeBytes = sample.freeBytes; + warning.sizeBytes = sample.sizeBytes; + result.warningCount++; + } + this->setWarned(sample.name); } else if (!isBelowThreshold && wasWarned) { - result.newRecoveries.push_back({sample.name}); - m_warned[sample.name] = false; + if (result.recoveryCount < STACK_MONITOR_MAX_THREADS) { + copyName(result.newRecoveries[result.recoveryCount].name, sample.name); + result.recoveryCount++; + } + this->clearWarned(sample.name); } } +} - return result; +std::int32_t StackMonitorCore::findWarned(const char* name) const { + for (std::uint32_t i = 0; i < STACK_MONITOR_MAX_THREADS; i++) { + if (this->m_warned[i].used && (std::strncmp(this->m_warned[i].name, name, STACK_MONITOR_MAX_NAME_LEN) == 0)) { + return static_cast(i); + } + } + return -1; +} + +void StackMonitorCore::setWarned(const char* name) { + if (this->findWarned(name) >= 0) { + return; + } + for (std::uint32_t i = 0; i < STACK_MONITOR_MAX_THREADS; i++) { + if (!this->m_warned[i].used) { + copyName(this->m_warned[i].name, name); + this->m_warned[i].used = true; + return; + } + } + // Table full: the warning event is still emitted by tick(); this + // thread just isn't latched (it may re-warn on a later tick). +} + +void StackMonitorCore::clearWarned(const char* name) { + std::int32_t index = this->findWarned(name); + if (index >= 0) { + this->m_warned[index].used = false; + } } } // namespace Components diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp index 1af6bd97..e0f82251 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp @@ -6,70 +6,119 @@ #pragma once #include -#include -#include -#include namespace Components { -//! One thread's stack usage as sampled for a single tick. +//! Fixed capacities: no heap allocation anywhere in the stack-monitoring +//! path (per-tick heap churn is not flight-safe; see issue #122 and the +//! S-Band reintegration plan). MAX_NAME_LEN matches Zephyr's default +//! CONFIG_THREAD_MAX_NAME_LEN; MAX_THREADS covers the 25-slot dynamic +//! thread pool plus static threads with margin. +static constexpr std::uint32_t STACK_MONITOR_MAX_NAME_LEN = 32; +static constexpr std::uint32_t STACK_MONITOR_MAX_THREADS = 32; + +//! One thread's stack usage as sampled for a single tick. POD with a +//! fixed-size name buffer: safe to fill inside k_thread_foreach's +//! spinlocked callback (integer stores + bounded string copy only). struct ThreadStackSample { - std::string name; + char name[STACK_MONITOR_MAX_NAME_LEN]; std::uint32_t sizeBytes; std::uint32_t freeBytes; }; +//! Caller-owned, fixed-capacity set of samples for one tick. +//! add() performs only a bounds check, a bounded string copy, and integer +//! stores -- no allocation of any kind -- so it is safe to call from the +//! k_thread_foreach callback, which Zephyr runs with the thread-monitor +//! spinlock held and IRQs locked. +struct ThreadStackSampleSet { + ThreadStackSample samples[STACK_MONITOR_MAX_THREADS]; + std::uint32_t count = 0; + bool overflowed = false; + + //! Reset to empty (count/overflowed only; sample slots are overwritten on add). + void clear(); + + //! Append a sample. When the set is full the sample is dropped and the + //! overflowed flag is set (no silent truncation); returns false in that case. + bool add(const char* name, std::uint32_t sizeBytes, std::uint32_t freeBytes); +}; + //! Per-tick summary across all sampled threads. struct StackMonitorSummary { - std::string worstThreadName; + char worstThreadName[STACK_MONITOR_MAX_NAME_LEN] = {0}; std::uint32_t worstThreadFreeBytes = 0; std::uint32_t worstThreadUsedPercent = 0; std::uint32_t threadsBelowThreshold = 0; + bool overflowed = false; //!< True when this tick saw more threads than capacity }; //! A thread that just crossed below its warn threshold this tick. struct StackWarning { - std::string name; + char name[STACK_MONITOR_MAX_NAME_LEN]; std::uint32_t freeBytes; std::uint32_t sizeBytes; }; //! A thread that just crossed back above its warn threshold this tick. struct StackRecovery { - std::string name; + char name[STACK_MONITOR_MAX_NAME_LEN]; }; -//! Result of processing one tick of thread stack samples. +//! Result of processing one tick of thread stack samples. Fixed-size; +//! intended to live as long-lived (member) storage, not on a small +//! thread's stack. struct StackMonitorTickResult { StackMonitorSummary summary; - std::vector newWarnings; - std::vector newRecoveries; + StackWarning newWarnings[STACK_MONITOR_MAX_THREADS]; + std::uint32_t warningCount = 0; + StackRecovery newRecoveries[STACK_MONITOR_MAX_THREADS]; + std::uint32_t recoveryCount = 0; }; //! Pure logic core for the StackMonitor component. //! //! Given a snapshot of per-thread stack usage taken once per tick, computes a -//! summary of the thread under the most stack pressure and (in a later slice) -//! warn/clear decisions. Host-compilable: no Zephyr or F Prime autocode -//! dependencies, matching the pattern used by DetumbleManager::BDot. +//! summary of the thread under the most stack pressure and warn/clear +//! decisions per thread with hysteresis. Host-compilable: no Zephyr or +//! F Prime autocode dependencies, matching the pattern used by +//! DetumbleManager::BDot. All state is fixed-size at construction; the +//! class performs zero heap allocation. class StackMonitorCore { public: //! \param warnThresholdPercent warn when a thread's free stack drops //! below this percent of its own size. explicit StackMonitorCore(std::uint32_t warnThresholdPercent); - //! Process one tick's worth of thread stack samples. - StackMonitorTickResult tick(const std::vector& samples); + //! Process one tick's worth of thread stack samples, writing into the + //! caller-provided result (cleared first). No allocation. + void tick(const ThreadStackSampleSet& sampleSet, StackMonitorTickResult& result); private: - std::uint32_t m_warnThresholdPercent; - - //! Latched warn state per thread name; true while a thread is below - //! threshold and hasn't yet recovered. Kept across ticks so a thread + //! Latched warn state, keyed by fixed-size thread name. An entry is + //! occupied while its thread is below threshold and hasn't yet + //! recovered; it is freed on recovery. Kept across ticks so a thread //! that stays below threshold isn't re-warned every tick, and a thread - //! that disappears from the sample set simply stops being updated - //! (no assert, no crash) until it reappears. - std::unordered_map m_warned; + //! that disappears from the sample set simply keeps its entry (no + //! assert, no crash, no re-warn if it reappears still below). + //! Linear scan over <=MAX_THREADS entries at 1 Hz. + struct WarnEntry { + char name[STACK_MONITOR_MAX_NAME_LEN]; + bool used; + }; + + //! Find the warn-table index for a name, or -1 if absent. + std::int32_t findWarned(const char* name) const; + + //! Mark a name warned (no-op if the table is unexpectedly full; the + //! warning event itself is still emitted by tick()). + void setWarned(const char* name); + + //! Clear a name's warned entry, if present. + void clearWarned(const char* name); + + std::uint32_t m_warnThresholdPercent; + WarnEntry m_warned[STACK_MONITOR_MAX_THREADS]; }; } // namespace Components diff --git a/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md b/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md index f5b5d3ad..c90383e2 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md +++ b/PROVESFlightControllerReference/Components/StackMonitor/docs/sdd.md @@ -15,12 +15,22 @@ with no Zephyr or F Prime dependencies, so it is host-testable in isolation thin adapter: it walks Zephyr threads, builds a sample list, and feeds it to the core each tick. +All storage is fixed-capacity POD (32 threads x 32-char names) kept as +component member state: the `k_thread_foreach` callback runs under the kernel +thread-monitor spinlock with IRQs locked, so it performs only a bounds check, +a bounded string copy, and integer stores. Zero heap allocation anywhere in +the monitoring path after boot (per-tick heap churn is the defect class that +got the S-Band radio removed; see issue #122). If a tick sees more live +threads than capacity, the extras are dropped and the `SampleOverflow` +telemetry channel reports it (no silent truncation). + ## Telemetry | Name | Description | |---|---| | MinFreeBytes | Free bytes remaining on the thread under the most stack pressure this tick | | WorstThread | Name of the thread under the most stack pressure this tick | | ThreadsBelowThreshold | Count of threads currently below the warn threshold | +| SampleOverflow | True when a tick saw more live threads than the monitor's fixed capacity | ## Events | Name | Description | diff --git a/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi b/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi index 7a556ae3..eb1de9aa 100644 --- a/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi +++ b/PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi @@ -133,6 +133,7 @@ telemetry packets ReferenceDeploymentPackets { ReferenceDeployment.stackMonitor.MinFreeBytes ReferenceDeployment.stackMonitor.WorstThread ReferenceDeployment.stackMonitor.ThreadsBelowThreshold + ReferenceDeployment.stackMonitor.SampleOverflow } packet HealthWarnings id 3 group 5 { diff --git a/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp b/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp index dd23f470..a1199fbf 100644 --- a/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp +++ b/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp @@ -1,11 +1,11 @@ #include -#include - #include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp" +using Components::STACK_MONITOR_MAX_THREADS; using Components::StackMonitorCore; -using Components::ThreadStackSample; +using Components::StackMonitorTickResult; +using Components::ThreadStackSampleSet; // Warn when a thread's free stack drops below 20% of its own size. const std::uint32_t WARN_THRESHOLD_PERCENT = 20; @@ -13,15 +13,16 @@ const std::uint32_t WARN_THRESHOLD_PERCENT = 20; TEST(StackMonitorCoreTest, SummarizesWorstThreadFromSampleSet) { StackMonitorCore core(WARN_THRESHOLD_PERCENT); - std::vector samples = { - {"idle", 1024, 900}, // 12% used, healthy - {"sband", 4096, 1024}, // 75% used, worst of the three - {"watchdog", 2048, 1800}, // ~12% used, healthy - }; + ThreadStackSampleSet samples; + samples.clear(); + samples.add("idle", 1024, 900); // 12% used, healthy + samples.add("sband", 4096, 1024); // 75% used, worst of the three + samples.add("watchdog", 2048, 1800); // ~12% used, healthy - auto result = core.tick(samples); + StackMonitorTickResult result; + core.tick(samples, result); - EXPECT_EQ(result.summary.worstThreadName, "sband"); + EXPECT_STREQ(result.summary.worstThreadName, "sband"); EXPECT_EQ(result.summary.worstThreadFreeBytes, 1024u); EXPECT_EQ(result.summary.worstThreadUsedPercent, 75u); EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); @@ -31,96 +32,140 @@ TEST(StackMonitorCoreTest, WarnsWhenThreadCrossesBelowThreshold) { StackMonitorCore core(WARN_THRESHOLD_PERCENT); // sband: 4096 total, 700 free => ~17% free, below the 20% threshold. - std::vector samples = { - {"sband", 4096, 700}, - }; + ThreadStackSampleSet samples; + samples.clear(); + samples.add("sband", 4096, 700); - auto result = core.tick(samples); + StackMonitorTickResult result; + core.tick(samples, result); EXPECT_EQ(result.summary.threadsBelowThreshold, 1u); - ASSERT_EQ(result.newWarnings.size(), 1u); - EXPECT_EQ(result.newWarnings[0].name, "sband"); + ASSERT_EQ(result.warningCount, 1u); + EXPECT_STREQ(result.newWarnings[0].name, "sband"); EXPECT_EQ(result.newWarnings[0].freeBytes, 700u); EXPECT_EQ(result.newWarnings[0].sizeBytes, 4096u); - EXPECT_TRUE(result.newRecoveries.empty()); + EXPECT_EQ(result.recoveryCount, 0u); } TEST(StackMonitorCoreTest, DoesNotReWarnWhileStillBelowThreshold) { StackMonitorCore core(WARN_THRESHOLD_PERCENT); - std::vector samples = { - {"sband", 4096, 700}, // below threshold - }; + ThreadStackSampleSet samples; + samples.clear(); + samples.add("sband", 4096, 700); // below threshold - auto first = core.tick(samples); - ASSERT_EQ(first.newWarnings.size(), 1u); + StackMonitorTickResult result; + core.tick(samples, result); + ASSERT_EQ(result.warningCount, 1u); // Still below threshold on the next tick: no new warning (hysteresis). - auto second = core.tick(samples); - EXPECT_TRUE(second.newWarnings.empty()); - EXPECT_EQ(second.summary.threadsBelowThreshold, 1u); + core.tick(samples, result); + EXPECT_EQ(result.warningCount, 0u); + EXPECT_EQ(result.summary.threadsBelowThreshold, 1u); } TEST(StackMonitorCoreTest, ClearsWhenThreadRecoversAboveThreshold) { StackMonitorCore core(WARN_THRESHOLD_PERCENT); - std::vector low = { - {"sband", 4096, 700}, // ~17% free, below threshold - }; - auto warned = core.tick(low); - ASSERT_EQ(warned.newWarnings.size(), 1u); + ThreadStackSampleSet low; + low.clear(); + low.add("sband", 4096, 700); // ~17% free, below threshold + + StackMonitorTickResult result; + core.tick(low, result); + ASSERT_EQ(result.warningCount, 1u); + + ThreadStackSampleSet recovered; + recovered.clear(); + recovered.add("sband", 4096, 2048); // 50% free, back above threshold - std::vector recovered = { - {"sband", 4096, 2048}, // 50% free, back above threshold - }; - auto result = core.tick(recovered); + core.tick(recovered, result); - EXPECT_TRUE(result.newWarnings.empty()); - ASSERT_EQ(result.newRecoveries.size(), 1u); - EXPECT_EQ(result.newRecoveries[0].name, "sband"); + EXPECT_EQ(result.warningCount, 0u); + ASSERT_EQ(result.recoveryCount, 1u); + EXPECT_STREQ(result.newRecoveries[0].name, "sband"); EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); // Crossing below threshold again after recovery re-arms the warning. - auto rewarned = core.tick(low); - ASSERT_EQ(rewarned.newWarnings.size(), 1u); - EXPECT_EQ(rewarned.newWarnings[0].name, "sband"); + core.tick(low, result); + ASSERT_EQ(result.warningCount, 1u); + EXPECT_STREQ(result.newWarnings[0].name, "sband"); } TEST(StackMonitorCoreTest, EmptySampleSetProducesNoCrashAndNeutralSummary) { StackMonitorCore core(WARN_THRESHOLD_PERCENT); - auto result = core.tick({}); + ThreadStackSampleSet samples; + samples.clear(); - EXPECT_EQ(result.summary.worstThreadName, ""); + StackMonitorTickResult result; + core.tick(samples, result); + + EXPECT_STREQ(result.summary.worstThreadName, ""); EXPECT_EQ(result.summary.worstThreadFreeBytes, 0u); EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); - EXPECT_TRUE(result.newWarnings.empty()); - EXPECT_TRUE(result.newRecoveries.empty()); + EXPECT_EQ(result.warningCount, 0u); + EXPECT_EQ(result.recoveryCount, 0u); } TEST(StackMonitorCoreTest, ThreadDisappearingBetweenTicksDoesNotAssert) { StackMonitorCore core(WARN_THRESHOLD_PERCENT); - std::vector withSband = { - {"sband", 4096, 700}, // below threshold, warns - {"idle", 1024, 900}, - }; - auto first = core.tick(withSband); - ASSERT_EQ(first.newWarnings.size(), 1u); + ThreadStackSampleSet withSband; + withSband.clear(); + withSband.add("sband", 4096, 700); // below threshold, warns + withSband.add("idle", 1024, 900); + + StackMonitorTickResult result; + core.tick(withSband, result); + ASSERT_EQ(result.warningCount, 1u); // sband thread has exited; only idle remains this tick. - std::vector withoutSband = { - {"idle", 1024, 900}, - }; - auto second = core.tick(withoutSband); + ThreadStackSampleSet withoutSband; + withoutSband.clear(); + withoutSband.add("idle", 1024, 900); - EXPECT_TRUE(second.newWarnings.empty()); - EXPECT_TRUE(second.newRecoveries.empty()); - EXPECT_EQ(second.summary.worstThreadName, "idle"); - EXPECT_EQ(second.summary.threadsBelowThreshold, 0u); + core.tick(withoutSband, result); + + EXPECT_EQ(result.warningCount, 0u); + EXPECT_EQ(result.recoveryCount, 0u); + EXPECT_STREQ(result.summary.worstThreadName, "idle"); + EXPECT_EQ(result.summary.threadsBelowThreshold, 0u); // sband reappears still below threshold: no re-warn (hysteresis survives the gap). - auto third = core.tick(withSband); - EXPECT_TRUE(third.newWarnings.empty()); - EXPECT_EQ(third.summary.threadsBelowThreshold, 1u); + core.tick(withSband, result); + EXPECT_EQ(result.warningCount, 0u); + EXPECT_EQ(result.summary.threadsBelowThreshold, 1u); +} + +TEST(StackMonitorCoreTest, MoreThreadsThanCapacitySetsOverflowFlag) { + StackMonitorCore core(WARN_THRESHOLD_PERCENT); + + ThreadStackSampleSet samples; + samples.clear(); + + // Fill to capacity: every add succeeds, no overflow yet. + char name[16]; + for (std::uint32_t i = 0; i < STACK_MONITOR_MAX_THREADS; i++) { + (void)snprintf(name, sizeof(name), "thread%u", i); + EXPECT_TRUE(samples.add(name, 4096, 2048)); + } + EXPECT_EQ(samples.count, STACK_MONITOR_MAX_THREADS); + EXPECT_FALSE(samples.overflowed); + + // One past capacity: the sample is dropped and the flag is raised. + EXPECT_FALSE(samples.add("straw", 4096, 2048)); + EXPECT_EQ(samples.count, STACK_MONITOR_MAX_THREADS); + EXPECT_TRUE(samples.overflowed); + + // The overflow is surfaced in the tick summary (no silent truncation). + StackMonitorTickResult result; + core.tick(samples, result); + EXPECT_TRUE(result.summary.overflowed); + + // A subsequent in-capacity tick reports clean again. + samples.clear(); + samples.add("idle", 1024, 900); + core.tick(samples, result); + EXPECT_FALSE(result.summary.overflowed); } From 2558ac371e74574ddf9485adaa349e656484c582 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Sun, 19 Jul 2026 18:11:43 -0700 Subject: [PATCH 9/9] fix(stack-monitor): address CodeRabbit review on PR #448 - Switch to k_thread_foreach_unlocked() so the per-thread stack scan no longer runs under the thread-monitor spinlock with IRQs disabled. - Guard against k_thread_name_get() returning NULL for unnamed threads. - Add missing include for snprintf in the core unit tests. - Correct S-BAND-REINTEGRATION-PLAN.md to match the shipped design: drop the ThreadInfoProviderIf seam that was never built, list CONFIG_INIT_STACKS alongside the other required Kconfig options, stop promising per-thread telemetry the component doesn't expose, and fix MD022 heading spacing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_019gMrPNe6LwGtBS6Y7B5bo8 --- .../Components/StackMonitor/StackMonitor.cpp | 24 ++++---- .../unit-tests/test_StackMonitor_Core.cpp | 2 + S-BAND-REINTEGRATION-PLAN.md | 60 +++++++++++++++---- 3 files changed, 62 insertions(+), 24 deletions(-) diff --git a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp index 9fab981e..58d8ace4 100644 --- a/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp +++ b/PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp @@ -11,13 +11,12 @@ namespace Components { namespace { -//! k_thread_foreach callback: append this thread's stack usage to the -//! ThreadStackSampleSet pointed to by userData. Zephyr runs this callback -//! with the thread-monitor spinlock held and IRQs locked, so it must not -//! allocate or block: it only reads the stack high-water mark, does a -//! bounded string copy of the name, and stores integers into a fixed slot -//! (ThreadStackSampleSet::add). Keeps all Zephyr calls out of -//! StackMonitorCore, which stays pure. +//! k_thread_foreach_unlocked callback: append this thread's stack usage to +//! the ThreadStackSampleSet pointed to by userData. Called without the +//! thread-monitor spinlock held, so it must not allocate or block: it only +//! reads the stack high-water mark, does a bounded string copy of the name, +//! and stores integers into a fixed slot (ThreadStackSampleSet::add). Keeps +//! all Zephyr calls out of StackMonitorCore, which stays pure. void appendThreadSample(const struct k_thread* thread, void* userData) { auto* samples = static_cast(userData); @@ -29,12 +28,15 @@ void appendThreadSample(const struct k_thread* thread, void* userData) { return; } - const char* name = ""; + const char* name = "unknown"; #if defined(CONFIG_THREAD_NAME) // thread is logically read-only here, but k_thread_name_get takes a // non-const k_tid_t; the callback signature is fixed by - // k_thread_foreach and always hands us a live thread object. - name = k_thread_name_get(const_cast(thread)); + // k_thread_foreach_unlocked and always hands us a live thread object. + const char* threadName = k_thread_name_get(const_cast(thread)); + if (threadName != nullptr) { + name = threadName; + } #endif (void)samples->add(name, static_cast(thread->stack_info.size), @@ -58,7 +60,7 @@ StackMonitor ::~StackMonitor() {} void StackMonitor ::run_handler(FwIndexType portNum, U32 context) { this->m_samples.clear(); - k_thread_foreach(&appendThreadSample, &this->m_samples); + k_thread_foreach_unlocked(&appendThreadSample, &this->m_samples); this->m_core.tick(this->m_samples, this->m_result); diff --git a/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp b/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp index a1199fbf..d32bd6fe 100644 --- a/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp +++ b/PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp @@ -1,5 +1,7 @@ #include +#include + #include "PROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hpp" using Components::STACK_MONITOR_MAX_THREADS; diff --git a/S-BAND-REINTEGRATION-PLAN.md b/S-BAND-REINTEGRATION-PLAN.md index b242e6be..3b4f10d6 100644 --- a/S-BAND-REINTEGRATION-PLAN.md +++ b/S-BAND-REINTEGRATION-PLAN.md @@ -40,7 +40,7 @@ management / nRST reset), branch `s-band-speedup` (post-removal correctness fixe | D3 | Failure semantics: bounded call → N consecutive errors/timeouts → auto nRST reset (reuse #109 path) → M failed resets → **FAULTED, latched until ground command**. Invariant: S-Band failure degrades to "no S-Band", never to backpressure, hang, or spacecraft reset. UHF unaffected. | | D4 | Test seam: new `SBandRadioIf` abstract interface over the 13 SX1280 calls the component uses; production impl wraps SX1280 and owns the bounded-timeout logic; F´ UTs drive the component against a scripted fake. | | D5 | Three staged PRs (infra → component+UTs → topology re-enable). PR 3 gated on ≥24 h dual-radio HWIL soak + end-to-end functional pass. | -| D6 | Program-wide `StackMonitor` component (all threads, 1 Hz, `k_thread_foreach` + `k_thread_stack_space_get`, needs `CONFIG_THREAD_MONITOR=y`). | +| D6 | Program-wide `StackMonitor` component (all threads, 1 Hz, `k_thread_foreach_unlocked` + `k_thread_stack_space_get`, needs `CONFIG_THREAD_MONITOR=y` and `CONFIG_INIT_STACKS=y`). | | D7 | Out of scope: throughput tuning (passthrough chunk/cooldown), async com-chain rework (documented fallback only), static stack-analysis tooling, stale `s-band-*` branch cleanup, fprime-zephyr changes (already merged via #326). | **Fallback trigger:** if soak shows SBand stack high-water > 70 % of 8 KB, revisit @@ -51,16 +51,28 @@ option C from #299 (async hop in the com chain) before flying. ## PR 1 — Infrastructure (SBand stays commented out; zero flight-behavior change) ### Slice 1.1 — StackMonitor component (tracer bullet) -New passive component `Components/StackMonitor`, seam: `ThreadInfoProviderIf` -(Zephyr impl trivial; UTs use a fake). One RED→GREEN cycle per behavior, in order: -1. On each `run` tick, reports minimum-free-stack for every thread the provider - exposes (telemetry observable via the component tester). +New passive component `Components/StackMonitor`. No provider seam: the Zephyr +adapter (`StackMonitor.cpp`) calls `k_thread_foreach_unlocked` + +`k_thread_stack_space_get` directly and stays a thin, heap-free translation +into fixed-capacity samples; all decision logic (summary, warning/recovery +transitions, overflow) lives in the pure, host-testable `StackMonitorCore`. +One RED→GREEN cycle per behavior in `StackMonitorCore`, in order: + +1. On each `tick`, computes the worst thread's free bytes (`MinFreeBytes`/ + `WorstThread`), the count of threads below the warning threshold, and + whether the fixed-capacity sample buffer overflowed — this is what + `StackMonitor` publishes as telemetry. 2. Emits a WARNING_HI EVR when any thread's free stack drops below a threshold (percentage of its own size), throttled; clears on recovery. 3. Handles thread count changing between ticks without asserting. +The Zephyr adapter itself (the `k_thread_foreach_unlocked` callback, real +telemetry/EVR wiring) has no host build target and is validated via +integration/HWIL in PR 3, not component-level UTs. + ### Slice 1.2 — Loud boot failure on task-start error + Verify what `ActiveComponentBase`/`Os::Task` does today when `k_thread_stack_alloc` returns null (`ERROR_RESOURCES`). If it can silently limp, add an assert/FATAL. Behavior to pin (host UT where the Os layer permits, else @@ -68,12 +80,15 @@ HWIL check in PR 3): a deployment whose thread can't get its stack never runs half-alive. ### Slice 1.3 — Kconfig flips -`CONFIG_DYNAMIC_THREAD_ALLOC=y`, `CONFIG_THREAD_MONITOR=y`. No new tests (config -only); gate = full existing suite + CI build for v5c/v5d/v5e + a short UHF bench -sanity to prove no regression while SBand is still off. -**PR 1 exit:** main builds and soaks exactly as before, now with per-thread stack -telemetry visible in YAMCS. +`CONFIG_DYNAMIC_THREAD_ALLOC=y`, `CONFIG_THREAD_MONITOR=y`, `CONFIG_INIT_STACKS=y` +(required for `k_thread_stack_space_get`). No new tests (config only); gate = +full existing suite + CI build for v5c/v5d/v5e + a short UHF bench sanity to +prove no regression while SBand is still off. + +**PR 1 exit:** main builds and soaks exactly as before, now with worst-thread +stack telemetry (`MinFreeBytes`/`WorstThread`/`ThreadsBelowThreshold`/ +`SampleOverflow`) visible in YAMCS's Health packet. --- @@ -83,50 +98,60 @@ Un-comment `add_fprime_subdirectory(.../SBand/)` and the UT registration; the library and its tests build without any topology change. ### Slice 2.1 — Seam + first light (tracer bullet) + RED: component UT constructs SBand with a fake `SBandRadioIf`; asserts that startup configures the radio and arms receive. GREEN: introduce `SBandRadioIf`, refactor SBand to hold a reference (production default = RadioLib-backed impl), pass. This cycle proves the whole UT harness path works. ### Slice 2.2 — RX happy path + Fake presents a received packet → component emits `dataOut` with the exact bytes, re-arms receive, updates RSSI/SNR telemetry. (Implementation note, not test subject: read directly into the allocated `Fw::Buffer` — the 256-byte stack array goes away here.) ### Slice 2.3 — RX allocation failure + Buffer manager returns invalid buffer → WARNING EVR, receive re-armed, no leak, no crash. ### Slice 2.4 — TX path + transmit gate + `dataIn` while ENABLED → radio transmit called with the frame, buffer returned, comStatus emitted. While DISABLED → no radio call, buffer still returned, comStatus still emitted (the com queue must never starve). ### Slice 2.5 — Errors are bounded and counted + Fake returns an error / simulated timeout → call completes promptly, EVR emitted, consecutive-error counter advances; one success resets the counter. ### Slice 2.6 — Auto-reset escalation + N consecutive failures → exactly one nRST reset request via the #109 path, EVR, counters observable in telemetry. ### Slice 2.7 — FAULTED latch + M resets without recovery → FAULTED: telemetry flag set; no further radio-interface calls; `dataIn` buffers returned immediately; `run` ticks are no-ops. Latched — further errors can't re-trigger resets. ### Slice 2.8 — Ground recovery + `RESET_RADIO` (existing command) while FAULTED → full re-init through the fake, FAULTED cleared on success, re-latched if re-init fails. ### Slice 2.9 — Cherry-pick `s-band-speedup` correctness fixes + BUSY-GPIO wiring into RadioLib (7c473f6/363101e), `dataOut_out` after all SPI ops (8aeee2f), atomic inter-thread flag (9b240d0), enableRx/enableTx return types (17df3ec). Where a fix has observable behavior (ordering, flag races), pin it with a UT first; pure wiring is covered by PR 3 bench work. ### Slice 2.10 — `RADIOLIB_STATIC_ONLY=1` + Set in SBand's CMake (where RadioLib is added). Gate = build + full UT suite; grep the map file to confirm no RadioLib heap symbols remain on hot paths. @@ -138,26 +163,35 @@ topology/flight change. ## PR 3 — Topology re-enable (soak-gated) ### Slice 3.1 — Wire it back + Un-comment instances, `ComCcsdsSband` subtopology, connections, rate-group slots. New `SBAND_STACK_SIZE = 8 * 1024` for the sband instance only (comment explaining the pool-fallback mechanism). CI builds for v5c/v5d/v5e; existing integration + day-in-the-life suites green. ### Slice 3.2 — HWIL functional pass (bench, v5e) + - Downlink: TM frames received over the S-Band link end-to-end. - Uplink: command through `ComCcsdsSband.authenticationRouter` → `cmdDisp` executes. -- StackMonitor shows the sband thread running with an 8 KB stack. +- HWIL console/debug view (not telemetry — StackMonitor only reports the + worst thread) confirms the sband thread is running with an 8 KB stack. ### Slice 3.3 — HWIL fault injection (the #122 repro, now with an expected answer) + Physically disconnect SPI/BUSY mid-operation → expect: bounded EVRs → auto-reset attempts → FAULTED. **No watchdog reset, no queue overflow, UHF link still up.** Then `RESET_RADIO` with hardware restored → link recovers. ### Slice 3.4 — Soak gate (merge gate for PR 3) + ≥24 h continuous, both radios active with periodic S-Band TX/RX traffic: - zero watchdog/unexpected resets; -- sband stack high-water < 70 % of 8 KB (else trigger the option-C fallback review); -- no other thread's watermark regresses vs the PR 1 baseline; +- sband stack high-water < 70 % of 8 KB, observed via HWIL console/debug (not + StackMonitor telemetry, which only reports the worst thread) — else trigger + the option-C fallback review; +- StackMonitor's worst-thread watermark doesn't regress vs. the PR 1 baseline, + cross-checked against the HWIL per-thread console view for the sband thread + specifically; - heap watermark flat after init (STATIC_ONLY doing its job); - RX/TX counters advance the whole window.