Update/framing deframing context #2150

LeStarch · 2023-07-22T00:25:13Z


*Originating Project/Creator*
*Affected Component*
*Affected Architectures(s)*
*Related Issue(s)*
*Has Unit Tests (y/n)*
*Builds Without Errors (y/n)*
*Unit Tests Pass (y/n)*
*Documentation Included (y/n)*

Change Description

This pull-request makes several changes to Framer and Deframer.

Framer now accepts buffers w/ context. This allows for frame protocols that need to metadata to assist in framing. For example: an APID may be passed-in alongside the data.
Framer file buffer type adjustments are handled through context rather than side-band data
Deframer may now create context
Deframer routing functionality has been moved to a new component as only projects using F´ protocol require this routing functionality.

Work:

Rationale

Many projects need additional contextual metadata when framing, but wish to leverage existing Framer and Deframer code. This PR allows this. Many projects also handle routing and don't need routing code commingled with Deframer.

LeStarch · 2023-07-22T00:27:08Z

@LMPS97 @arizvi786 here are the changes you were interested in for Framer/Deframer. You can leverage this work immediately as:

Documentation is not required for you to leverage the implementation
UTs pass for the components that concern you
FprimeRouter is not needed in your project.

LeStarch · 2023-07-22T00:28:14Z

@pcrosemurgy this PR breaks out routing from Deframer. It is possible that you could use these updates as you were specifically disrupted by the combined deframing and routing.

Svc/FprimeRouter/FprimeRouter.cpp

+    // Specifically does nothing
+}
+
+void FprimeRouter ::deframedIn_handler(const NATIVE_INT_TYPE portNum, Fw::Buffer& packetBuffer, Fw::Buffer& context) {


Svc/FprimeRouter/FprimeRouter.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void FprimeRouter ::cmdResponseIn_handler(const NATIVE_INT_TYPE portNum,


Svc/FprimeRouter/FprimeRouter.cpp

+// Construction, initialization, and destruction
+// ----------------------------------------------------------------------
+
+FprimeRouter ::FprimeRouter(const char* const compName) : FprimeRouterComponentBase(compName) {}


Svc/FprimeRouter/FprimeRouter.cpp

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


Svc/Framer/Framer.cpp

+    this->handle_framing(buffer, Fw::Buffer());
+}
+
+void Framer ::bufferAndContextIn_handler(const NATIVE_INT_TYPE portNum, Fw::Buffer& data, Fw::Buffer& context) {


github-advanced-security

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

pcrosemurgy · 2023-07-22T00:38:04Z

@pcrosemurgy this PR breaks out routing from Deframer. It is possible that you could use these updates as you were specifically disrupted by the combined deframing and routing.

@LeStarch Thank you! I'll take a closer look at this!

LMPS97 · 2023-07-25T18:58:44Z

I am getting an error compiling FprimeProtocol.cpp. Please add #include <limits> to it.

LMPS97 · 2023-07-26T02:01:10Z

Svc/Framer/Framer.hpp

@@ -39,11 +39,6 @@ class Framer : public FramerComponentBase, public FramingProtocolInterface {
    Framer(const char* const compName /*!< The component name*/
    );

-    //! Initialize object Framer
-    //!
-    void init(const NATIVE_INT_TYPE instance = 0 /*!< The instance number*/


Why was the init function deleted on framer? Also, why only on framer and not deframer?

init functions are provided by the component base class and are, in older component implementations, improperly shadowed. Thus, we are removing them.

Why not deframer? Oversight. We will remove it there too before merging.

LMPS97 · 2023-08-16T20:51:28Z

I ran into a slight issue with the Framer. Sometimes, I need to send a package out with an empty data buffer. This is because the information is carried out by the header which is created from the context buffer instead. Because of that, the empty buffer is deallocated to the buffer manager which throws an EVR that the buffer size is 0. A solution would be to add a check on the Framer before deallocating the buffer to see if it is a valid buffer in the first place.

LMPS97 · 2023-08-16T22:51:25Z

I just ran into an other issue when unit testing my code. Apparently, if you try to create a Fw::Buffer from a non valid Fw::Buffer using the following constructor causes an assert:

Buffer::Buffer(const Buffer& src) : Serializable(),
    m_serialize_repr(src.m_bufferData, src.m_size),
    m_bufferData(src.m_bufferData),
    m_size(src.m_size),
    m_context(src.m_context)
{}

I encountered this issue when a function of mine returns a non-valid Fw::Buffer.

LMPS97 · 2024-01-02T21:06:07Z

Svc/FramingProtocol/FprimeProtocol.cpp

    FW_ASSERT(m_interface != nullptr);
    // Use of I32 size is explicit as ComPacketType will be specifically serialized as an I32
-    FpFrameHeader::TokenType real_data_size = size + ((packet_type != Fw::ComPacket::FW_PACKET_UNKNOWN) ? sizeof(I32) : 0);
+    FpFrameHeader::TokenType real_data_size = context.getSize() + data.getSize();
    FpFrameHeader::TokenType total = real_data_size + FpFrameHeader::SIZE + HASH_DIGEST_LENGTH;
    Fw::Buffer buffer = m_interface->allocate(total);


There should be a check here to see if the buffer is empty since the buffer manager can run out of buffers. I don't think the check should be an assert since it wouldn't be appropriate to assert flight software on a lack of buffer manager buffers. Therefore, I think it should do an early return with an error code. This error code could be use by the framer and an evr could be outputted.

Agreed wholeheartedly. Although we should check for .isValid() in the latest F´ because when buffer manager has no buffers, an invalid buffer is returned.

LMPS97 · 2024-01-02T21:28:29Z

Svc/FramingProtocol/FprimeProtocol.cpp

@@ -128,7 +128,8 @@ DeframingProtocol::DeframingStatus FprimeDeframing::deframe(Types::CircularBuffe
    buffer.setSize(size);


Similar to the frame function, this check should be a assert but a early return with an error code.

LeStarch · 2024-01-08T22:06:12Z

is deallocated to the buffer manager which throws an EVR that the buffer size is 0. A solution would be to add a check on the Framer before deallocating the buffer to see if it is a valid buffer in the first place.

We should deallocate it when the buffer isValid() and not dealocate otherwise. While the "zero length" EVR is annoying, it would be worse to never return a buffer to buffer manager even when it is zero length. Make sure the "i don''t use this buffers" are invalid and I agree, we should guard the deallocation.

LeStarch added 4 commits July 21, 2023 11:14

Refactoring framer to pass context

bb29e50

Making framer pass file-buffer information through context

43a9815

Adding context to deframer and splitting out fprime routing functions

81ff2d2

Adding FprimeRouter to offload routing functions from Deframer

3bb159d

LeStarch marked this pull request as draft July 22, 2023 00:25

github-advanced-security bot found potential problems Jul 22, 2023

View reviewed changes

LMPS97 reviewed Jul 26, 2023

View reviewed changes

LMPS97 reviewed Jan 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/framing deframing context #2150

Update/framing deframing context #2150

LeStarch commented Jul 22, 2023 •

edited

LeStarch commented Jul 22, 2023

LeStarch commented Jul 22, 2023

github-advanced-security bot left a comment

pcrosemurgy commented Jul 22, 2023 •

edited

LMPS97 commented Jul 25, 2023 •

edited

LMPS97 Jul 26, 2023

LeStarch Jan 8, 2024

LMPS97 commented Aug 16, 2023

LMPS97 commented Aug 16, 2023

LMPS97 Jan 2, 2024

LeStarch Jan 8, 2024

LMPS97 Jan 2, 2024

LeStarch Jan 8, 2024

LeStarch commented Jan 8, 2024

		@@ -128,7 +128,8 @@ DeframingProtocol::DeframingStatus FprimeDeframing::deframe(Types::CircularBuffe
		buffer.setSize(size);

Update/framing deframing context #2150

Are you sure you want to change the base?

Update/framing deframing context #2150

Conversation

LeStarch commented Jul 22, 2023 • edited

Change Description

Rationale

LeStarch commented Jul 22, 2023

LeStarch commented Jul 22, 2023

github-advanced-security bot left a comment

Choose a reason for hiding this comment

pcrosemurgy commented Jul 22, 2023 • edited

LMPS97 commented Jul 25, 2023 • edited

LMPS97 Jul 26, 2023

Choose a reason for hiding this comment

LeStarch Jan 8, 2024

Choose a reason for hiding this comment

LMPS97 commented Aug 16, 2023

LMPS97 commented Aug 16, 2023

LMPS97 Jan 2, 2024

Choose a reason for hiding this comment

LeStarch Jan 8, 2024

Choose a reason for hiding this comment

LMPS97 Jan 2, 2024

Choose a reason for hiding this comment

LeStarch Jan 8, 2024

Choose a reason for hiding this comment

LeStarch commented Jan 8, 2024

LeStarch commented Jul 22, 2023 •

edited

pcrosemurgy commented Jul 22, 2023 •

edited

LMPS97 commented Jul 25, 2023 •

edited