Split Deframer into FrameAccumulator Deframer and Router #3250

thomas-bc · 2025-02-20T02:22:10Z


*Related Issue(s)*	#2763
*Has Unit Tests (y/n)*
*Documentation Included (y/n)*

Change Description

(GitHub doesn't let me re-open #2900 for some reason..., so re-opening here)

Implements #2763

Split up Deframer into 3 components:

FrameAccumulator
FprimeDeframer
Router

Rationale

Better reusability and chaining of deframers, preliminary work for CCSDS integrations

TODO

!!! UPDATE fprime-util new --deployment

To discuss

Should we keep old Deframer in (and what to do with config/DeframerCfg.hpp)

Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

Svc/FprimeDeframer/FprimeDeframer.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+FprimeDeframer ::FprimeDeframer(const char* const compName) : FprimeDeframerComponentBase(compName) {}


Svc/FprimeDeframer/FprimeDeframer.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void FprimeDeframer ::framedIn_handler(FwIndexType portNum, Fw::Buffer& data, Fw::Buffer& context) {


Svc/FrameAccumulator/FrameAccumulator.cpp

Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

+namespace Svc {
+namespace FrameDetectors {
+
+FrameDetector::Status FprimeFrameDetector::detect(const Types::CircularBuffer& data, FwSizeType& size_out) const {


Svc/Router/Router.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+Router ::Router(const char* const compName) : RouterComponentBase(compName) {}


Svc/Router/Router.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void Router ::dataIn_handler(NATIVE_INT_TYPE portNum, Fw::Buffer& packetBuffer, Fw::Buffer& contextBuffer) {


Svc/Router/Router.cpp

+    }
+
+    // Whether to deallocate the packet buffer
+    bool deallocate = true;


Svc/Router/Router.cpp

+    }
+}
+
+void Router ::cmdResponseIn_handler(NATIVE_INT_TYPE portNum,


github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Svc/FprimeDeframer/FprimeDeframer.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+FprimeDeframer ::FprimeDeframer(const char* const compName) : FprimeDeframerComponentBase(compName) {}


Svc/FprimeDeframer/FprimeDeframer.cpp

+
+FprimeDeframer ::FprimeDeframer(const char* const compName) : FprimeDeframerComponentBase(compName) {}
+
+FprimeDeframer ::~FprimeDeframer() {}


Svc/FrameAccumulator/FrameAccumulator.cpp

+    }
+    // Deallocate the buffer
+    this->bufferDeallocate_out(0, buffer);
+}


Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

+namespace Svc {
+namespace FrameDetectors {
+
+FrameDetector::Status FprimeFrameDetector::detect(const Types::CircularBuffer& data, FwSizeType& size_out) const {


Svc/Router/Router.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+Router ::Router(const char* const compName) : RouterComponentBase(compName) {}


Svc/Router/Router.cpp

+
+Router ::Router(const char* const compName) : RouterComponentBase(compName) {}
+
+Router ::~Router() {}


Svc/Router/Router.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void Router ::dataIn_handler(NATIVE_INT_TYPE portNum, Fw::Buffer& packetBuffer, Fw::Buffer& contextBuffer) {


Svc/Router/Router.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void Router ::dataIn_handler(NATIVE_INT_TYPE portNum, Fw::Buffer& packetBuffer, Fw::Buffer& contextBuffer) {


Svc/FprimeDeframer/FprimeDeframer.cpp

+    data.setData(data.getData() + FprimeProtocol::FrameHeader::SERIALIZED_SIZE);
+    data.setSize(data.getSize() - FprimeProtocol::FrameHeader::SERIALIZED_SIZE - FprimeProtocol::FrameTrailer::SERIALIZED_SIZE);
+
+    this->deframedOut_out(0, data, context);


Svc/FrameAccumulator/FrameAccumulator.cpp

+    bool recoverable = false;
+    FW_ASSERT(std::numeric_limits<NATIVE_INT_TYPE>::max() >= store_size, static_cast<FwAssertArgType>(store_size));
+    NATIVE_UINT_TYPE store_size_int = static_cast<NATIVE_UINT_TYPE>(store_size);
+    void* data_void = allocator.allocate(allocationId, store_size_int, recoverable);


Svc/FrameAccumulator/FrameAccumulator.cpp

+    bool recoverable = false;
+    FW_ASSERT(std::numeric_limits<NATIVE_INT_TYPE>::max() >= store_size, static_cast<FwAssertArgType>(store_size));
+    NATIVE_UINT_TYPE store_size_int = static_cast<NATIVE_UINT_TYPE>(store_size);
+    void* data_void = allocator.allocate(allocationId, store_size_int, recoverable);


Svc/FrameAccumulator/FrameAccumulator.cpp

+    U8* data = new(data_void) U8[store_size_int];
+    FW_ASSERT(data != nullptr);
+    FW_ASSERT(store_size_int >= store_size);
+    m_inRing.setup(data, store_size_int);


Svc/FrameAccumulator/FrameAccumulator.cpp

+    // Check whether there is data to process
+    if (status.e == Drv::RecvStatus::RECV_OK) {
+        // There is: process the data
+        this->processBuffer(buffer);


Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

Svc/Router/Router.cpp

+    FwPacketDescriptorType packetType = Fw::ComPacket::FW_PACKET_UNKNOWN;
+    Fw::SerializeStatus status = Fw::FW_SERIALIZE_OK;
+    {
+        Fw::SerializeBufferBase& serial = packetBuffer.getSerializeRepr();


Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

+    Utils::HashBuffer hashBuffer;
+
+    // Compute CRC over the transmitted data (header + body)
+    FwSizeType hash_field_size = header.getlength() + FprimeProtocol::FrameHeader::SERIALIZED_SIZE;


Svc/FrameAccumulator/FrameAccumulator.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+FrameAccumulator ::FrameAccumulator(const char* const compName)


Svc/FrameAccumulator/FrameAccumulator.cpp

+}
+
+void FrameAccumulator ::configure(const FrameDetector& detector,
+                                  NATIVE_UINT_TYPE allocationId,


Svc/FrameAccumulator/FrameAccumulator.cpp

+void FrameAccumulator ::configure(const FrameDetector& detector,
+                                  NATIVE_UINT_TYPE allocationId,
+                                  Fw::MemAllocator& allocator,
+                                  FwSizeType store_size) {


Svc/FrameAccumulator/FrameAccumulator.cpp

+            break;
+        }
+        // Compute the size of data to serialize
+        const NATIVE_UINT_TYPE ringFreeSize = this->m_inRing.get_free_size();


Svc/FrameAccumulator/FrameAccumulator.cpp

+        }
+        // Compute the size of data to serialize
+        const NATIVE_UINT_TYPE ringFreeSize = this->m_inRing.get_free_size();
+        const NATIVE_UINT_TYPE serSize =


Svc/FrameAccumulator/FrameAccumulator.cpp

+    // The protocol status
+    FrameDetector::Status status = FrameDetector::Status::FRAME_DETECTED;
+    // The ring buffer capacity
+    const NATIVE_UINT_TYPE ringCapacity = this->m_inRing.get_capacity();


Svc/FrameAccumulator/FrameAccumulator.cpp

+            break;
+        }
+        // size_out is a return variable we initialize to zero, but it should be overwritten
+        FwSizeType size_out = 0;


Svc/FrameAccumulator/FrameAccumulator.hpp

+    U8* m_memory;
+
+    //! Identification used with the memory allocator
+    NATIVE_UINT_TYPE m_allocatorId;


Svc/FrameAccumulator/FrameAccumulator.cpp

+                FW_ASSERT(this->m_inRing.peek(buffer.getData(), static_cast<NATIVE_UINT_TYPE>(size_out)) ==
+                          Fw::SerializeStatus::FW_SERIALIZE_OK);


Svc/FrameAccumulator/FrameAccumulator.cpp

+                FW_ASSERT(this->m_inRing.peek(buffer.getData(), static_cast<NATIVE_UINT_TYPE>(size_out)) ==
+                          Fw::SerializeStatus::FW_SERIALIZE_OK);
+                buffer.setSize(static_cast<U32>(size_out));
+                m_inRing.rotate(static_cast<U32>(size_out));


Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

+    // If the deserialized length token can't fit in current allocated size -> MORE_DATA_NEEDED
+    if (data.get_allocated_size() < FprimeProtocol::FrameHeader::SERIALIZED_SIZE + header.getlength() +
+                                        FprimeProtocol::FrameTrailer::SERIALIZED_SIZE) {
+        size_out = header.getlength() + FprimeProtocol::FrameHeader::SERIALIZED_SIZE +


Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

+        // Add Status::DATA_ERROR , and use it for deserialization errors as well?
+        return Status::NO_FRAME_DETECTED;
+    }
+    size_out = header.getlength() + FprimeProtocol::FrameHeader::SERIALIZED_SIZE +


Svc/FrameAccumulator/FrameAccumulator.cpp

+      m_detector(nullptr),
+      m_memoryAllocator(nullptr),
+      m_memory(nullptr),
+      m_allocatorId(0) {}


Svc/FrameAccumulator/FrameAccumulator.cpp

+    }
+}
+
+void FrameAccumulator ::configure(const FrameDetector& detector,


Svc/FrameAccumulator/FrameAccumulator.cpp

+    FW_ASSERT(remaining == 0 || this->m_inRing.get_free_size() == 0, static_cast<FwAssertArgType>(remaining));
+}
+
+void FrameAccumulator ::processRing() {


Svc/FrameAccumulator/FrameAccumulator.cpp

+    FW_ASSERT(remaining == 0 || this->m_inRing.get_free_size() == 0, static_cast<FwAssertArgType>(remaining));
+}
+
+void FrameAccumulator ::processRing() {


thomas-bc · 2025-02-27T22:32:34Z

Svc/Router/Router.cpp

+                break;
+            }
+            // Take no action for other packet types
+            default:


Would it make sense to emit these on a third output port so that custom routing can be extended easily?

I do like that. Send everything else unknown to a third output port and if connected. WARNING_HI if not connected.

Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp

LeStarch

A lot of minor changes, but this is looking to be in really good shape!

LeStarch · 2025-02-28T00:33:58Z

Ref/Top/topology.fpp

@@ -13,6 +13,7 @@ module Ref {
    enum Ports_StaticMemory {


You aren't using this anymore. Delete it.

LeStarch · 2025-02-28T00:36:02Z

Svc/FprimeDeframer/CMakeLists.txt

+)
+
+set(MOD_DEPS
+    Svc/FramingProtocol


Why is this a dependency?

Should not be anymore - will remove. We should probably capture a follow-up ticket to look into whether we can do some cleanup in the FramingProtocol itself

LeStarch · 2025-02-28T00:48:10Z

config/FpConfig.h

@@ -119,6 +119,11 @@ typedef U32 FwDpIdType;
 typedef U32 FwDpPriorityType;
 #define PRI_FwDpPriorityType PRIu32

+// Type for start word and length word used in fprime framing


Delete this.

LeStarch · 2025-02-28T00:51:27Z

Svc/Interfaces/RouterInterface.fppi

@@ -0,0 +1,8 @@
+@ Receiving data (Fw::Buffer) to be routed with optional context to help with routing
+guarded input port dataIn: Fw.DataWithContext


Since the data is just routed. This could be sync as no modifications of internal data is done. Right?

LeStarch · 2025-02-28T00:53:48Z

Svc/Router/Router.cpp

+                if (status == Fw::FW_SERIALIZE_OK) {
+                    // Send the com buffer
+                    commandOut_out(0, com, 0);
+                    // REVIEW NOTE: Deframer did not check if the output port was connected, should it?


This was done because it was presumed to be an error if data paths weren't connected. Since this is critical for base functionality, leave a comment. Users could always add in a dev-null "cap" component to explicitly discard.

LeStarch · 2025-02-28T01:09:19Z

Svc/FrameAccumulator/FrameAccumulator.fpp

@@ -0,0 +1,17 @@
+module Svc {


For completeness, should this be an interface too?

LeStarch · 2025-02-28T01:10:41Z

Svc/FprimeDeframer/docs/sdd.md

+title: "F Prime Frame Format"
+---
+packet-beta
+  0-31: "Start word: 0xDEADBEEF [4 bytes]"


Technically aren't these intended to be configurable?

LeStarch · 2025-02-28T01:12:32Z

Svc/FrameAccumulator/FrameAccumulator.cpp

@@ -0,0 +1,166 @@
+// ======================================================================


Might need to reimport this code, if and when my PR merges that changes types.

LeStarch · 2025-02-28T01:14:50Z

Svc/FrameAccumulator/FrameAccumulator.cpp

+            FW_ASSERT(size_out != 0);
+            FW_ASSERT(size_out <= remaining, static_cast<FwAssertArgType>(size_out),
+                      static_cast<FwAssertArgType>(remaining));
+            Fw::Buffer buffer = this->bufferAllocate_out(0, static_cast<U32>(size_out));


Need to check for overflow from FwSizeType to U32

LeStarch · 2025-02-28T01:15:49Z

Svc/FrameAccumulator/FrameAccumulator.cpp

+                Fw::Buffer nullContext;
+                this->frameOut_out(0, buffer, nullContext);
+            } else {
+                // No buffer is available, we need to exit and try again later


Should produce an event in this case. Probably a WARNING_HI

thomas-bc and others added 30 commits June 7, 2024 14:11

Split Router out of Deframer - initial implementation

7b65b2e

Rename variable to packetBuffer and resolve conversion warnings

4983004

Add test cases for each incoming packet type

01e62e4

Adjust comments and stuff

450a78a

Update Ref topology with Router

3774af5

WIP - FrameAccumulator

982db63

Add lightweight deframer

e5f1ba2

Working frame detector, accumulator, and fprime packet

2192847

CCSDS implementation and better checksums

728a2a3

Fixing overly small start word to use SCID as well

cce1aeb

Fixing frame accumulator for new structure

7c9486a

Minor cleanup to frame detectors

88eba2b

Add CCSDS Deframers and F´ minimal deframer

3d6852e

Clean up comments

7706122

Update Ref topology with Accumulator/Deframer/Router

c7db8e4

Update buffer manager

251caa9

Merge latest devel to get latest changes

1acb643

Upgrade Router.cpp to latest Fw::Logger

44ad2ae

Update RPI deployment to use FrameAccumulator/Router

4b17736

Remove CCSDS Deframers

e6458d3

Merge branch 'devel' into preliminary-ccsds-work

0da8df1

Merge branch 'devel' into preliminary-ccsds-work

6538397

Merge branch 'devel' into preliminary-ccsds-work

006f27e

Merge branch 'devel' into preliminary-ccsds-work

bcde1ee

Add first pass at UTs

44f4292

add buffer accumulation testing

13f8cd1

start SDD and rename author

e2b5426

Fix static analysis warnings and add comments to UTs

5c22baf

Spelling

82fcd9b

Add FprimeFrameDetector UTs

9007b8e

thomas-bc added 6 commits February 18, 2025 16:55

Remove file

892607d

Remove templated detectors

d21a591

Improve test code

85fe64a

Merge branch 'devel' into preliminary-ccsds-work

064be5b

Rename frame Footer to Trailer, and improve SDDs

f95bc79

finish renaming footer to trailer and add implementation note

c02006a

thomas-bc added the Update Instructions Needed Need to add instructions in the release notes for updates. label Feb 20, 2025

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp Fixed Show fixed Hide fixed

Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

thomas-bc added 2 commits February 19, 2025 18:34

Fix spelling and CI warning

c619f98

Fix test code

eb52a3c

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

More CI and spelling fixes

7df46bb

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

thomas-bc and others added 6 commits February 19, 2025 19:20

Add back virtual destructor

8cc6dc7

appease pedantic warnings

cada0b0

Fix declaration ordering for safe destruction

d481d9e

Run formatting and add a few comments

fdfaa0d

Add RouterInterface.fppi and use it in Router

93543dc

Merge remote-tracking branch 'nasa/devel' into preliminary-ccsds-work

325c3f0

github-advanced-security bot found potential problems Feb 27, 2025

View reviewed changes

thomas-bc added a commit to thomas-bc/fprime-tools that referenced this pull request Feb 27, 2025

Update deployment with new uplink stack: nasa/fprime#3250

5d5cd35

thomas-bc requested a review from LeStarch February 27, 2025 01:44

thomas-bc commented Feb 27, 2025

View reviewed changes

Svc/FrameAccumulator/FrameDetector/FprimeFrameDetector.cpp Show resolved Hide resolved

LeStarch requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split Deframer into FrameAccumulator Deframer and Router #3250

Split Deframer into FrameAccumulator Deframer and Router #3250

thomas-bc commented Feb 20, 2025

github-advanced-security bot left a comment

thomas-bc Feb 27, 2025

LeStarch Feb 28, 2025

LeStarch left a comment

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

thomas-bc Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025

LeStarch Feb 28, 2025


		FprimeDeframer ::FprimeDeframer(const char* const compName) : FprimeDeframerComponentBase(compName) {}

		FprimeDeframer ::~FprimeDeframer() {}


		Router ::Router(const char* const compName) : RouterComponentBase(compName) {}

		Router ::~Router() {}

		FW_ASSERT(this->m_inRing.peek(buffer.getData(), static_cast<NATIVE_UINT_TYPE>(size_out)) ==
		Fw::SerializeStatus::FW_SERIALIZE_OK);

		@@ -0,0 +1,8 @@
		@ Receiving data (Fw::Buffer) to be routed with optional context to help with routing
		guarded input port dataIn: Fw.DataWithContext

		@@ -0,0 +1,166 @@
		// ======================================================================

Split Deframer into FrameAccumulator Deframer and Router #3250

Are you sure you want to change the base?

Split Deframer into FrameAccumulator Deframer and Router #3250

Conversation

thomas-bc commented Feb 20, 2025

Change Description

Rationale

TODO

To discuss

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment