From c0b75fb06ab3615b5b1934d9361dbf3a087bb07b Mon Sep 17 00:00:00 2001 From: Thomas Boyer-Chammard <49786685+thomas-bc@users.noreply.github.com> Date: Wed, 22 Oct 2025 15:17:27 -0700 Subject: [PATCH] Refactor FrameAccumulator not to assert on incorrect size_out (#4340) * Refactor FrameAccumulator not to assert on incorrect size_out * spelling * revert port names * Update check logic --- Svc/FrameAccumulator/FrameAccumulator.cpp | 24 ++++++++--- Svc/FrameAccumulator/FrameAccumulator.fpp | 14 +++--- Svc/FrameAccumulator/docs/sdd.md | 3 ++ .../test/ut/FrameAccumulatorTestMain.cpp | 5 +++ .../test/ut/FrameAccumulatorTester.cpp | 43 +++++++++++++++++++ .../test/ut/FrameAccumulatorTester.hpp | 3 ++ 6 files changed, 79 insertions(+), 13 deletions(-) diff --git a/Svc/FrameAccumulator/FrameAccumulator.cpp b/Svc/FrameAccumulator/FrameAccumulator.cpp index 4608da27e4..a704601cdf 100644 --- a/Svc/FrameAccumulator/FrameAccumulator.cpp +++ b/Svc/FrameAccumulator/FrameAccumulator.cpp @@ -116,19 +116,31 @@ void FrameAccumulator ::processRing() { // Detect must not consume data in the ring buffer FW_ASSERT(m_inRing.get_allocated_size() == remaining, static_cast(m_inRing.get_allocated_size()), static_cast(remaining)); + + // Drop frames that are too large to handle (can't fit in the accumulation circular buffer) + if (size_out > ringCapacity) { + // Detector reports a size_out larger than the accumulation buffer capacity, we will never be able + // to process it. Log a warning and discard a byte, then keep iterating to look for a new frame + this->log_WARNING_HI_FrameDetectionSizeError(size_out); + // Discard a single byte of data and start again + (void)this->m_inRing.rotate(1); + FW_ASSERT(m_inRing.get_allocated_size() == remaining - 1, + static_cast(m_inRing.get_allocated_size()), + static_cast(remaining)); + continue; + } + // On successful detection, consume data from the ring buffer and place it into an allocated frame if (status == FrameDetector::FRAME_DETECTED) { - // size_out must be set to the size of the buffer and must fit within the existing data + // size_out must be set (non-zero) and must fit within the remaining data FW_ASSERT(size_out != 0); FW_ASSERT(size_out <= remaining, static_cast(size_out), static_cast(remaining)); - // check for overflow before casting down to U32 - FW_ASSERT(size_out <= std::numeric_limits::max()); - Fw::Buffer buffer = this->bufferAllocate_out(0, static_cast(size_out)); + Fw::Buffer buffer = this->bufferAllocate_out(0, size_out); if (buffer.isValid()) { // Copy data out of ring buffer into the allocated buffer Fw::SerializeStatus serialize_status = this->m_inRing.peek(buffer.getData(), size_out); - buffer.setSize(static_cast(size_out)); + buffer.setSize(size_out); FW_ASSERT(serialize_status == Fw::SerializeStatus::FW_SERIALIZE_OK); // Consume (rotate) the data from the ring buffer serialize_status = this->m_inRing.rotate(size_out); @@ -146,8 +158,6 @@ void FrameAccumulator ::processRing() { } // More data needed else if (status == FrameDetector::MORE_DATA_NEEDED) { - // size_out can never be larger than the capacity of the ring. Otherwise all uplink will fail. - FW_ASSERT(size_out < m_inRing.get_capacity(), static_cast(size_out)); // Detection should report "more is needed" and set size_out to something larger than available data FW_ASSERT(size_out > remaining, static_cast(size_out), static_cast(remaining)); diff --git a/Svc/FrameAccumulator/FrameAccumulator.fpp b/Svc/FrameAccumulator/FrameAccumulator.fpp index cbf2f30b07..e614555d60 100644 --- a/Svc/FrameAccumulator/FrameAccumulator.fpp +++ b/Svc/FrameAccumulator/FrameAccumulator.fpp @@ -5,7 +5,7 @@ module Svc { # ---------------------------------------------------------------------- # FrameAccumulator interface # ---------------------------------------------------------------------- - import FrameAccumulator + import Svc.FrameAccumulator @ Port for deallocating buffers holding extracted frames output port bufferDeallocate: Fw.BufferSend @@ -18,6 +18,11 @@ module Svc { severity warning high \ format "Could not allocate a valid buffer to fit the detected frame" + @ A frame was detected whose size exceeds the internal accumulation buffer + @ capacity. No choice but to drop the frame. + event FrameDetectionSizeError(size_out: FwSizeType) \ + severity warning high \ + format "Reported size_out={} exceeds accumulation buffer capacity" ############################################################################### # Standard AC Ports for Events @@ -25,11 +30,8 @@ module Svc { @ Port for requesting the current time time get port timeCaller - @ Port for sending textual representation of events - text event port logTextOut - - @ Port for sending events to downlink - event port logOut + @ Ports for logging events + import Fw.Event } } diff --git a/Svc/FrameAccumulator/docs/sdd.md b/Svc/FrameAccumulator/docs/sdd.md index 0b7fb0e45f..e5a0a442bd 100644 --- a/Svc/FrameAccumulator/docs/sdd.md +++ b/Svc/FrameAccumulator/docs/sdd.md @@ -22,6 +22,9 @@ The `Svc::FrameAccumulator` receives `Fw::Buffer` objects on its `dataIn` input - `FRAME_DETECTED`: indicates there is a frame at the current head of the circular buffer. The `Svc::FrameAccumulator` allocates a new `Fw::Buffer` object to hold the frame, copies the detected frame from the circular buffer into the new `Fw::Buffer` object, and emits the new `Fw::Buffer` object (containing the frame) on its `dataOut` output port. The `Svc::FrameAccumulator` then rotates the circular buffer to remove the data that was just extracted, and deallocates the original `Fw::Buffer` that was received on the `dataIn` input port. - `MORE_DATA_NEEDED`: indicates that more data is needed to determine whether there is a valid frame. The `Svc::FrameAccumulator` deallocates the original `Fw::Buffer` that was received on the `dataIn` input port and halts execution, effectively waiting for the next `Fw::Buffer` to be received on the `dataIn` input port. +> [!NOTE] +> If the `Svc::FrameDetector` reports a `size_out` that is larger than the capacity of the internal accumulation buffer, the frame will cannot be processed. The `Svc::FrameAccumulator` logs a warning event, and continues searching for a new frame. + ```mermaid sequenceDiagram diff --git a/Svc/FrameAccumulator/test/ut/FrameAccumulatorTestMain.cpp b/Svc/FrameAccumulator/test/ut/FrameAccumulatorTestMain.cpp index 5f29fd93a1..793fd7da54 100644 --- a/Svc/FrameAccumulator/test/ut/FrameAccumulatorTestMain.cpp +++ b/Svc/FrameAccumulator/test/ut/FrameAccumulatorTestMain.cpp @@ -47,6 +47,11 @@ TEST(FrameAccumulator, testBufferReturnDeallocation) { tester.testBufferReturnDeallocation(); } +TEST(FrameAccumulator, testDetectionErrorHandling) { + Svc::FrameAccumulatorTester tester; + tester.testDetectionErrorHandling(); +} + int main(int argc, char** argv) { STest::Random::seed(); ::testing::InitGoogleTest(&argc, argv); diff --git a/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.cpp b/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.cpp index 6351bef10b..40cc7a9bcd 100644 --- a/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.cpp +++ b/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.cpp @@ -160,6 +160,49 @@ void FrameAccumulatorTester ::testBufferReturnDeallocation() { ASSERT_EQ(this->fromPortHistory_bufferDeallocate->at(0).fwBuffer.getSize(), sizeof(data)); } +void FrameAccumulatorTester ::testDetectionErrorHandling() { + FwSizeType too_large_size = this->component.m_inRing.get_capacity() + 1; + // Using buffer_size=1 to simplify test since otherwise Accumulator will loop `buffer_size` times + Fw::Buffer::SizeType buffer_size = 1; + U8 data[buffer_size]; + Fw::Buffer buffer(data, buffer_size); + ComCfg::FrameContext context; + + // Too large size reported by detector should emit event and continue + this->mockDetector.set_next_result(FrameDetector::Status::FRAME_DETECTED, too_large_size); + this->invoke_to_dataIn(0, buffer, context); + // Checks + ASSERT_from_dataReturnOut_SIZE(1); // input buffer ownership was returned + ASSERT_from_dataOut_SIZE(0); // No frame was sent out + ASSERT_EVENTS_SIZE(1); // One event should be logged: + ASSERT_EVENTS_FrameDetectionSizeError_SIZE(1); // FrameDetectionSizeError + ASSERT_EVENTS_FrameDetectionSizeError(0, too_large_size); // with expected size_out + + this->clearHistory(); + + // Too large size reported by detector should emit event and continue + this->mockDetector.set_next_result(FrameDetector::Status::MORE_DATA_NEEDED, too_large_size); + this->invoke_to_dataIn(0, buffer, context); + // Checks + ASSERT_from_dataReturnOut_SIZE(1); // input buffer ownership was returned + ASSERT_from_dataOut_SIZE(0); // No frame was sent out + ASSERT_EVENTS_SIZE(1); // One event should be logged: + ASSERT_EVENTS_FrameDetectionSizeError_SIZE(1); // FrameDetectionSizeError + ASSERT_EVENTS_FrameDetectionSizeError(0, too_large_size); // with expected size_out + + this->clearHistory(); + + // Too large size reported by detector should emit event and continue + this->mockDetector.set_next_result(FrameDetector::Status::NO_FRAME_DETECTED, too_large_size); + this->invoke_to_dataIn(0, buffer, context); + // Checks + ASSERT_from_dataReturnOut_SIZE(1); // input buffer ownership was returned + ASSERT_from_dataOut_SIZE(0); // No frame was sent out + ASSERT_EVENTS_SIZE(1); // One event should be logged: + ASSERT_EVENTS_FrameDetectionSizeError_SIZE(1); // FrameDetectionSizeError + ASSERT_EVENTS_FrameDetectionSizeError(0, too_large_size); // with expected size_out +} + // ---------------------------------------------------------------------- // Helper functions // ---------------------------------------------------------------------- diff --git a/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.hpp b/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.hpp index 47a78c82eb..528ae6398b 100644 --- a/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.hpp +++ b/Svc/FrameAccumulator/test/ut/FrameAccumulatorTester.hpp @@ -66,6 +66,9 @@ class FrameAccumulatorTester : public FrameAccumulatorGTestBase { //! Test returning ownership of a buffer void testBufferReturnDeallocation(); + //! Test handling of errors from the FrameDetector (too large size_out) + void testDetectionErrorHandling(); + private: // ---------------------------------------------------------------------- // Helper functions