-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Svc.ComAggregator: consolidate idle filling and configuration #5939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
Changes from all commits
8cd810f
a9f2841
00f9b7f
7a9b11d
02b76d7
97cad9b
028da83
51f6826
dd414e1
9a2f6ba
0f878b0
2c81043
af8acbb
6abb7a1
ee3b6a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,24 +2,21 @@ | |
|
|
||
| The `Svc::Ccsds::TmFramer` is an implementation of the [FramerInterface](../../../Interfaces/docs/sdd.md) for the CCSDS [TM Space Data Link Protocol](https://ccsds.org/Pubs/132x0b3.pdf). | ||
|
|
||
| It receives payload data (such as a Space Packet or a VCA_SDU) on input and produces a TM frame on its output port as a result. Please refer to the CCSDS [TM specification (CCSDS 132.0-B-3)](https://ccsds.org/Pubs/132x0b3.pdf) for details on the frame format and protocol. | ||
| It receives a complete TM Transfer Frame Data Field (`Svc.Ccsds.TmDataFieldSize` bytes, produced by an upstream [`Svc::ComAggregator`](../../../ComAggregator/docs/sdd.md)) on input and produces a TM frame on its output port as a result. Please refer to the CCSDS [TM specification (CCSDS 132.0-B-3)](https://ccsds.org/Pubs/132x0b3.pdf) for details on the frame format and protocol. | ||
|
|
||
| The `Svc::Ccsds::TmFramer` is designed to work in the common F Prime telemetry stack, receiving data from an upstream [`Svc::ComQueue`](../../../ComQueue/docs/sdd.md) and passing frames to a [Communications Adapter](../../../Interfaces/docs/sdd.md), such as a Radio manager component or [`Svc::ComStub`](../../../ComStub/docs/sdd.md), for transmission on the wire. It is commonly coupled with the [`Svc::Ccsds::SpacePacketFramer`](../../SpacePacketFramer/docs/sdd.md) to wrap CCSDS Space Packets into TM frames. | ||
| The `Svc::Ccsds::TmFramer` is designed to work in the common F Prime telemetry stack, receiving complete, idle-filled data fields from an upstream [`Svc::ComAggregator`](../../../ComAggregator/docs/sdd.md) and passing frames to a [Communications Adapter](../../../Interfaces/docs/sdd.md), such as a Radio manager component or [`Svc::ComStub`](../../../ComStub/docs/sdd.md), for transmission on the wire. It is commonly coupled with the [`Svc::Ccsds::SpacePacketFramer`](../../SpacePacketFramer/docs/sdd.md) to wrap CCSDS Space Packets into TM frames. | ||
|
|
||
| ## Internals | ||
|
|
||
| The TM protocol specifies a fixed frame size. This can be configured in the `config/ComCfg.fpp` file. | ||
|
|
||
| The `Svc::Ccsds::TmFramer` uses an internal (member) buffer to hold the fixed size frame. The buffer **must** be returned to the TmFramer via the `dataReturnIn` port once it has been used or consumed. When the buffer returns to the TmFramer it will reuse the buffer for the next frame. Should a component want to use the frame data past the time it is returned to the TmFramer, data should be copied before the original buffer is returned to the TmFramer via the `dataReturnIn` port. | ||
|
|
||
| The static sizing checks enforce that `ComCfg::AggregationSize` fits in the TM data field. The residual rule is | ||
| enforced at runtime in `dataIn_handler`, deterministically on the first frame if the configuration is misconfigured. | ||
|
|
||
| The data received on `dataIn` must either fill the frame data field exactly (e.g. when delivered by `Svc::ComAggregator` with packet spanning enabled) or leave at least 7 bytes (a Space Packet header plus one byte of idle data) so that the remainder can be filled with an Idle Packet as required by the protocol (4.2.2.5). Any other size is rejected by assertion. `ComCfg::AggregationSize` is the full TM data field available to `Svc::ComAggregator`; with spanning disabled, the maximum aggregate is `ComCfg::AggregationSize - 7`, while spanning-enabled aggregates fill the field exactly. Any intermediate layer that adds bytes (e.g. the 2-byte SA index of `Svc::Ccsds::CcsdsSdlsFramer`) must be subtracted from `ComCfg::AggregationSize` by the project. | ||
| The `Svc::Ccsds::TmFramer` is a pure frame layer: the data received on `dataIn` must fill the frame data field exactly (`Svc.Ccsds.TmDataFieldSize`, i.e. `ComCfg.TmFrameFixedSize` minus the 6-byte primary header and 2-byte trailer) and is copied into the frame unchanged. Any other size is rejected by assertion, deterministically on the first frame when the upstream stack is misconfigured. Idle filling of the data field (protocol 4.2.2.5) is the responsibility of the upstream [`Svc::ComAggregator`](../../../ComAggregator/docs/sdd.md), which emits aggregates of exactly its configured size; this keeps idle data upstream of any layer inserted between the aggregator and the framer, such as `Svc::Ccsds::CcsdsSdlsFramer`, whose added bytes (`Svc.Ccsds.SdlsSaIndexSize`) the project subtracts from the aggregation size. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Documentation] could fix Intro (line 7) still describes (Anchored below the offending line; the diff does not include line 7.) With this PR
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 97cad9b.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Documentation] Fixed in 028da83. |
||
|
|
||
| ## Usage Examples | ||
|
|
||
| The `Svc::Ccsds::TmFramer` component, as well as the rest of the CCSDS communications stack, is used in the [`Ref/`](https://github.com/nasa/fprime/tree/devel/Ref) example application. It is also generated by the `fprime-util new --deployment` command. | ||
| The `Svc::Ccsds::TmFramer` component, as well as the rest of the CCSDS communications stack, is used in the [`Ref/`](https://github.com/nasa/fprime/tree/devel/TestDeploymentsProject/Ref) example application. It is also generated by the `fprime-util new --deployment` command. | ||
|
|
||
| ## CCSDS Header Fields | ||
|
|
||
|
|
@@ -57,7 +54,7 @@ For each frame generated, the `Svc::Ccsds::TmFramer` will populate the CCSDS TM | |
| | SVC-Ccsds-TM-FRAMER-001 | The TmFramer shall implement the `Svc.FramerInterface`. | Inspection, Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-002 | The TmFramer shall construct CCSDS Telemetry (TM) Transfer Frames compliant with the CCSDS 132.0-B-3 standard. | Unit Test, Inspection | | ||
| | SVC-Ccsds-TM-FRAMER-002 | The TmFramer shall use a fixed frame size that is configurable by the project. | Unit Test, Inspection | | ||
| | SVC-Ccsds-TM-FRAMER-003 | The TmFramer shall accept payload data (Space Packets or VCA_SDU) to be framed via its `dataIn` port. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-003 | The TmFramer shall accept a complete frame data field of exactly `Svc.Ccsds.TmDataFieldSize` bytes to be framed via its `dataIn` port, and shall assert on any other size. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-004 | The TmFramer shall output the constructed TM Transfer Frame via its `dataOut` port. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-005 | The TmFramer shall return ownership of the input buffer via the `dataReturnOut` port after the framing process is complete. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-006 | The TmFramer shall accept returned buffers (previously sent via `dataOut`) through the `dataReturnIn` port for deallocation or reuse. | Unit Test | | ||
|
|
@@ -67,6 +64,6 @@ For each frame generated, the `Svc::Ccsds::TmFramer` will populate the CCSDS TM | |
| | SVC-Ccsds-TM-FRAMER-010 | The TmFramer shall be configurable with a Spacecraft Identifier. | Inspection, Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-011 | The TmFramer shall use the Virtual Channel Identifier passed in the `context` object on `dataIn`. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-012 | The TmFramer shall manage Master Channel Frame Count and Virtual Channel Frame Count. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-013 | The TmFramer shall fill the data field of the TM Transfer Frame with the payload data received on `dataIn`, and fill up the rest of the fixed-size frame with a single Idle Packet as defined by the protocol. When the payload data fills the data field exactly, no Idle Packet shall be inserted. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-014 | The TmFramer shall assert that payload data received on `dataIn` either fills the data field exactly or leaves room for a minimum Idle Packet (header plus one byte). | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-013 | The TmFramer shall fill the data field of the TM Transfer Frame with the payload data received on `dataIn`, unchanged. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-014 | The TmFramer shall assert that payload data received on `dataIn` fills the data field exactly. | Unit Test | | ||
| | SVC-Ccsds-TM-FRAMER-015 | The TmFramer shall assert that the First Header Pointer received in the `dataIn` context fits within the 11-bit First Header Pointer field. | Unit Test | | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Security] could fix
ground-reachable-assert(narrowed tolerance). This assert now requires an exact size where the previous check accepted any size leaving0or>= IdlePacket::MIN_SIZEresidual. In theComCcsdsSdlspathdata.getSize()is the encryptor output plus the SA index, and the encryptor is selected bySdlsSaRouterfrom the SA index, whichCcsdsSdlsFramertakes from the ground-settableSA_INDEXparameter when the context leaves it unset. A deployment with more than one SA whose encryptors differ in per-frame overhead (e.g. clear-text vs. AES-GCM +28) turns aSA_INDEXparameter update into a guaranteed assert on the next frame, where the smaller-overhead direction was previously tolerated. The constraint is only documented in the SDLS sdd, not enforced. Consider dropping the frame with an event (returningdataand signallingcomStatusOut) instead of asserting, or validating the size where the SA is resolved.cc @LeStarch @thomas-bc @bitWarrior — low-confidence finding, please confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Design] Concur — also in scope for design intent: the PR states invalid sizing fails at compile time or in
configComponents, but with any encryptor that adds per-frame overhead theComCcsdsSdlsstatic_assert(<=) cannot establish the exact fit, so this runtime assert on the first frame is the only enforcement; consider having the SDLS layer expose its overhead so the fit is checked at configuration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Operational] Concur — also in scope for configuration extremes: in the plain
ComCcsdsTM stack anyAggregator.aggregationSizebelow 1016 (e.g. an SDLS value of 1014/986 carried into a non-SDLS build) passes the<=static_assertand aborts the FSW here on the first frame, where the previous framer tolerated any residual of 0 or >= 7; theComCcsdsConfig.Aggregator.aggregationSizecomment should state "must equalSvc.Ccsds.TmDataFieldSizeexactly unless a layer between aggregator and TmFramer adds bytes".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and intentionally kept as an assert for now. On a fixed-size TM frame, a stack whose SAs map to encryptors with different per-frame overhead cannot produce valid frames for both SAs whatever the aggregator does; before this PR the shorter case "worked" only because
TmFramerappended an unauthenticated plaintext idle packet after the ciphertext (#5779). Documented in 97cad9b (ComCcsdsSdlsSDD,ComCcsdsConfig): all encryptors behindSdlsSaRoutermust share one overhead. If maintainers prefer a graceful drop (return +comStatusOut+ event) over the assert I can change it in this PR; otherwise padding inside the SDLS layer is the proper fix for mixed-overhead SAs and belongs in a follow-up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Security] Disagreement — escalating. I still flag this on 028da83; the contributor's response above indicates we disagree.
Re-checked the full
TmFramer.cppand the SDLS path on this head: the exact-sizeFW_ASSERTis unchanged andSA_INDEXis still a ground-settable parameter, so a mixed-overhead SA set turns a parameter update into an FSW abort. I accept that mixed-overhead SAs cannot produce valid fixed-size frames either way, and that this is now documented; my concern is only that the failure mode for a ground-reachable input is an assert rather than a dropped frame with an event.cc @LeStarch @thomas-bc @bitWarrior — needs human adjudication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Design] Disagreement — escalating. I still flag this on 028da83; the contributor's response above indicates we disagree.
Re-checked on this head:
ComCcsds.fppandComCcsdsSdls.fppboth boundaggregationSizewith<=, so nothing in the build establishes the exact fit the framer requires; a mis-sized deployment still only fails as a runtime assert on the first frame. The documented "all encryptors behindSdlsSaRoutershare one overhead" rule is sound and I accept the mixed-SA padding deferral — whether the remaining enforcement should stay an assert, become a graceful drop, or move to a configuration-time check once the SDLS layer exposes its overhead, is a design-owner call.cc @LeStarch @thomas-bc — needs human adjudication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Operational] Fixed in 028da83.