Split Deframer into FrameAccumulator and Router #2900
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| class CRCWrapper { | ||
| protected: | ||
| //! \brief constructor setting initial value | ||
| CRCWrapper(TokenType initial) : m_crc(initial) {} |
Check warning
Code scanning / CppCheck
Single-parameter constructors should be marked explicit. Warning
thomas-bc
added
the
Update Instructions Needed
There was a problem hiding this comment.
check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| class CRCWrapper { | ||
| protected: | ||
| //! \brief constructor setting initial value | ||
| CRCWrapper(TokenType initial) : m_crc(initial) {} |
Check warning
Code scanning / CppCheck
Single-parameter constructors should be marked explicit.
thomas-bc
changed the title
|
Will fix #2073 |
|
@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 |
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
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)?
| // No frame was detected or an unknown status was received | ||
| else { | ||
| // Discard a single byte of data and start again | ||
| m_inRing.rotate(1); |
There was a problem hiding this comment.
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.
| output port frameAllocate: Fw.BufferGet | ||
|
|
||
| @ Port for sending an extracted frame out | ||
| output port frameOut: Fw.DataWithContext |
There was a problem hiding this comment.
Why do we send DataWithContext if there's always no context?
| //! | ||
| //! \param data: circular buffer with read-only access | ||
| //! \param size_out: set as output to caller indicating size when appropriate | ||
| //! \return status of the detection to be pared with size_out |
|
|
||
| protected: | ||
| TokenType m_value; | ||
| }; |
There was a problem hiding this comment.
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.
| } | ||
| // TODO: add asserts? | ||
| data.setData(data.getData() + FrameConfig::HEADER_SIZE); | ||
| data.setSize(data.getSize() - FrameConfig::HEADER_SIZE - FrameConfig::CHECKSUM_SIZE); |
There was a problem hiding this comment.
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.
| Fw::Buffer buffer = this->frameAllocate_out(0, size_out); | ||
| if (buffer.isValid()) { | ||
| // Copy out data and rotate | ||
| FW_ASSERT(this->m_inRing.peek(buffer.getData(), size_out) == Fw::SerializeStatus::FW_SERIALIZE_OK); |
There was a problem hiding this comment.
I'm worried about the performance overhead of this additional copy. In the case that a custom protocol requires transformation of the received data, the decision to split out FrameAccumulator and Deframer means that we'll have to yet another copy in what is already a many-copy architecture.
Here are the copies:
- The driver loads the received data bytes from the device into a buffer and passes the buffer to FrameAccumulator.
- FrameAccumulator copies the bytes into its internal ring buffer.
- FrameAccumulator copies the bytes out of its internal ring buffer into a new allocated frame. (This is the extra copy added by the new model, compared to the existing Deframer.) This frame is passed to the custom deframer.
- The custom deframer copies the bytes into a new buffer (or reuses the existing buffer) as it translates the received data. The resulting decoded packet is passed onward.
- The receiving component processes the data and copies it to its final destination, for example saving the data into the filesystem.
I like the idea of splitting out the Router from the Deframer, but I'm not so sure I like the idea of splitting out the FrameAccumulator from the Deframer, at least not in this way.
Also, while I know that F Prime does not discard the condition of an assertion when assertions are disabled, it still unnerves me that we have an assertion condition with a side effect, because that's dangerous in many frameworks.
There was a problem hiding this comment.
The decision to split out the Accumulator was done to provide support for projects that receive a complete packet from the radio. This allows them to push the buffer from the driver straight through the deframer chain, removing both the copy-in and copy-out of the ring buffer.
I do see your point, that when a rearranging of the incoming data is needed, copying to a buffer only to copy again to rearrange adds overhead.
Change Description
Split up Deframer into 3 components:
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
!!! Unit tests, SDDs, update documentation
!!! UPDATE
fprime-util new --deployment