Remove FprimeRouter Copy (#4984)

* Remove unnecessary buffer allocation/copy in FprimeRouter for file and unknown packets

Instead of allocating a new buffer and copying data for file and unknown
packet types, pass the original buffer directly to the receiver. The
buffer is not returned to the deframer via dataReturnOut until it comes
back on fileBufferReturnIn, at which point an empty context is
constructed for the return.

This eliminates the need for bufferAllocate and bufferDeallocate ports,
as well as the AllocationReason enum and AllocationError event.

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Remove dangling fprimeRouter buffer allocation connections from subtopologies

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Move dataReturnOut into each switch case instead of using early returns

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Fix formatting: remove extra blank line

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Update Svc/FprimeRouter/docs/sdd.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Address review: empty context on all return paths, clarify buffer return contract, document context discard

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

* Clarify SDD: receivers must return buffers via fileBufferReturnIn

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: 3 people

Svc/FprimeRouter/FprimeRouter.cpp

@@ -42,56 +42,40 @@ void FprimeRouter ::dataIn_handler(FwIndexType portNum, Fw::Buffer& packetBuffer
             } else {
                 this->log_WARNING_HI_SerializationError(status);
             }
+            // Return ownership of the incoming packetBuffer with an empty context
+            ComCfg::FrameContext emptyContext;
+            this->dataReturnOut_out(0, packetBuffer, emptyContext);
             break;
         }
         // Handle a file packet
         case Fw::ComPacketType::FW_PACKET_FILE: {
-            // If the file uplink output port is connected, send the file packet. Otherwise take no action.
+            // If the file uplink output port is connected, send the file packet directly.
+            // Ownership is passed to the receiver and will come back on fileBufferReturnIn,
+            // at which point we return it to the deframer via dataReturnOut.
             if (this->isConnected_fileOut_OutputPort(0)) {
-                // Copy buffer into a new allocated buffer. This lets us return the original buffer with dataReturnOut,
-                // and FprimeRouter can handle the deallocation of the file buffer when it returns on fileBufferReturnIn
-                Fw::Buffer packetBufferCopy = this->bufferAllocate_out(0, packetBuffer.getSize());
-                // Confirm we got a valid buffer before using it
-                if (packetBufferCopy.isValid()) {
-                    auto copySerializer = packetBufferCopy.getSerializer();
-                    status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
-                                                          Fw::Serialization::OMIT_LENGTH);
-                    FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
-                    // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done
-                    // with it
-                    this->fileOut_out(0, packetBufferCopy);
-                } else {
-                    this->log_WARNING_HI_AllocationError(FprimeRouter_AllocationReason::FILE_UPLINK);
-                }
+                this->fileOut_out(0, packetBuffer);
+            } else {
+                // Port not connected, return the buffer immediately with an empty context
+                ComCfg::FrameContext emptyContext;
+                this->dataReturnOut_out(0, packetBuffer, emptyContext);
             }
             break;
         }
         default: {
             // Packet type is not known to the F Prime protocol. If the unknownDataOut port is
-            // connected, forward packet and context for further processing
+            // connected, forward packet and context for further processing.
+            // Ownership is passed to the receiver and will come back on fileBufferReturnIn,
+            // at which point we return it to the deframer via dataReturnOut.
             if (this->isConnected_unknownDataOut_OutputPort(0)) {
-                // Copy buffer into a new allocated buffer. This lets us return the original buffer with dataReturnOut,
-                // and FprimeRouter can handle the deallocation of the unknown buffer when it returns on bufferReturnIn
-                Fw::Buffer packetBufferCopy = this->bufferAllocate_out(0, packetBuffer.getSize());
-                // Confirm we got a valid buffer before using it
-                if (packetBufferCopy.isValid()) {
-                    auto copySerializer = packetBufferCopy.getSerializer();
-                    status = copySerializer.serializeFrom(packetBuffer.getData(), packetBuffer.getSize(),
-                                                          Fw::Serialization::OMIT_LENGTH);
-                    FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
-                    // Send the copied buffer out. It will come back on fileBufferReturnIn once the receiver is done
-                    // with it
-                    this->unknownDataOut_out(0, packetBufferCopy, context);
-                } else {
-                    this->log_WARNING_HI_AllocationError(FprimeRouter_AllocationReason::USER_BUFFER);
-                }
+                this->unknownDataOut_out(0, packetBuffer, context);
+            } else {
+                // Port not connected, return the buffer immediately with an empty context
+                ComCfg::FrameContext emptyContext;
+                this->dataReturnOut_out(0, packetBuffer, emptyContext);
             }
             break;
         }
     }
-
-    // Return ownership of the incoming packetBuffer
-    this->dataReturnOut_out(0, packetBuffer, context);
 }
 
 void FprimeRouter ::cmdResponseIn_handler(FwIndexType portNum,
@@ -102,7 +86,9 @@ void FprimeRouter ::cmdResponseIn_handler(FwIndexType portNum,
 }
 
 void FprimeRouter ::fileBufferReturnIn_handler(FwIndexType portNum, Fw::Buffer& fwBuffer) {
-    this->bufferDeallocate_out(0, fwBuffer);
+    // Return ownership of the buffer to the deframer with an empty context
+    ComCfg::FrameContext context;
+    this->dataReturnOut_out(0, fwBuffer, context);
 }
 
 }  // namespace Svc

Svc/FprimeRouter/FprimeRouter.fpp

@@ -7,22 +7,11 @@ module Svc {
         # ----------------------------------------------------------------------
         import Router
 
-        enum AllocationReason : U8{
-            FILE_UPLINK,  @< Buffer allocation for file uplink
-            USER_BUFFER   @< Buffer allocation for user handled buffer
-        }
-
         @ Port for forwarding non-recognized packet types
-        @ Ownership of the buffer is retained by the FprimeRouter, meaning receiving
-        @ components should either process data synchronously, or copy the data if needed
+        @ Ownership of the buffer is passed to the receiver. The receiver must return
+        @ the buffer via fileBufferReturnIn when done.
         output port unknownDataOut: Svc.ComDataWithContext
 
-        @ Port for allocating buffers
-        output port bufferAllocate: Fw.BufferGet
-
-        @ Port for deallocating buffers
-        output port bufferDeallocate: Fw.BufferSend
-
         @ An error occurred while serializing a com buffer
         event SerializationError(
                 status: U32 @< The status of the operation
@@ -37,11 +26,6 @@ module Svc {
             severity warning high \
             format "Deserializing packet type failed with status {}"
 
-        @ An allocation error occurred
-        event AllocationError(reason: AllocationReason) severity warning high \
-            format "Buffer allocation for {} failed"
-
-
         ###############################################################################
         # Standard AC Ports for Events 
         ###############################################################################

Svc/FprimeRouter/docs/sdd.md

@@ -6,11 +6,13 @@ The `Svc::FprimeRouter` component receives F´ packets (as [Fw::Buffer](../../..
 
 The `Svc::FprimeRouter` component supports `Fw::ComPacketType::FW_PACKET_COMMAND` and `Fw::ComPacketType::FW_PACKET_FILE` packet types. Unknown packet types are forwarded on the `unknownDataOut` port, which a project-specific component can connect to for custom routing.
 
-About memory management, all buffers sent by `Svc::FprimeRouter` on the `fileOut` and `unknownDataOut` ports are expected to be returned to the router through the `fileBufferReturnIn` port for deallocation.
+About memory management, buffers sent by `Svc::FprimeRouter` on the `fileOut` and `unknownDataOut` ports are passed through directly without copying. Receivers of these buffers **must** return them to `Svc::FprimeRouter` through the `fileBufferReturnIn` port when finished processing. The original buffer is not returned to the deframer until this happens.
+
+**Note:** The `FrameContext` received on `dataIn` is not preserved across the buffer return path. When buffers are returned via `fileBufferReturnIn`, `Svc::FprimeRouter` constructs an empty `ComCfg::FrameContext` for the `dataReturnOut` call. This applies to all packet types — the context is always discarded on return.
 
 ## Custom Routing
 
-The `Svc::FprimeRouter` component is designed to be extensible through the use of a project-specific router. The `unknownDataOut` port can be connected to a project-specific component that can receive all unknown packet types. This component can then implement custom handling of these unknown packets. After processing, the project-specific component shall return the received buffer to the `Svc::FprimeRouter` component through the `fileBufferReturnIn` port (named this way as it only receives file packets in the common use-case), which will deallocate the buffer.
+The `Svc::FprimeRouter` component is designed to be extensible through the use of a project-specific router. The `unknownDataOut` port can be connected to a project-specific component that can receive all unknown packet types. This component can then implement custom handling of these unknown packets. After processing, the project-specific component shall return the received buffer to the `Svc::FprimeRouter` component through the `fileBufferReturnIn` port (named this way as it only receives file packets in the common use-case), which will return the buffer to the deframer.
 
 ## Usage Examples
 
@@ -32,8 +34,6 @@ In the canonical uplink communications stack, `Svc::FprimeRouter` is connected t
 | `output` | `fileOut` | `Fw.BufferSend` | Port for sending file packets as Fw::Buffer (ownership passed to receiver) |
 | `sync input` | `fileBufferReturnIn` | `Fw.BufferSend` | Receiving back ownership of buffer sent on `fileOut` and `unknownDataOut` | 
 | `output` | `unknownDataOut` | `Svc.ComDataWithContext` | Port forwarding unknown data (useful for adding custom routing rules with a  project-defined router) |
-| `output`| `bufferAllocate` | `Fw.BufferGet` | Port for allocating buffers, allowing copy of received data |
-| `output`| `bufferDeallocate` | `Fw.BufferSend` | Port for deallocating buffers |
 
 ## Requirements
 
@@ -44,5 +44,5 @@ SVC-ROUTER-002 | `Svc::FprimeRouter` shall route packets of type `Fw::ComPacketT
 SVC-ROUTER-003 | `Svc::FprimeRouter` shall route packets of type `Fw::ComPacketType::FW_PACKET_FILE` to the `fileOut` output port. | Routing file packets | Unit test |
 SVC-ROUTER-004 | `Svc::FprimeRouter` shall route data that is neither `Fw::ComPacketType::FW_PACKET_COMMAND` nor `Fw::ComPacketType::FW_PACKET_FILE` to the `unknownDataOut` output port. | Allows for projects to provide custom routing for additional (project-specific) uplink data types | Unit test |
 SVC-ROUTER-005 | `Svc::FprimeRouter` shall emit warning events if serialization errors occur during processing of incoming packets | Aid in diagnosing uplink issues | Unit test |
-SVC-ROUTER-005 | `Svc::FprimeRouter` shall make a copy of buffers that represent a `FW_PACKET_FILE` | Aid in memory management of file buffers | Unit test |
-SVC-ROUTER-005 | `Svc::FprimeRouter` shall return ownership of all buffers received on `dataIn` through `dataReturnOut` | Memory management | Unit test |
+SVC-ROUTER-006 | `Svc::FprimeRouter` shall pass through buffers for `FW_PACKET_FILE` and unknown packet types without copying, and defer returning them to the deframer until they are returned via `fileBufferReturnIn` | Efficient memory management | Unit test |
+SVC-ROUTER-007 | `Svc::FprimeRouter` shall return ownership of all buffers received on `dataIn` through `dataReturnOut` | Memory management | Unit test |

Svc/FprimeRouter/test/ut/FprimeRouterTestMain.cpp

@@ -28,18 +28,8 @@ TEST(FprimeRouter, TestRouteUnknownPacketUnconnected) {
     Svc::FprimeRouterTester tester(true);
     tester.testRouteUnknownPacketUnconnected();
 }
-TEST(FprimeRouter, TestAllocationFailureFile) {
-    COMMENT("Test failure to allocate for files");
-    Svc::FprimeRouterTester tester;
-    tester.testAllocationFailureFile();
-}
-TEST(FprimeRouter, TestAllocationFailureUnknown) {
-    COMMENT("Test failure to allocate for unknown packets");
-    Svc::FprimeRouterTester tester;
-    tester.testAllocationFailureUnknown();
-}
 TEST(FprimeRouter, TestBufferReturn) {
-    COMMENT("Deallocate a returning buffer");
+    COMMENT("Return a buffer via fileBufferReturnIn");
     Svc::FprimeRouterTester tester;
     tester.testBufferReturn();
 }

Svc/FprimeRouter/test/ut/FprimeRouterTester.cpp

@@ -34,25 +34,22 @@ void FprimeRouterTester ::testRouteComInterface() {
     ASSERT_from_fileOut_SIZE(0);         // no file packet emitted
     ASSERT_from_unknownDataOut_SIZE(0);  // no unknown data emitted
     ASSERT_from_dataReturnOut_SIZE(1);   // data ownership should always be returned
-    ASSERT_from_bufferAllocate_SIZE(0);  // no buffer allocation for Com packets
 }
 
 void FprimeRouterTester ::testRouteFileInterface() {
     this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_FILE);
     ASSERT_from_commandOut_SIZE(0);      // no command packet emitted
     ASSERT_from_fileOut_SIZE(1);         // one file packet emitted
     ASSERT_from_unknownDataOut_SIZE(0);  // no unknown data emitted
-    ASSERT_from_dataReturnOut_SIZE(1);   // data ownership should always be returned
-    ASSERT_from_bufferAllocate_SIZE(1);  // file packet was copied into a new allocated buffer
+    ASSERT_from_dataReturnOut_SIZE(0);   // data ownership is not returned yet (will come back on fileBufferReturnIn)
 }
 
 void FprimeRouterTester ::testRouteUnknownPacket() {
     this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_UNKNOWN);
     ASSERT_from_commandOut_SIZE(0);      // no command packet emitted
     ASSERT_from_fileOut_SIZE(0);         // no file packet emitted
     ASSERT_from_unknownDataOut_SIZE(1);  // one unknown data emitted
-    ASSERT_from_dataReturnOut_SIZE(1);   // data ownership should always be returned
-    ASSERT_from_bufferAllocate_SIZE(1);  // unknown packet was copied into a new allocated buffer
+    ASSERT_from_dataReturnOut_SIZE(0);   // data ownership is not returned yet (will come back on fileBufferReturnIn)
 }
 
 void FprimeRouterTester ::testRouteUnknownPacketUnconnected() {
@@ -61,32 +58,15 @@ void FprimeRouterTester ::testRouteUnknownPacketUnconnected() {
     ASSERT_from_fileOut_SIZE(0);         // no file packet emitted
     ASSERT_from_unknownDataOut_SIZE(0);  // zero unknown data emitted when port is unconnected
     ASSERT_from_dataReturnOut_SIZE(1);   // data ownership should always be returned
-    ASSERT_from_bufferAllocate_SIZE(0);  // no buffer allocation when port is unconnected
-}
-
-void FprimeRouterTester ::testAllocationFailureFile() {
-    this->m_forceAllocationError = true;
-    this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_FILE);
-    ASSERT_EVENTS_AllocationError_SIZE(1);  // allocation error should be logged
-    ASSERT_EVENTS_AllocationError(0, FprimeRouter_AllocationReason::FILE_UPLINK);
-    ASSERT_from_dataReturnOut_SIZE(1);  // data ownership should always be returned
-}
-
-void FprimeRouterTester ::testAllocationFailureUnknown() {
-    this->m_forceAllocationError = true;
-    this->mockReceivePacketType(Fw::ComPacketType::FW_PACKET_UNKNOWN);
-    ASSERT_EVENTS_AllocationError_SIZE(1);  // allocation error should be logged
-    ASSERT_EVENTS_AllocationError(0, FprimeRouter_AllocationReason::USER_BUFFER);
-    ASSERT_from_dataReturnOut_SIZE(1);  // data ownership should always be returned
 }
 
 void FprimeRouterTester ::testBufferReturn() {
     U8 data[1];
     Fw::Buffer buffer(data, sizeof(data));
     this->invoke_to_fileBufferReturnIn(0, buffer);
-    ASSERT_from_bufferDeallocate_SIZE(1);  // incoming buffer should be deallocated
-    ASSERT_EQ(this->fromPortHistory_bufferDeallocate->at(0).fwBuffer.getData(), data);
-    ASSERT_EQ(this->fromPortHistory_bufferDeallocate->at(0).fwBuffer.getSize(), sizeof(data));
+    ASSERT_from_dataReturnOut_SIZE(1);  // buffer should be returned via dataReturnOut
+    ASSERT_EQ(this->fromPortHistory_dataReturnOut->at(0).data.getData(), data);
+    ASSERT_EQ(this->fromPortHistory_dataReturnOut->at(0).data.getSize(), sizeof(data));
 }
 
 void FprimeRouterTester ::testCommandResponse() {
@@ -120,27 +100,9 @@ void FprimeRouterTester::connectPortsExceptUnknownData() {
     this->connect_to_dataIn(0, this->component.get_dataIn_InputPort(0));
     this->connect_to_fileBufferReturnIn(0, this->component.get_fileBufferReturnIn_InputPort(0));
     // Connect typed output ports
-    this->component.set_bufferAllocate_OutputPort(0, this->get_from_bufferAllocate(0));
-    this->component.set_bufferDeallocate_OutputPort(0, this->get_from_bufferDeallocate(0));
     this->component.set_commandOut_OutputPort(0, this->get_from_commandOut(0));
     this->component.set_dataReturnOut_OutputPort(0, this->get_from_dataReturnOut(0));
     this->component.set_fileOut_OutputPort(0, this->get_from_fileOut(0));
 }
 
-// ----------------------------------------------------------------------
-// Port handler overrides
-// ----------------------------------------------------------------------
-Fw::Buffer FprimeRouterTester::from_bufferAllocate_handler(FwIndexType portNum, FwSizeType size) {
-    this->pushFromPortEntry_bufferAllocate(size);
-    if (this->m_forceAllocationError) {
-        this->m_buffer.setData(nullptr);
-        this->m_buffer.setSize(0);
-    } else {
-        this->m_buffer.setData(this->m_buffer_slot);
-        this->m_buffer.setSize(size);
-        ::memset(this->m_buffer.getData(), 0, size);
-    }
-    return this->m_buffer;
-}
-
 }  // namespace Svc

Svc/FprimeRouter/test/ut/FprimeRouterTester.hpp

@@ -56,13 +56,7 @@ class FprimeRouterTester : public FprimeRouterGTestBase {
     //! Route a packet of unknown type
     void testRouteUnknownPacketUnconnected();
 
-    //! Route a packet of unknown type
-    void testAllocationFailureFile();
-
-    //! Deallocate a returning buffer
-    void testAllocationFailureUnknown();
-
-    //! Deallocate a returning buffer
+    //! Test buffer return via fileBufferReturnIn
     void testBufferReturn();
 
     //! Invoke the command response input port
@@ -85,23 +79,13 @@ class FprimeRouterTester : public FprimeRouterGTestBase {
     //! Mock the reception of a packet of a specific type
     void mockReceivePacketType(Fw::ComPacketType packetType);
 
-    // ----------------------------------------------------------------------
-    // Port handler overrides
-    // ----------------------------------------------------------------------
-    //! Overriding bufferAllocate handler to be able to request a buffer in component tests
-    Fw::Buffer from_bufferAllocate_handler(FwIndexType portNum, FwSizeType size) override;
-
   private:
     // ----------------------------------------------------------------------
     // Member variables
     // ----------------------------------------------------------------------
 
     //! The component under test
     FprimeRouter component;
-
-    Fw::Buffer m_buffer;  // buffer to be returned by mocked bufferAllocate call
-    U8 m_buffer_slot[64];
-    bool m_forceAllocationError = false;  // Flag to force allocation error
 };
 
 }  // namespace Svc

Svc/Interfaces/Router.fpp

@@ -17,7 +17,8 @@ module Svc {
         @ Port for sending file packets as Fw::Buffer (ownership passed to receiver)
         output port fileOut: Fw.BufferSend
 
-        @ Port for receiving ownership back of buffers sent on fileOut
+        @ Port for receiving back ownership of buffers sent on fileOut or any other
+        @ output port that passes buffer ownership to the receiver
         sync input port fileBufferReturnIn: Fw.BufferSend
 
         @ Port for sending command packets as Fw::ComBuffers

Svc/Subtopologies/ComCcsds/ComCcsds.fpp

@@ -177,9 +177,6 @@ module ComCcsds {
             # SpacePacketDeframer <-> Router
             spacePacketDeframer.dataOut -> fprimeRouter.dataIn
             fprimeRouter.dataReturnOut  -> spacePacketDeframer.dataReturnIn
-            # Router buffer allocations
-            fprimeRouter.bufferAllocate   -> commsBufferManager.bufferGetCallee
-            fprimeRouter.bufferDeallocate -> commsBufferManager.bufferSendIn
         }
     } # end FramingSubtopology
 

Svc/Subtopologies/ComFprime/ComFprime.fpp

@@ -141,9 +141,6 @@ module ComFprime {
             # Deframer <-> Router
             deframer.dataOut           -> fprimeRouter.dataIn
             fprimeRouter.dataReturnOut -> deframer.dataReturnIn
-            # Router buffer allocations
-            fprimeRouter.bufferAllocate   -> commsBufferManager.bufferGetCallee
-            fprimeRouter.bufferDeallocate -> commsBufferManager.bufferSendIn
         }
     } # end FramingSubtopology