Conversation
& test LoL + header wrapping
packet context overwrite
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| binaryfile | ||
| BINDIR | ||
| bitfield | ||
| Bitfield |
Check warning
Code scanning / check-spelling
Ignored Expect Variant
| endmacro | ||
| endraw | ||
| enduml | ||
| EPP |
Check warning
Code scanning / check-spelling
Ignored Expect Variant
|
This PR should look less scary once #4630 merges |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
thomas-bc
left a comment
thomas-bc
left a comment
There was a problem hiding this comment.
Looks good! A few notes and questions. I'm also overall seeing very few asserts in the processing parts of the code, it'd be good if we add some.
| const FwSizeType headerCap = AosDeframerVc::SpanningPacketState::HEADER_BUF_SIZE; | ||
| const FwSizeType toHeader = FW_MIN(size, headerCap - vc.spanningPacket.bytesReceived); | ||
| if (toHeader > 0) { | ||
| ::memcpy(vc.spanningPacket.headerBuf + vc.spanningPacket.bytesReceived, data, toHeader); |
There was a problem hiding this comment.
The logic is a bit complex to follow here, could we add a few comments? I'm also not entirely certain that there isn't a very niche edge-case in case FwSizeType isn't large, could there be a scenario where:
- you've accumulated a very large spanning packet
headerCap - vc.spanningPacket.bytesReceivedends up wrapping around to a value lower than receivedsize- I think you'd hit an invalid
memcpy()below?
Probably at least should assert both toHeader <= 8 and vc.spanningPacket.bytesReceived < 8 here?
There was a problem hiding this comment.
added those asserts & more more comments
| } | ||
| } | ||
|
|
||
| AosDeframer::~AosDeframer() {} |
Check notice
Code scanning / CodeQL
More than one statement per line Note
|
|
||
| AosDeframer::~AosDeframer() {} | ||
|
|
||
| void AosDeframer::configure(U32 fixedFrameSize, bool frameErrorControlField, U16 spacecraftId, U8 vcId, U8 pvnMask) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| // Handler implementations for user-defined typed input ports | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| void AosDeframer::dataIn_handler(FwIndexType portNum, Fw::Buffer& data, const ComCfg::FrameContext& context) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| } | ||
| } | ||
|
|
||
| void AosDeframer::abandonSpanningPacket(AosDeframerVc& vc) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| return nullptr; | ||
| } | ||
|
|
||
| AosDeframer::AosDeframerVc* AosDeframer::parseAndValidateHeader(Fw::Buffer& data, ComCfg::FrameContext& context) { |
Check notice
Code scanning / CodeQL
Function too long Note
| } | ||
| } | ||
|
|
||
| FwSizeType AosDeframer::sizePacket(AosDeframerVc& vc, U8* packetStart, FwSizeType remainingBytes) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| } | ||
| } | ||
|
|
||
| FwSizeType AosDeframer::sizeSppPacket(U8* payloadStart, FwSizeType payloadSize) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| return totalPacketSize; | ||
| } | ||
|
|
||
| FwSizeType AosDeframer::sizeEppPacket(const U8* const payloadStart, FwSizeType payloadSize) { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| AosDeframerVc* getVcStruct(const U8 vcId); | ||
|
|
||
| //! Per-virtual-channel state, mirroring the AosFramer::AosVc pattern for future multi-VC support | ||
| struct AosDeframerVc { |
Check notice
Code scanning / CodeQL
More than one statement per line Note
|
|
||
| // Spanning packet state (for packets that span multiple frames) | ||
| // Per CCSDS 732.0-B-5 Section 4.1.4.2.2.3 | ||
| struct SpanningPacketState { |
Check notice
Code scanning / CodeQL
More than one statement per line Note
LeStarch
added
Deferred To Future Release
|
I believe that should address all of your comments |
5 participants
Change Description
Add a CCSDS AOS SDL version 5 defamer that supports:
Rationale
You need an AOS Deframer to rx AOS Frames from payloads/other craft.
AOS lets you do fixed frame width w/ variable packet size (including packets that span multiple frames)
Testing/Review Recommendations
Take a look at the processing I'm doing on SPP & EPP packets and what I'm emitting in the context for them. I could imagine a later rev of this component sharing w/ SPP defamer more logic/doing its job for it.
Other facet on the SPP & EPP processing is how light my validation is. I'm only checking that they contain the right PVN & grabbing size + idle. If we moved the full SPP parsing into this/called into a lib then you could maybe avoid throwing away packets by ID-ing a problem earlier (although dropping a frame upon packet issues is probably the only safe thing to do).
A double check on my read of the EPP spec would be beneficial since I caught a few bugs that lingered all the way into testing.
Future Work
I am likely done w/ AOS for near term; however, future work could be:
AI Usage (see policy)
Overall, I'm not convinced the first 2 steps really provided that much of a speedup since much of the EPP structure was completely wrong, and the approach to spanning packets had significant code duplication. Maybe a minor psychological speedup simply because I had code to review to bootstrap rather than a blank slate.
The minor test writing & event creation usage felt like a speedup, but better editor skills/snippets might be able to handily beat that.