feat(sband) 1/3: StackMonitor + dynamic-thread-alloc infrastructure - #448
Open
Mikefly123 wants to merge 9 commits into
Open
feat(sband) 1/3: StackMonitor + dynamic-thread-alloc infrastructure#448Mikefly123 wants to merge 9 commits into
Mikefly123 wants to merge 9 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…e_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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a fixed-capacity Zephyr stack monitor with warning and recovery events, integrates it into the reference deployment and Health telemetry, enables required thread configuration, adds core unit tests, and documents a staged S-Band reintegration plan. ChangesStackMonitor
S-Band reintegration plan
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant RateGroup
participant StackMonitor
participant ZephyrThreads
participant StackMonitorCore
participant Telemetry
RateGroup->>StackMonitor: invoke run
StackMonitor->>ZephyrThreads: enumerate thread stack information
ZephyrThreads-->>StackMonitor: return bounded samples
StackMonitor->>StackMonitorCore: tick samples
StackMonitorCore-->>StackMonitor: return summary and transitions
StackMonitor->>Telemetry: write Health metrics and log events
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cpp`:
- Around line 32-38: Update the thread-name handling in the k_thread_foreach
callback to replace a NULL result from k_thread_name_get with the default string
"unknown" before passing name to samples->add; retain the existing empty/default
behavior only where CONFIG_THREAD_NAME is unavailable.
- Around line 14-25: Update appendThreadSample and its caller to avoid invoking
k_thread_stack_space_get while k_thread_foreach holds the thread-monitor lock
and disables IRQs. Use k_thread_foreach_unlocked() when available; otherwise
restructure sampling so the stack high-water-mark scan runs outside the locked
callback path while preserving fixed-buffer collection.
In `@PROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cpp`:
- Around line 1-3: Add the standard <cstdio> header to
test_StackMonitor_Core.cpp alongside the existing includes so the later snprintf
usage has an explicit declaration; leave the test logic unchanged.
In `@S-BAND-REINTEGRATION-PLAN.md`:
- Around line 53-58: Revise Slice 1.1 to match the current implementation:
remove the planned ThreadInfoProviderIf seam and component-level test
requirement, and explicitly require StackMonitorCore host tests plus
integration/HWIL coverage for StackMonitor’s direct k_thread_foreach behavior.
Keep the minimum-free-stack reporting behavior unchanged.
- Around line 43-44: Update the infrastructure plan’s configuration
prerequisites to include CONFIG_INIT_STACKS alongside
CONFIG_DYNAMIC_THREAD_ALLOC and CONFIG_THREAD_MONITOR. Ensure the StackMonitor
entry explicitly reflects that initialized stacks are required by
k_thread_stack_space_get.
- Line 53: Insert one blank line after each listed Markdown section heading in
S-BAND-REINTEGRATION-PLAN.md, including the “Slice 1.1 — StackMonitor component
(tracer bullet)” heading and all referenced headings, without changing their
content.
- Around line 57-58: Update the validation criteria around the run-tick
telemetry and related stack checks to match what StackMonitor actually exposes:
worst-thread/minimum free bytes, below-threshold count, and overflow status.
Either add instrumentation that reports per-thread watermarks and supports the
stated 8 KB and regression checks, or rewrite those criteria to rely on
explicitly identified external measurements rather than promising unavailable
telemetry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9c6dff8d-67de-4d38-add7-f4de88736ab8
📒 Files selected for processing (15)
PROVESFlightControllerReference/Components/CMakeLists.txtPROVESFlightControllerReference/Components/StackMonitor/CMakeLists.txtPROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cppPROVESFlightControllerReference/Components/StackMonitor/StackMonitor.fppPROVESFlightControllerReference/Components/StackMonitor/StackMonitor.hppPROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.cppPROVESFlightControllerReference/Components/StackMonitor/StackMonitorCore.hppPROVESFlightControllerReference/Components/StackMonitor/docs/sdd.mdPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppiPROVESFlightControllerReference/ReferenceDeployment/Top/instances.fppPROVESFlightControllerReference/ReferenceDeployment/Top/topology.fppPROVESFlightControllerReference/test/unit-tests/CMakeLists.txtPROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cppS-BAND-REINTEGRATION-PLAN.mdprj.conf
Comment on lines
+43
to
+44
| | 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). | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include CONFIG_INIT_STACKS in the infrastructure plan.
k_thread_stack_space_get requires initialized stacks, and the PR objectives explicitly identify CONFIG_INIT_STACKS as required. Listing only CONFIG_DYNAMIC_THREAD_ALLOC and CONFIG_THREAD_MONITOR leaves the documented build prerequisites incomplete.
Also applies to: 70-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S-BAND-REINTEGRATION-PLAN.md` around lines 43 - 44, Update the infrastructure
plan’s configuration prerequisites to include CONFIG_INIT_STACKS alongside
CONFIG_DYNAMIC_THREAD_ALLOC and CONFIG_THREAD_MONITOR. Ensure the StackMonitor
entry explicitly reflects that initialized stacks are required by
k_thread_stack_space_get.
Contributor
Author
There was a problem hiding this comment.
Correct — CONFIG_INIT_STACKS is required (k_thread_stack_space_get depends on it) and it's already set in prj.conf for this PR, just missing from the plan doc's Kconfig list. Will add it alongside CONFIG_DYNAMIC_THREAD_ALLOC/CONFIG_THREAD_MONITOR in D6 and Slice 1.3.
|
|
||
| ## PR 1 — Infrastructure (SBand stays commented out; zero flight-behavior change) | ||
|
|
||
| ### Slice 1.1 — StackMonitor component (tracer bullet) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines after section headings.
The listed headings lack the blank line required by the Markdown linter, producing MD022 warnings. Insert one blank line after each heading.
Also applies to: 63-63, 70-70, 85-85, 91-91, 97-97, 101-101, 106-106, 110-110, 114-114, 119-119, 123-123, 129-129, 140-140, 146-146, 151-151, 156-156
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 53-53: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S-BAND-REINTEGRATION-PLAN.md` at line 53, Insert one blank line after each
listed Markdown section heading in S-BAND-REINTEGRATION-PLAN.md, including the
“Slice 1.1 — StackMonitor component (tracer bullet)” heading and all referenced
headings, without changing their content.
Source: Linters/SAST tools
Contributor
Author
There was a problem hiding this comment.
Will fix the MD022 blank-line nits, no content change.
Comment on lines
+53
to
+58
| ### 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). |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the planned test seam with the implementation.
The plan requires a ThreadInfoProviderIf seam and component-level tests, but StackMonitor.cpp directly invokes k_thread_foreach; the supplied implementation only exposes StackMonitorCore as host-testable. Either add the provider seam and component tests, or revise this slice to explicitly require core-only tests plus integration/HWIL coverage.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 53-53: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S-BAND-REINTEGRATION-PLAN.md` around lines 53 - 58, Revise Slice 1.1 to match
the current implementation: remove the planned ThreadInfoProviderIf seam and
component-level test requirement, and explicitly require StackMonitorCore host
tests plus integration/HWIL coverage for StackMonitor’s direct k_thread_foreach
behavior. Keep the minimum-free-stack reporting behavior unchanged.
Contributor
Author
There was a problem hiding this comment.
Correct, this is stale. We dropped the ThreadInfoProviderIf seam during implementation — direct k_thread_foreach in the Zephyr adapter plus host GTest coverage of StackMonitorCore proved sufficient and kept the component heap-free without an extra abstraction layer. Will revise Slice 1.1 to describe what actually shipped: StackMonitorCore host tests + HWIL/integration coverage of the adapter, no seam.
Contributor
Author
There was a problem hiding this comment.
Correct, this is stale. We dropped the ThreadInfoProviderIf seam during implementation — direct k_thread_foreach in the Zephyr adapter plus host GTest coverage of StackMonitorCore proved sufficient and kept the component heap-free without an extra abstraction layer. Will revise Slice 1.1 to describe what actually shipped: StackMonitorCore host tests + HWIL/integration coverage of the adapter, no seam.
Comment on lines
+57
to
+58
| 1. On each `run` tick, reports minimum-free-stack for every thread the provider | ||
| exposes (telemetry observable via the component tester). |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not promise per-thread telemetry that StackMonitor does not expose.
The implementation publishes only the worst thread, minimum free bytes, below-threshold count, and overflow status. It cannot currently prove “minimum-free-stack for every thread,” verify an 8 KB stack from telemetry, or establish that no other thread’s watermark regressed. Add the required telemetry/instrumentation or rewrite these validation criteria to use an external measurement.
Also applies to: 146-149, 159-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S-BAND-REINTEGRATION-PLAN.md` around lines 57 - 58, Update the validation
criteria around the run-tick telemetry and related stack checks to match what
StackMonitor actually exposes: worst-thread/minimum free bytes, below-threshold
count, and overflow status. Either add instrumentation that reports per-thread
watermarks and supports the stated 8 KB and regression checks, or rewrite those
criteria to rely on explicitly identified external measurements rather than
promising unavailable telemetry.
Contributor
Author
There was a problem hiding this comment.
Correct — telemetry is worst-thread + MinFreeBytes + ThreadsBelowThreshold + SampleOverflow, not a full per-thread report. The validation criteria overstate what telemetry alone can prove. Will rewrite them to reference what's actually exposed, and note that the 8 KB/regression checks in the PR description came from the HWIL console/debug view, not telemetry.
- 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 <cstdio> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gMrPNe6LwGtBS6Y7B5bo8
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@S-BAND-REINTEGRATION-PLAN.md`:
- Line 43: Update the StackMonitorCore integration requirements in the plan so
the configured maximum live thread count cannot exceed the fixed sampling
capacity; explicitly require SampleOverflow == false as an acceptance criterion,
ensuring MinFreeBytes and WorstThread cover every thread.
- Around line 127-128: Update the radio-call timeout behavior described in the
plan to define a measurable maximum duration or per-operation budget for every
call, including simulated timeouts. Add unit or integration assertions that each
call completes within that bound before exercising EVR emission and
consecutive-error/reset escalation.
- Around line 137-144: The FAULTED behavior description must distinguish normal
operation from recovery: clarify that periodic run ticks and standard dataIn
handling make no radio-interface calls while FAULTED, while the existing
RESET_RADIO command remains permitted to perform full re-initialization and
clear or re-latch FAULTED based on success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f082dae2-ac1c-411e-84eb-2f71efc87780
📒 Files selected for processing (3)
PROVESFlightControllerReference/Components/StackMonitor/StackMonitor.cppPROVESFlightControllerReference/test/unit-tests/test_StackMonitor_Core.cppS-BAND-REINTEGRATION-PLAN.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of three stacked PRs re-integrating the S-Band (SX1280) radio removed by #256, per the reintegration plan (S-BAND-REINTEGRATION-PLAN.md). This PR is pure infrastructure — no radio code.
What
StackMonitorcomponent (TDD'd, heap-free): samples every thread's stack high-water each 1 Hz tick viak_thread_foreach, reports the worst thread (MinFreeBytes/WorstThread/ThreadsBelowThreshold/SampleOverflow). Written heap-free after review caught a push_back-under-spinlock defect.CONFIG_DYNAMIC_THREAD_ALLOC=y(+THREAD_MONITOR,THREAD_NAME,INIT_STACKS— the latter is required ork_thread_stack_space_getdoesn't link): lets a single oversized thread-stack request (the 8 KB S-Band stack in PR 3) fall back to boot-timek_mallocinstead of raising the global 25-slot pool size (+100 KB, no-boot — the [FEATURE] S Band Radio #299 trap).Why
Issue #299's root cause was stack exhaustion with no visibility and no way to size one thread independently. This PR provides both, and is useful on
mainregardless of the radio: HWIL bring-up used these channels to gate every subsequent step.Verification
Host GTest suite for StackMonitor (repo test convention: pure-class tests in test/unit-tests); v5e + v5d builds clean; HWIL-validated on the bench v5e (Health packet reports live watermarks; deployment-wide worst thread
ActComp_17@ 1048 B free is pre-existing, tracked for the PR 3 soak).Stack: PR 1 (this) → PR 2 (component) → PR 3 (topology, HWIL-gated).
🤖 Generated with Claude Code