test(comdelay): host-side unit tests for latch/divider pacing (target: perf/comdelay-divider0-passthrough, stacks on #475) - #478
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ComDelay had no unit test coverage (CMakeLists UT block was a commented template). Extract the tick/divider/latch state machine into a host-testable ComDelayLogic class -- following this repo's established pattern for passive-component logic extraction (RtcHelper, BDot, Magnetorquer, StrategySelector, Bypasser, Parser/Validator/Authenticator) -- and add it to the gtest suite driven by `make test-unit`. ComDelay.cpp/.hpp now delegate to ComDelayLogic; behavior is unchanged (verified against the pre-existing atomic compare_exchange consume-once semantics and against the U8 tick-counter width). Covers: tick-paced release, divider math (small N and the default divider), exactly-once consumption, no-emit when nothing is latched, runtime divider changes mid-cycle (shrink and grow), and param-invalid fallback to the default divider. Finding: DEFAULT_DIVIDER=299 does not actually yield a ~30s/300-tick period as commented in ComDelay.fpp -- the production tick counter is a U8 (max 255), so it wraps via 8-bit overflow at 256 ticks before ever reaching the 299 comparison threshold. Tests pin the real (256-tick) behavior rather than the aspirational one; no behavior change made here.
d7ed952 to
a91b95e
Compare
|
Closing: accidental duplicate of #479 — two agents collided on the same head branch (test/comdelay-ut), leaving this PR's diff against the stale pre-#477 base meaningless. #479 carries the ComDelay UT work (same ComDelayLogic extraction + tests, based on current feat/usp-radio). The divider-0 passthrough test cases should be added on #475's branch once it is updated against post-#477 feat/usp-radio. |
Adds the passthrough test cases that belong with perf/comdelay-divider0-passthrough (#475), mirroring the coverage intended by closed PR #478: - divider-0 forwards immediately (both SUCCESS and FAILURE values) without ever setting the latch, so run ticks cannot double-emit - divider 0 -> N runtime transition latches/tick-paces again - N -> 0 transition with a status already latched neither loses the latched status nor double-emits it (exactly one tick emission) - invalid DIVIDER parameter falls back to the nonzero default and therefore latches, never passthroughs Kept as a separate commit so the extraction/refactor layer diff of this branch stays reviewable on its own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Stacks on #475 (perf/comdelay-divider0-passthrough). Extracts ComDelay's latch/divider pacing into a header-only, F'-free
DelayCore.hppand adds host-side gtest coverage, following the repo's established extracted-helper convention (test_LinkProfiles, test_DetumbleManager_*, test_RtcManager_RtcHelper inPROVESFlightControllerReference/test/unit-tests/). The component (ComDelay.cpp) keeps ownership of F' types and ports and delegates all timing decisions to the core — a behavior-preserving refactor plus tests.Why: the divider-0 passthrough in #475 shipped hardware-validated on HWIL but with zero automated coverage of ComDelay's pacing semantics; this pins them. Pacing correctness directly gates the comStatus-driven downlink cadence that the S-Band/USP throughput-ladder work (#439 and follow-ons) depends on.
Coverage (numbered per test plan)
LatchedStatusReleasedOnlyOnCounterZeroTickDividerNReleasesEveryNPlusOneTicks; default 299 pinned inDefaultDivider299WrapsAt256(see finding below)LatchedStatusConsumedExactlyOnceNoEmitWithoutLatchedStatusRuntimeDividerChangeKeepsLatchedStatusExactlyOnce,LatchedStatusSurvivesRuntimeChangeToDividerZeroDividerZeroPassesThroughWithoutLatching,DividerZeroThenNonZeroLatchingStillWorksInvalidParamFallsBackToDefaultDividerFinding: U8 tick-counter wrap
The tick counter is
U8, so for DIVIDER >= 256 it wraps 255->0 before reaching the divider: the default DIVIDER=299 actually releases every 256 ticks (~25.6 s @ 1 Hz), not the intended 300 (30 s).DefaultDivider299WrapsAt256pins the actual flight behavior and documents the fix path (widen to U16 in DelayCore + ComDelay). Left unfixed here to keep this PR behavior-preserving.Notes
feat/usp-radio; per instruction that branch's history was not touched. This PR is based on perf(comdelay): DIVIDER=0 passes comStatus straight through (TX-done-paced downlink) #475's current head as-is; chore(radio): collapse carried patches into OSSF fork integration pins #477 only collapses carried radio patches and does not touch ComDelay.make test-unit: 12/12 suites pass (newtest_ComDelay_DelayCore: 10/10 cases), existing suites unaffected.Do not merge before #475.
🤖 Generated with Claude Code