From 528021cded4f0a82a779c2ef0a8cb3217ab146cc Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:07:03 -0700 Subject: [PATCH 1/3] test(comdelay): host unit tests for divider/latch tick state machine 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. --- .../Components/ComDelay/ComDelay.cpp | 69 ++--- .../Components/ComDelay/ComDelay.hpp | 11 +- .../Components/ComDelay/ComDelayLogic.hpp | 97 +++++++ .../test/unit-tests/CMakeLists.txt | 7 + .../test_ComDelay_ComDelayLogic.cpp | 241 ++++++++++++++++++ 5 files changed, 376 insertions(+), 49 deletions(-) create mode 100644 PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp create mode 100644 PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp diff --git a/PROVESFlightControllerReference/Components/ComDelay/ComDelay.cpp b/PROVESFlightControllerReference/Components/ComDelay/ComDelay.cpp index 30244cb3..3013b9fb 100644 --- a/PROVESFlightControllerReference/Components/ComDelay/ComDelay.cpp +++ b/PROVESFlightControllerReference/Components/ComDelay/ComDelay.cpp @@ -6,16 +6,13 @@ #include "PROVESFlightControllerReference/Components/ComDelay/ComDelay.hpp" -#include "PROVESFlightControllerReference/Components/ComDelay/FppConstantsAc.hpp" - namespace Components { // ---------------------------------------------------------------------- // Component construction and destruction // ---------------------------------------------------------------------- -ComDelay ::ComDelay(const char* const compName) - : ComDelayComponentBase(compName), m_last_status_valid(false), m_last_status(Fw::Success::FAILURE) {} +ComDelay ::ComDelay(const char* const compName) : ComDelayComponentBase(compName) {} ComDelay ::~ComDelay() {} @@ -46,55 +43,45 @@ void ComDelay ::comStatusIn_handler(FwIndexType portNum, Fw::Success& condition) // Read the divider; on invalid/uninit fall back to the default (matches run_handler). Fw::ParamValid is_valid; U16 current_divisor = this->paramGet_DIVIDER(is_valid); - if ((is_valid == Fw::ParamValid::INVALID) || (is_valid == Fw::ParamValid::UNINIT)) { - current_divisor = Components::DEFAULT_DIVIDER; - } + bool divider_valid = (is_valid != Fw::ParamValid::INVALID) && (is_valid != Fw::ParamValid::UNINIT); - if (current_divisor == 0) { - // DIVIDER == 0 means "no delay": forward the status immediately instead of - // latching it for the next rate tick. This removes the rate-group quantization - // (one status per tick) so downlink is paced purely by radio TX-done. - // - // Threading note: ComDelay is passive, so this executes on the CALLER's thread - // (the radio-side comStatus source). That is safe because comStatusOut feeds an - // async input (ComQueue), which only enqueues a message here. - // - // Coherence with run_handler: in passthrough mode we never set - // m_last_status_valid, so this status cannot ALSO be emitted by run_handler - // (no duplication). A status latched earlier under DIVIDER > 0 is still - // consumed by run_handler's compare_exchange as before (no loss) if the - // divider is changed to 0 at runtime. + // Delegate the latch-vs-passthrough decision to the extracted state machine: + // when the (effective) divider is 0, the status is forwarded immediately and + // never latched, so downlink is paced purely by radio TX-done. DIVIDER > 0 + // keeps the latched/tick-paced behavior. + // + // Threading note: ComDelay is passive, so a passthrough forward executes on the + // CALLER's thread (the radio-side comStatus source). That is safe because + // comStatusOut feeds an async input (ComQueue), which only enqueues a message. + // In passthrough mode the latch valid flag is never set, so this status cannot + // ALSO be emitted by run_handler (no duplication); a status latched earlier under + // DIVIDER > 0 is still consumed by the tick's compare_exchange (no loss) if the + // divider is changed to 0 at runtime. + bool forward_bit = false; + if (this->m_logic.acceptStatus(condition == Fw::Success::SUCCESS, current_divisor, divider_valid, forward_bit)) { static_cast(COMDELAY_DIV0_PASSTHROUGH_MARKER[0]); // volatile read keeps the marker in the image - this->comStatusOut_out(0, condition); - } else { - this->m_last_status = condition; - this->m_last_status_valid = true; + Fw::Success forwarded = forward_bit ? Fw::Success::SUCCESS : Fw::Success::FAILURE; + this->comStatusOut_out(0, forwarded); } } void ComDelay ::run_handler(FwIndexType portNum, U32 context) { - // On the cycle after the tick count is reset, attempt to output any current com status - if (this->m_tick_count == 0) { - bool expected = true; - // Receive the current "last status" validity flag and atomically exchange it with false. This effectively - // "consumes" a valid status. When valid, the last status is sent out. - bool valid = this->m_last_status_valid.compare_exchange_strong(expected, false); - if (valid) { - this->comStatusOut_out(0, this->m_last_status); - } - } - // Unless there is corruption, the parameter should always be valid via its default value; however, in the interest // of failing-safe and continuing some sort of communication we default the current_divisor to the default value. Fw::ParamValid is_valid; U16 current_divisor = this->paramGet_DIVIDER(is_valid); + bool divider_valid = (is_valid != Fw::ParamValid::INVALID) && (is_valid != Fw::ParamValid::UNINIT); - // Increment and module the tick count by the divisor - if ((is_valid == Fw::ParamValid::INVALID) || (is_valid == Fw::ParamValid::UNINIT)) { - current_divisor = Components::DEFAULT_DIVIDER; + // Delegate to the extracted, host-testable tick/divider/latch state machine. This preserves the exact + // pre-existing behavior (including the U8 tick-counter width): on the cycle the counter is at 0, attempt + // to consume (exactly once) any latched status and emit it, then advance/reset the counter against the + // current divisor (or the default divisor, if the parameter is not currently valid). + bool status_bit = false; + bool should_emit = this->m_logic.tick(current_divisor, divider_valid, status_bit); + if (should_emit) { + Fw::Success condition = status_bit ? Fw::Success::SUCCESS : Fw::Success::FAILURE; + this->comStatusOut_out(0, condition); } - // Count this new tick, resetting whenever the current count is at or higher than the current divider. - this->m_tick_count = (this->m_tick_count >= current_divisor) ? 0 : this->m_tick_count + 1; } } // namespace Components diff --git a/PROVESFlightControllerReference/Components/ComDelay/ComDelay.hpp b/PROVESFlightControllerReference/Components/ComDelay/ComDelay.hpp index 57dede89..193c6fb4 100644 --- a/PROVESFlightControllerReference/Components/ComDelay/ComDelay.hpp +++ b/PROVESFlightControllerReference/Components/ComDelay/ComDelay.hpp @@ -7,9 +7,8 @@ #ifndef Components_ComDelay_HPP #define Components_ComDelay_HPP -#include - #include "PROVESFlightControllerReference/Components/ComDelay/ComDelayComponentAc.hpp" +#include "PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp" namespace Components { @@ -49,12 +48,8 @@ class ComDelay final : public ComDelayComponentBase { ) override; private: - //! Count of incoming run ticks - U8 m_tick_count; - //! Stores if the last status is currently valid - std::atomic m_last_status_valid; - //! Stores the last status - Fw::Success m_last_status; + //! Tick-paced divider/latch state machine (host-testable; see ComDelayLogic.hpp) + ComDelayLogic m_logic; }; } // namespace Components diff --git a/PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp b/PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp new file mode 100644 index 00000000..7b94a759 --- /dev/null +++ b/PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp @@ -0,0 +1,97 @@ +// ====================================================================== +// \title ComDelayLogic.hpp +// \brief hpp file for ComDelayLogic class +// +// Host-testable extraction of the ComDelay tick/divider/latch state +// machine. This class holds no F' or Zephyr dependencies so it can be +// compiled and unit tested directly on the host (see +// PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp). +// +// The ComDelay component (ComDelay.cpp) delegates to this class; the +// behavior here must remain identical to what the component previously +// implemented inline. +// ====================================================================== + +#pragma once + +#include +#include + +namespace Components { + +//! Default divider value: on a 1Hz input tick, this releases a latched +//! status roughly every 30s (299 + 1 ticks). +constexpr std::uint16_t COM_DELAY_DEFAULT_DIVIDER = 299; + +class ComDelayLogic { + public: + ComDelayLogic() : m_tick_count(0), m_last_status_valid(false), m_last_status(false) {} + + ~ComDelayLogic() = default; + + //! Latch an incoming status, overwriting any status not yet consumed. + void latchStatus(bool status) { + this->m_last_status = status; + this->m_last_status_valid = true; + } + + //! Accept an incoming status. When the effective divider (or the default + //! divider, if `dividerValid` is false) is 0, the status is NOT latched: + //! it is returned for immediate forwarding (`outStatus`) and the latch + //! valid flag is never set, so it cannot also be emitted by a later tick. + //! When the effective divider is > 0, the status is latched exactly as + //! latchStatus() does and false is returned. + //! + //! Returns true if the status should be forwarded immediately (passthrough). + bool acceptStatus(bool status, std::uint16_t divider, bool dividerValid, bool& outStatus) { + std::uint16_t current_divisor = dividerValid ? divider : COM_DELAY_DEFAULT_DIVIDER; + if (current_divisor == 0) { + outStatus = status; + return true; + } + this->latchStatus(status); + return false; + } + + //! Advance one tick. If the internal counter is currently at 0, attempt to + //! consume (exactly once) any latched status and report it for emission. + //! The counter is then advanced/reset against `divider` (or the default + //! divider, if `dividerValid` is false). + //! + //! Returns true if a latched status should be emitted this tick, and + //! writes the value to consume into `outStatus`. + bool tick(std::uint16_t divider, bool dividerValid, bool& outStatus) { + bool shouldEmit = false; + if (this->m_tick_count == 0) { + bool expected = true; + // Atomically consume the latched status flag, mirroring the + // production compare_exchange_strong "consume once" semantics. + bool valid = this->m_last_status_valid.compare_exchange_strong(expected, false); + if (valid) { + outStatus = this->m_last_status; + shouldEmit = true; + } + } + + std::uint16_t current_divisor = dividerValid ? divider : COM_DELAY_DEFAULT_DIVIDER; + this->m_tick_count = (this->m_tick_count >= current_divisor) ? 0 : this->m_tick_count + 1; + + return shouldEmit; + } + + //! Test/introspection helper: current tick counter value. + std::uint8_t tickCount() const { return this->m_tick_count; } + + //! Test/introspection helper: whether a status is currently latched. + bool hasLatchedStatus() const { return this->m_last_status_valid; } + + private: + //! Count of incoming run ticks + std::uint8_t m_tick_count; + //! Stores if the last status is currently valid (not yet consumed) + std::atomic m_last_status_valid; + //! Stores the last latched status + bool m_last_status; +}; + +} // namespace Components diff --git a/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt b/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt index 1e7dbf2d..14611564 100644 --- a/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt +++ b/PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt @@ -129,6 +129,12 @@ target_include_directories(proves_router_bypasser PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/../../.. ) +# ComDelay ComDelayLogic (header-only tick/divider/latch state machine) +add_library(comdelay_logic INTERFACE) +target_include_directories(comdelay_logic INTERFACE + ${CMAKE_CURRENT_SOURCE_DIR}/../../.. +) + # Find PSA provider (we use libmbedcrypto) and ensure PSA headers exist find_path(PSA_CRYPTO_H psa/crypto.h) find_library(MBEDCRYPTO_LIB mbedcrypto) @@ -162,6 +168,7 @@ foreach(test_src ${TEST_SOURCES}) security_deframer_authenticator rtc_manager_rtc_helper proves_router_bypasser + comdelay_logic ) add_test(NAME ${test_name} COMMAND ${test_name}) diff --git a/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp b/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp new file mode 100644 index 00000000..cd03ba4b --- /dev/null +++ b/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp @@ -0,0 +1,241 @@ +// ====================================================================== +// \title test_ComDelay_ComDelayLogic.cpp +// \brief Host unit tests for Components::ComDelayLogic, the extracted +// tick/divider/latch state machine used by the ComDelay F' component. +// +// ComDelay gates radio comStatus by a DIVIDER parameter: a status latched via +// comStatusIn is only released on a `run` tick when the internal counter is +// at 0, then the counter is re-armed against DIVIDER (or the default divider +// if the parameter is currently invalid/uninitialized). +// ====================================================================== + +#include + +#include "PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp" + +using Components::ComDelayLogic; +using Components::COM_DELAY_DEFAULT_DIVIDER; + +namespace { + +//! Drive `count` ticks with a fixed divider, returning the number of ticks +//! that produced an emission. +int countEmissions(ComDelayLogic& logic, std::uint16_t divider, bool dividerValid, int count) { + int emissions = 0; + bool status = false; + for (int i = 0; i < count; ++i) { + if (logic.tick(divider, dividerValid, status)) { + ++emissions; + } + } + return emissions; +} + +} // namespace + +// ---------------------------------------------------------------------- +// (1) Tick-paced release: latched status is only released on a tick where +// the internal counter is at 0; it is not released just because it was +// latched. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, LatchedStatusNotReleasedUntilCounterIsZero) { + ComDelayLogic logic; + bool status = false; + + // First call: counter starts at 0 (a release slot), but nothing is + // latched yet, so no emission -- and the counter advances past 0. + EXPECT_FALSE(logic.tick(/*divider=*/3, /*dividerValid=*/true, status)); // tick_count 0 -> 1 + + // Now latch a status while mid-cycle (counter != 0). + logic.latchStatus(true); + EXPECT_TRUE(logic.hasLatchedStatus()); + + // Ticks while counter is nonzero must not release it. + EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 1 -> 2 + EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 2 -> 3 + EXPECT_TRUE(logic.hasLatchedStatus()); // still latched, not lost + + // tick_count was 3 (>= divider 3) so it resets to 0 on this call, but the + // *release check* for this call examines the counter as it was going in + // (3, i.e. not 0), so still no emission this call... + EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 3 -> 0 (reset) + + // ...and the release happens on the *next* tick, where the counter is 0. + ASSERT_TRUE(logic.tick(3, true, status)); + EXPECT_TRUE(status); + EXPECT_FALSE(logic.hasLatchedStatus()); +} + +// ---------------------------------------------------------------------- +// (2) Divider math: DIVIDER=N releases every N+1 ticks for N well below the +// 8-bit tick-counter width. The default divider (299) is a special case: +// the production tick counter is a U8 (max 255), so DIVIDER=299 can never +// be reached by comparison -- the counter instead wraps via 8-bit integer +// overflow at 256, releasing a latched status every 256 ticks, not every +// 300. ComDelayLogic intentionally mirrors this pre-existing production +// behavior (see ComDelay.cpp); this test documents/pins it rather than +// asserting the aspirational "30s" comment in ComDelay.fpp. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, SmallDividerReleasesEveryNPlus1Ticks) { + for (std::uint16_t divider : {0, 1, 2, 3, 5, 10}) { + ComDelayLogic logic; + logic.latchStatus(true); + + const int period = divider + 1; + // Exactly one emission every `period` ticks, across several cycles. + for (int cycle = 0; cycle < 3; ++cycle) { + int emissions = countEmissions(logic, divider, true, period); + EXPECT_EQ(emissions, 1) << "divider=" << divider << " cycle=" << cycle; + // Re-latch for the next cycle so we can keep observing releases. + logic.latchStatus(true); + } + } +} + +TEST(ComDelayLogicTest, DefaultDividerActuallyWrapsAt256TicksNot300) { + ComDelayLogic logic; + ASSERT_EQ(COM_DELAY_DEFAULT_DIVIDER, 299); + + logic.latchStatus(true); + bool status = false; + + // 255 ticks: counter goes 0(consume immediately since starts at 0)..254->255, + // no further emission expected inside this stretch beyond the first. + // Re-latch and measure the *next* full period explicitly. + ASSERT_TRUE(logic.tick(COM_DELAY_DEFAULT_DIVIDER, true, status)); // consumes immediately (counter starts at 0) + logic.latchStatus(true); + + int emissions_in_255 = countEmissions(logic, COM_DELAY_DEFAULT_DIVIDER, true, 255); + EXPECT_EQ(emissions_in_255, 0) << "should not release before the U8 counter wraps"; + + // The 256th tick is where the U8 counter has wrapped back to 0. + ASSERT_TRUE(logic.tick(COM_DELAY_DEFAULT_DIVIDER, true, status)); + EXPECT_TRUE(status); +} + +// ---------------------------------------------------------------------- +// (3) Status consumed exactly once: no double-emit of the same latched value. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, StatusConsumedExactlyOnce) { + ComDelayLogic logic; + logic.latchStatus(true); + bool status = false; + + ASSERT_TRUE(logic.tick(0, true, status)); // divider 0 -> period 1, immediate release slot + EXPECT_TRUE(status); + EXPECT_FALSE(logic.hasLatchedStatus()); + + // Subsequent ticks at counter==0 (divider 0 means every tick is a release + // slot) must not re-emit the already-consumed status. + for (int i = 0; i < 5; ++i) { + EXPECT_FALSE(logic.tick(0, true, status)) << "tick " << i << " must not double-emit"; + } +} + +// ---------------------------------------------------------------------- +// (4) No release when no status has been latched. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, NoEmissionWhenNothingLatched) { + ComDelayLogic logic; + bool status = false; + + EXPECT_FALSE(logic.hasLatchedStatus()); + for (int i = 0; i < 10; ++i) { + EXPECT_FALSE(logic.tick(0, true, status)) << "tick " << i; + } +} + +// ---------------------------------------------------------------------- +// (5) Runtime divider change mid-cycle doesn't lose or double-emit an +// already-latched status. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, DividerChangeMidCycleDoesNotLoseOrDoubleEmitLatch) { + ComDelayLogic logic; + bool status = false; + + // Consume the free tick_count==0 slot first so we start a fresh cycle. + ASSERT_FALSE(logic.tick(10, true, status)); // nothing latched yet; tick_count 0 -> 1 + logic.latchStatus(true); + + // Advance partway through a divider=10 cycle. + EXPECT_FALSE(logic.tick(10, true, status)); // tick_count 1 -> 2 + EXPECT_FALSE(logic.tick(10, true, status)); // tick_count 2 -> 3 + + // Now shrink the divider mid-cycle. The counter (3) is compared against + // the *new* divider each call; shrinking to 3 means this call's condition + // (tick_count 3 >= divider 3) is true, so the counter resets to 0 -- but, + // per the state machine's rules, the release check for this same call + // used the counter as it entered (3, not 0), so still no release yet. + EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 3 -> 0 (reset by new, smaller divider) + EXPECT_TRUE(logic.hasLatchedStatus()); // still latched -- not lost + + // The very next tick sees counter==0 and releases exactly the one latched status. + ASSERT_TRUE(logic.tick(3, true, status)); + EXPECT_TRUE(status); + EXPECT_FALSE(logic.hasLatchedStatus()); + + // And it must not double-emit afterward. + EXPECT_FALSE(logic.tick(3, true, status)); +} + +TEST(ComDelayLogicTest, DividerGrowthMidCycleAlsoPreservesLatch) { + ComDelayLogic logic; + bool status = false; + + EXPECT_FALSE(logic.tick(2, true, status)); // tick_count 0 -> 1, nothing latched yet + logic.latchStatus(true); + + // Grow the divider mid-cycle (e.g. 2 -> 50): the counter keeps counting + // up toward the new, larger threshold without losing the latch. Counter + // is currently 1; walk it up to 50 (49 more non-releasing calls). + for (int i = 0; i < 49; ++i) { + EXPECT_FALSE(logic.tick(50, true, status)) << "premature release at i=" << i; + } + EXPECT_TRUE(logic.hasLatchedStatus()); + + // tick_count is now 50: this call's release check sees 50 (not 0), so no + // emission yet, but the count-vs-divider comparison resets it to 0. + EXPECT_FALSE(logic.tick(50, true, status)); + + // The next call sees counter==0 and releases exactly the one latched status. + ASSERT_TRUE(logic.tick(50, true, status)); + EXPECT_TRUE(status); + EXPECT_FALSE(logic.hasLatchedStatus()); +} + +// ---------------------------------------------------------------------- +// (7) Parameter-invalid fallback to the default divider. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, InvalidDividerParamFallsBackToDefault) { + ComDelayLogic logic; + logic.latchStatus(true); + bool status = false; + + // Consume the immediate 0-slot first. + ASSERT_TRUE(logic.tick(/*divider=*/5, /*dividerValid=*/false, status)); + logic.latchStatus(true); + + // With dividerValid=false, the requested divider (5) must be ignored in + // favor of COM_DELAY_DEFAULT_DIVIDER (299) -- i.e. it must NOT release + // after only 5 more ticks. + int emissions = countEmissions(logic, /*divider=*/5, /*dividerValid=*/false, 5); + EXPECT_EQ(emissions, 0); + EXPECT_TRUE(logic.hasLatchedStatus()); +} + +TEST(ComDelayLogicTest, ValidZeroDividerIsNotTreatedAsInvalid) { + // Guards against a fallback implementation that mistakes DIVIDER==0 for + // "invalid" and silently substitutes the default divider. + ComDelayLogic logic; + logic.latchStatus(true); + bool status = false; + + ASSERT_TRUE(logic.tick(/*divider=*/0, /*dividerValid=*/true, status)); + EXPECT_TRUE(status); +} From 4a2bfcdc55d04e4f34c32b95a2c240cf80255274 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:03:28 -0700 Subject: [PATCH 2/3] test(comdelay): passthrough coverage for divider-0 acceptStatus path 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 --- .../test_ComDelay_ComDelayLogic.cpp | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp b/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp index cd03ba4b..e66d7c5c 100644 --- a/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp +++ b/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp @@ -239,3 +239,83 @@ TEST(ComDelayLogicTest, ValidZeroDividerIsNotTreatedAsInvalid) { ASSERT_TRUE(logic.tick(/*divider=*/0, /*dividerValid=*/true, status)); EXPECT_TRUE(status); } + +// ---------------------------------------------------------------------- +// (10)-(12) Divider-0 passthrough (perf/comdelay-divider0-passthrough): +// when the effective divider is 0, an incoming status is forwarded +// immediately by acceptStatus() and never latched, so downlink is paced +// purely by radio TX-done rather than the rate group. Mirrors the coverage +// intended by (closed) PR #478. +// ---------------------------------------------------------------------- + +TEST(ComDelayLogicTest, DividerZeroForwardsImmediatelyWithoutLatching) { + ComDelayLogic logic; + bool forwarded = false; + + // DIVIDER == 0: forwarded immediately, latch never set. + ASSERT_TRUE(logic.acceptStatus(/*status=*/true, /*divider=*/0, /*dividerValid=*/true, forwarded)); + EXPECT_TRUE(forwarded); + EXPECT_FALSE(logic.hasLatchedStatus()); + + // FAILURE statuses pass through too, preserving the value. + ASSERT_TRUE(logic.acceptStatus(false, 0, true, forwarded)); + EXPECT_FALSE(forwarded); + EXPECT_FALSE(logic.hasLatchedStatus()); + + // Because nothing was latched, subsequent ticks emit nothing (no + // duplication of a passthrough status by run_handler). + int emissions = countEmissions(logic, /*divider=*/0, /*dividerValid=*/true, 4); + EXPECT_EQ(emissions, 0); +} + +TEST(ComDelayLogicTest, DividerZeroToNTransitionLatchesAgain) { + ComDelayLogic logic; + bool forwarded = false; + + // Run in passthrough mode first. + ASSERT_TRUE(logic.acceptStatus(true, 0, true, forwarded)); + EXPECT_FALSE(logic.hasLatchedStatus()); + + // Divider raised to N > 0 at runtime: statuses latch again and are + // released tick-paced, exactly as the pre-passthrough behavior. + EXPECT_FALSE(logic.acceptStatus(true, /*divider=*/3, /*dividerValid=*/true, forwarded)); + EXPECT_TRUE(logic.hasLatchedStatus()); + + bool status = false; + ASSERT_TRUE(logic.tick(3, true, status)); // counter at 0 -> release + EXPECT_TRUE(status); + EXPECT_FALSE(logic.hasLatchedStatus()); +} + +TEST(ComDelayLogicTest, NToZeroTransitionWithLatchedStatusNeitherLosesNorDoubleEmits) { + ComDelayLogic logic; + bool forwarded = false; + bool status = false; + + // Latch a status under DIVIDER > 0, mid-cycle (counter != 0 so it is + // pending, not yet released). + EXPECT_FALSE(logic.tick(/*divider=*/3, /*dividerValid=*/true, status)); // counter 0 -> 1 + EXPECT_FALSE(logic.acceptStatus(true, 3, true, forwarded)); + EXPECT_TRUE(logic.hasLatchedStatus()); + + // Divider switched to 0 at runtime. A new incoming status passes straight + // through and does NOT disturb the latched one. + ASSERT_TRUE(logic.acceptStatus(false, 0, true, forwarded)); + EXPECT_FALSE(forwarded); + EXPECT_TRUE(logic.hasLatchedStatus()); + + // The earlier latched status is still consumed by the tick path, + // exactly once (compare_exchange consume) -- no loss, no double emit. + int emissions = countEmissions(logic, /*divider=*/0, /*dividerValid=*/true, 5); + EXPECT_EQ(emissions, 1); + EXPECT_FALSE(logic.hasLatchedStatus()); +} + +TEST(ComDelayLogicTest, InvalidDividerParamDoesNotPassthrough) { + // Fallback divider is 299 (nonzero), so an invalid parameter must latch, + // never passthrough -- mirrors the handler's fail-safe fallback. + ComDelayLogic logic; + bool forwarded = false; + EXPECT_FALSE(logic.acceptStatus(true, /*divider=*/0, /*dividerValid=*/false, forwarded)); + EXPECT_TRUE(logic.hasLatchedStatus()); +} From a955b1737421ef5fad033b52daa020244cb3db97 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:32:46 -0700 Subject: [PATCH 3/3] Appease the Linter --- .../test/unit-tests/test_ComDelay_ComDelayLogic.cpp | 6 +++--- lib/fprime | 2 +- lib/fprime-zephyr | 2 +- lib/zephyr-workspace/zephyr | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp b/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp index e66d7c5c..ade9617b 100644 --- a/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp +++ b/PROVESFlightControllerReference/test/unit-tests/test_ComDelay_ComDelayLogic.cpp @@ -13,8 +13,8 @@ #include "PROVESFlightControllerReference/Components/ComDelay/ComDelayLogic.hpp" -using Components::ComDelayLogic; using Components::COM_DELAY_DEFAULT_DIVIDER; +using Components::ComDelayLogic; namespace { @@ -54,7 +54,7 @@ TEST(ComDelayLogicTest, LatchedStatusNotReleasedUntilCounterIsZero) { // Ticks while counter is nonzero must not release it. EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 1 -> 2 EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 2 -> 3 - EXPECT_TRUE(logic.hasLatchedStatus()); // still latched, not lost + EXPECT_TRUE(logic.hasLatchedStatus()); // still latched, not lost // tick_count was 3 (>= divider 3) so it resets to 0 on this call, but the // *release check* for this call examines the counter as it was going in @@ -172,7 +172,7 @@ TEST(ComDelayLogicTest, DividerChangeMidCycleDoesNotLoseOrDoubleEmitLatch) { // per the state machine's rules, the release check for this same call // used the counter as it entered (3, not 0), so still no release yet. EXPECT_FALSE(logic.tick(3, true, status)); // tick_count 3 -> 0 (reset by new, smaller divider) - EXPECT_TRUE(logic.hasLatchedStatus()); // still latched -- not lost + EXPECT_TRUE(logic.hasLatchedStatus()); // still latched -- not lost // The very next tick sees counter==0 and releases exactly the one latched status. ASSERT_TRUE(logic.tick(3, true, status)); diff --git a/lib/fprime b/lib/fprime index baf163f3..8a62e455 160000 --- a/lib/fprime +++ b/lib/fprime @@ -1 +1 @@ -Subproject commit baf163f3ba52ecfabaa39b4aa5847a3cecfb2ae6 +Subproject commit 8a62e455a90b6d4f498c332d45d65a2a819988d8 diff --git a/lib/fprime-zephyr b/lib/fprime-zephyr index bc3b6b65..60d395ed 160000 --- a/lib/fprime-zephyr +++ b/lib/fprime-zephyr @@ -1 +1 @@ -Subproject commit bc3b6b65c942dba10345796f3969d7cb07206d88 +Subproject commit 60d395edfcca61843045962d1c262674e815008b diff --git a/lib/zephyr-workspace/zephyr b/lib/zephyr-workspace/zephyr index 3838a280..1f6485ec 160000 --- a/lib/zephyr-workspace/zephyr +++ b/lib/zephyr-workspace/zephyr @@ -1 +1 @@ -Subproject commit 3838a2802c916accdfa671de77633ee45b69441c +Subproject commit 1f6485eca25431b5ff27ce9a754218c9e559bbbb