Conversation
|
Only uncovered line is the (not yet implemented) EPP Idle packet generation |
thomas-bc
left a comment
There was a problem hiding this comment.
I haven't been able to dive in the code itself too deep yet, as I was also discovering the AOS spec in parallel. I should be able to get to it more thoroughly in the next day or two. But in the meantime, for the sake of iterating more quickly, I figured I'd dump a first set of nits/questions/comments.
This looks great! Love the internal pattern of tracking AosVc's to extend further down the road.
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.
|
yeah those RHEL8 warnings are annoying, but we've seen that before, just need to cast the whole result of the operation... for example |
thomas-bc
left a comment
There was a problem hiding this comment.
Code LGTM; just a few follow-ups and questions in some places.
| vcId: U8 @< 6 bit Virtual Channel ID - used for TC and TM | ||
| vcId: U8 @< 6 bit Virtual Channel ID - used for AOS | ||
| replayFlag: bool @< AOS bool for realtime vs replay (passed directly into the frame) | ||
| sendNow: bool @< Flag to AOS Framer that the Frame this packet goes into should be sent ASAP |
There was a problem hiding this comment.
Could this functionality come in on a separate port instead? What component do you envision would turn that on in the framing chain?
There was a problem hiding this comment.
My thought process is that some sources upstream of the ComQueue might want to request immediate sending, while others would prioritize bandwidth over latency. Since we're already conveying the APID via the context it seems reasonable to me that we'd convey sending immediacy this way too
There was a problem hiding this comment.
General question; are you all planning to use AOS for a specific reason that you would be willing to share?
I have been looking at these standards for a bit but am still out of the loop on use-cases and such. For example, why use AOS and not USLP? Is AOS better supported by other tools because of its "seniority" over USLP ?
There was a problem hiding this comment.
From what I understand, AOS Downlink + TC Uplink is pretty common. Or you can go USLP (In different modes I believe) both ways.
For FPrime upstream I think it makes sense to swap the ComAggregator (which I didn't know about until today) + TM Framer for the AOS Framer since it enables packets that span multiple frames. Or at least make a new AOS+TC Subtopo that does. And if FPrime gets a USLP defamer that can do variable length deframing, then it would make sense to switch over/provide USLP for both.
The big AOS upgrade--that this implementation doesn't provide--is the transfer frame insert zone, which is meant for realtime voice/video to be interspersed at a regular rate w/ standard telemetry & downlink.
We are likely not using this AOS framer for anything other than payload comms due to particulars of our radio.
There was a problem hiding this comment.
Has this Framer been tested end-to-end against a tool that is known to deframe AOS frames correctly?
There was a problem hiding this comment.
Not yet. I could try to work out a YAMCS test for downlink; however, my primary need for this component is for communicating w/ payloads, so I was planning on placing my integration testing focus on comms w/ those particular payloads
2772cbe to
fe4254c
Compare
| BASE_CONFIG | ||
| ) | ||
| target_compile_definitions(PlatformDarwin INTERFACE -DTGT_OS_TYPE_DARWIN) | ||
| target_link_options(PlatformDarwin INTERFACE -Wl,-no_warn_duplicate_libraries) |
There was a problem hiding this comment.
We should revert this deletion, maybe appeared because of the force push or something
|
|
||
| AosFramer ::AosFramer(const char* const compName) : AosFramerComponentBase(compName) { | ||
| // Default to FECF on, Max Sized if you don't override w/ another configure call | ||
| configure(ComCfg::AosMaxFrameFixedSize, true); |
Check notice
Code scanning / CodeQL
More than one statement per line Note
| // Default to FECF on, Max Sized if you don't override w/ another configure call | ||
| configure(ComCfg::AosMaxFrameFixedSize, true); | ||
| } | ||
|
|
Check notice
Code scanning / CodeQL
Long function without assertion Note
| } | ||
|
|
||
| // ---------------------------------------------------------------------- | ||
| // Handler implementations for typed input ports |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| } | ||
|
|
||
| // Pack this packet | ||
| pack_pad_send(data, context); |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| // Ensure configure was called | ||
| FW_ASSERT(currentVc.vc_struct_index == ind); | ||
| FW_ASSERT(currentVc.virtualChannelId == context.get_vcId()); | ||
| return currentVc; |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| Fw::SerializeStatus status = serializer.serializeFrom(header); | ||
| FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status); | ||
|
|
||
| // Fill with idle pattern |
Check notice
Code scanning / CodeQL
Function too long Note
| OWNED, //!< The buffer is currently owned by the AosFramer | ||
| }; | ||
|
|
||
| struct PDU { |
Check notice
Code scanning / CodeQL
More than one statement per line Note
| FwSizeType offset = 0; //!< Offset into the above packet to write | ||
| }; | ||
|
|
||
| struct AosVc { |
Check notice
Code scanning / CodeQL
More than one statement per line Note
|
|
||
| // Because the AOS protocol use fixed width frames, and only one frame is in transit between ComQueue and | ||
| // ComInterface at a time, we can use a member fixed-size buffer to hold the frame data | ||
| struct FrameBuffer { |
Check notice
Code scanning / CodeQL
More than one statement per line Note
| // SPP Idle packet backstop | ||
| // Technically we'd only use 6 of the 7 bytes at worst | ||
| // cuz the first one had to go into the prev frame | ||
| struct SppIdle { |
Check notice
Code scanning / CodeQL
More than one statement per line Note
Change Description
Implements a CCSDS AOS Framer that supports both many packets per frame and packets than span multiple frames.
Rationale
AOS is a newer protocol than TM (although USLP is even newer). Requiring that packets fit within one frame is overly restrictive.
Testing/Review Recommendations
Ensure code style, layout and coverage looks good. Determine if existing subset of AOS features would be sufficient for Fprime downlink.
Future Work
An AOS Deframer will be made to accompany this, and AOS gds infrastructure will also likely be added.
AI Usage (see policy)
N/A