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

@@ -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<FwAssertArgType>(m_inRing.get_allocated_size()), static_cast<FwAssertArgType>(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<FwAssertArgType>(m_inRing.get_allocated_size()),
+                      static_cast<FwAssertArgType>(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<FwAssertArgType>(size_out),
                       static_cast<FwAssertArgType>(remaining));
-            // check for overflow before casting down to U32
-            FW_ASSERT(size_out <= std::numeric_limits<U32>::max());
-            Fw::Buffer buffer = this->bufferAllocate_out(0, static_cast<U32>(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<Fw::Buffer::SizeType>(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<FwAssertArgType>(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<FwAssertArgType>(size_out),
                       static_cast<FwAssertArgType>(remaining));

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
 
     }
 }

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

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);

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
 // ----------------------------------------------------------------------

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