Split Deframer into FrameAccumulator and Router #2900

thomas-bc · 2024-09-25T21:18:58Z


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

Change Description

Split up Deframer into 3 components:

FrameAccumulator
Deframer
Router (rename UplinkRouter ?)

Using this new structure, adding in 2 new deframers for CCSDS SpacePacket and TM protocols.

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/StartLengthCrcDetector.hpp

+class CRCWrapper {
+  protected:
+    //! \brief constructor setting initial value
+    CRCWrapper(TokenType initial) : m_crc(initial) {}


github-advanced-security

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

Svc/FrameAccumulator/FrameDetector/StartLengthCrcDetector.hpp

+class CRCWrapper {
+  protected:
+    //! \brief constructor setting initial value
+    CRCWrapper(TokenType initial) : m_crc(initial) {}


thomas-bc · 2024-10-16T17:46:32Z

Will fix #2073

ReggieMarr · 2024-11-13T18:21:08Z

@thomas-bc @LeStarch I made a fork and kinda ran with it in my repo I found what I think is an issue with the frame detection here

Currently I'm workin to fix my TM framer impl but let me know if I'm stepping on anyones toes or going in a bad direction

celskeggs

Thanks for putting together these changes, @thomas-bc. I like the splitting out of the Router from the Deframer, but I'm not so sure I like splitting out the FrameAccumulator from the Deframer; this will actually make it less likely that I can use these stock components in my design. More details in individual comments.

celskeggs · 2024-11-19T18:31:31Z

Svc/FrameAccumulator/FrameAccumulator.cpp

+        remaining -= serSize;
+    }
+    // Either all the bytes from the data buffer must be processed, or the ring must be full
+    FW_ASSERT(remaining == 0 || this->m_inRing.get_free_size() == 0, static_cast<FwAssertArgType>(remaining));


Does this mean that data will be silently dropped under some circumstances? Can we have a way to make it not silent (either a FATAL or some other kind of response)?

celskeggs · 2024-11-19T18:33:35Z

Svc/FrameAccumulator/FrameAccumulator.cpp

+            // No frame was detected or an unknown status was received
+            else {
+                // Discard a single byte of data and start again
+                m_inRing.rotate(1);


What if we know how much data is invalid and should be discarded? Stepping through one byte at a time is likely to be needlessly slow. Yes, some frame detectors won't know how many bytes to discard, but some will.

celskeggs · 2024-11-19T18:35:03Z

Svc/FrameAccumulator/FrameAccumulator.fpp

+        output port frameAllocate: Fw.BufferGet
+
+        @ Port for sending an extracted frame out
+        output port frameOut: Fw.DataWithContext


Why do we send DataWithContext if there's always no context?

Svc/FrameAccumulator/FrameDetector.hpp

celskeggs · 2024-11-19T18:46:45Z

Svc/FrameAccumulator/FrameDetector/StartLengthCrcDetector.hpp

+
+  protected:
+    TokenType m_value;
+};


I don't know if this is my place to comment, since I won't be using the StartLengthCrcDetector, but I do want to say that I think all the templates in this file are needlessly convoluted. I don't see what benefit all of this abstraction adds. If you want to make it easy to implement a custom Start-Length-CRC detector, I'd just provide example code and tell people to copy it. It shouldn't be very complicated.

celskeggs · 2024-11-19T18:50:43Z

Svc/Deframer/Deframer.cpp

-    }
+    // TODO: add asserts?
+    data.setData(data.getData() + FrameConfig::HEADER_SIZE);
+    data.setSize(data.getSize() - FrameConfig::HEADER_SIZE - FrameConfig::CHECKSUM_SIZE);


I suspect that this isn't going to be something you'll change, but I do want to note that modifying the buffer passed in as a reference seems dangerous. What if the caller didn't expect its buffer pointer and size to change? Yes, the caller should probably realize this could happen and not rely on it (especially since it should expect the data in the buffer to change anyway), but it still makes me nervous.

Svc/FrameAccumulator/FrameAccumulator.cpp

Svc/Deframer/Deframer.cpp

-{
-    (void) memset(m_pollBuffer, 0, sizeof m_pollBuffer);
-}
+Deframer ::Deframer(const char* const compName) : DeframerComponentBase(compName) {}


Svc/Deframer/Deframer.cpp

-        DeframingProtocol::DEFRAMING_STATUS_SUCCESS;
-    // The ring buffer capacity
-    const NATIVE_UINT_TYPE ringCapacity = m_inRing.get_capacity();
+void Deframer ::framedIn_handler(FwIndexType portNum, Fw::Buffer& data, Fw::Buffer& context) {


Svc/FrameAccumulator/FrameAccumulator.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+FrameAccumulator ::FrameAccumulator(const char* const compName) : FrameAccumulatorComponentBase(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.hpp

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


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,


Svc/Deframer/Deframer.cpp


+    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);
+    U8* data = reinterpret_cast<U8*>(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);
+    U8* data = reinterpret_cast<U8*>(allocator.allocate(allocationId, store_size_int, recoverable));


Svc/FrameAccumulator/FrameAccumulator.cpp

+    FW_ASSERT(store_size_int >= store_size);
+    m_inRing.setup(data, store_size_int);
+
+    this->m_detector = &detector;


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/FrameAccumulator.cpp

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


Svc/FrameAccumulator/FrameAccumulator.cpp

+}
+
+void FrameAccumulator ::processBuffer(Fw::Buffer& buffer) {
+    const U32 bufferSize = buffer.getSize();


Svc/FrameAccumulator/FrameAccumulator.cpp

+                    // Copy out data and rotate
+                    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/FrameAccumulator.cpp

+            // No frame was detected or an unknown status was received
+            else {
+                // Discard a single byte of data and start again
+                m_inRing.rotate(1);


Svc/Router/Router.cpp

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


Svc/Deframer/Deframer.cpp


+    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);
+    U8* data = reinterpret_cast<U8*>(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);
+    U8* data = reinterpret_cast<U8*>(allocator.allocate(allocationId, store_size_int, recoverable));


Svc/FrameAccumulator/FrameAccumulator.cpp

+    FW_ASSERT(store_size_int >= store_size);
+    m_inRing.setup(data, store_size_int);
+
+    this->m_detector = &detector;


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/FrameAccumulator.cpp

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


Svc/FrameAccumulator/FrameAccumulator.cpp

+}
+
+void FrameAccumulator ::processBuffer(Fw::Buffer& buffer) {
+    const U32 bufferSize = buffer.getSize();


Svc/FrameAccumulator/FrameAccumulator.cpp

+                    // Copy out data and rotate
+                    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/FrameAccumulator.cpp

+            // No frame was detected or an unknown status was received
+            else {
+                // Discard a single byte of data and start again
+                m_inRing.rotate(1);


Svc/Router/Router.cpp

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


Svc/Deframer/Deframer.cpp

-{
-    (void) memset(m_pollBuffer, 0, sizeof m_pollBuffer);
-}
+Deframer ::Deframer(const char* const compName) : DeframerComponentBase(compName) {}


Svc/FrameAccumulator/FrameAccumulator.cpp

+// ----------------------------------------------------------------------
+
+FrameAccumulator ::FrameAccumulator(const char* const compName) : FrameAccumulatorComponentBase(compName),
+    m_detector(nullptr), m_memoryAllocator(nullptr), m_memory(nullptr), m_allocatorId(0) {}


Svc/FrameAccumulator/FrameAccumulator.cpp

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


Svc/FrameAccumulator/FrameAccumulator.cpp

+    this->dataDeallocate_out(0, buffer);
+}
+
+void FrameAccumulator ::processBuffer(Fw::Buffer& buffer) {


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() {


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/Deframer/Deframer.cpp

-{
-    (void) memset(m_pollBuffer, 0, sizeof m_pollBuffer);
-}
+Deframer ::Deframer(const char* const compName) : DeframerComponentBase(compName) {}


Svc/FrameAccumulator/FrameAccumulator.cpp

+// ----------------------------------------------------------------------
+
+FrameAccumulator ::FrameAccumulator(const char* const compName) : FrameAccumulatorComponentBase(compName),
+    m_detector(nullptr), m_memoryAllocator(nullptr), m_memory(nullptr), m_allocatorId(0) {}


Svc/FrameAccumulator/FrameAccumulator.cpp

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


Svc/FrameAccumulator/FrameAccumulator.cpp

+    this->dataDeallocate_out(0, buffer);
+}
+
+void FrameAccumulator ::processBuffer(Fw::Buffer& buffer) {


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() {


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/Deframer/Deframer.cpp

-{
-    (void) memset(m_pollBuffer, 0, sizeof m_pollBuffer);
-}
+Deframer ::Deframer(const char* const compName) : DeframerComponentBase(compName) {}


Svc/Deframer/Deframer.cpp

-        DeframingProtocol::DEFRAMING_STATUS_SUCCESS;
-    // The ring buffer capacity
-    const NATIVE_UINT_TYPE ringCapacity = m_inRing.get_capacity();
+void Deframer ::framedIn_handler(FwIndexType portNum, Fw::Buffer& data, Fw::Buffer& context) {


Svc/FrameAccumulator/FrameAccumulator.cpp

+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+FrameAccumulator ::FrameAccumulator(const char* const compName) : FrameAccumulatorComponentBase(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.hpp

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


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,


thomas-bc · 2025-02-06T23:15:18Z

After further review, we're going to try to move away from the templated FrameDetectors and instead leverage FPP to specify the header format, and leverage the struct's serializing functions to do the grunt work.

Closing for now so as not to spam CI and yall with notifications - will reopen when we're closer to done

thomas-bc and others added 19 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

github-advanced-security bot found potential problems Sep 25, 2024

View reviewed changes

thomas-bc added the Update Instructions Needed Need to add instructions in the release notes for updates. label Sep 25, 2024

Remove CCSDS Deframers

e6458d3

github-advanced-security bot found potential problems Sep 30, 2024

View reviewed changes

thomas-bc mentioned this pull request Sep 30, 2024

Build uplink router #2765

Open

This was linked to issues Sep 30, 2024

Break Apart Deframer #2763

Open

Build uplink router #2765

Open

Build A Frame Reassembler #2764

Open

thomas-bc changed the title ~~Add FrameAccumulator, Router and CCSDS deframers~~ Split Deframer into FrameAccumulator and Router Sep 30, 2024

Merge branch 'devel' into preliminary-ccsds-work

0da8df1

LeStarch added this to the Release v3.5.1 milestone Oct 7, 2024

celskeggs suggested changes Nov 19, 2024

View reviewed changes

thomas-bc self-assigned this Nov 20, 2024

thomas-bc modified the milestones: Release v3.5.1, Release v3.6.0 Dec 3, 2024

thomas-bc added 7 commits January 22, 2025 09:42

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

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

View reviewed changes

thomas-bc added 5 commits February 3, 2025 14:19

Spelling

82fcd9b

Add FprimeFrameDetector UTs

9007b8e

Fix CI issue

78e1aad

Fix test component creation

5a5ba8a

Add first pass at SDDs

99836d6

thomas-bc closed this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split Deframer into FrameAccumulator and Router #2900

Split Deframer into FrameAccumulator and Router #2900

thomas-bc commented Sep 25, 2024 •

edited

Loading

github-advanced-security bot left a comment

thomas-bc commented Oct 16, 2024

ReggieMarr commented Nov 13, 2024

celskeggs left a comment

celskeggs Nov 19, 2024

celskeggs Nov 19, 2024

celskeggs Nov 19, 2024

celskeggs Nov 19, 2024

celskeggs Nov 19, 2024

thomas-bc commented Feb 6, 2025


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

		Router ::~Router() {}

Split Deframer into FrameAccumulator and Router #2900

Split Deframer into FrameAccumulator and Router #2900

Conversation

thomas-bc commented Sep 25, 2024 • edited Loading

Change Description

Rationale

TODO

To discuss

github-advanced-security bot left a comment

Choose a reason for hiding this comment

thomas-bc commented Oct 16, 2024

ReggieMarr commented Nov 13, 2024

celskeggs left a comment

Choose a reason for hiding this comment

celskeggs Nov 19, 2024

Choose a reason for hiding this comment

celskeggs Nov 19, 2024

Choose a reason for hiding this comment

celskeggs Nov 19, 2024

Choose a reason for hiding this comment

celskeggs Nov 19, 2024

Choose a reason for hiding this comment

celskeggs Nov 19, 2024

Choose a reason for hiding this comment

thomas-bc commented Feb 6, 2025

thomas-bc commented Sep 25, 2024 •

edited

Loading