Svc.ComAggregator: consolidate idle filling and configuration by thomas-bc · Pull Request #5939 · nasa/fprime

thomas-bc · 2026-09-11T00:30:52Z


*Related Issue(s)*	#5836, #5779, #5928
*Has Unit Tests (y/n)*	y
*Documentation Included (y/n)*	y
*Generative AI was used in this contribution (y/n)*	AI

Change Description

Closes #5836
Closes #5779
Closes #5928

Svc.ComAggregator is the single non-AOS idle filler and is configured per instance:

-void configure(bool spanningEnabled);
+void configure(FwSizeType aggregationSize, bool spanningEnabled,
+               FwEnumStoreType allocationId, Fw::MemAllocator& allocator);
+void cleanup();

Every aggregate is exactly aggregationSize bytes, idle-filled in both spanning and non-spanning mode (non-spanning still never splits a packet). Storage comes from the allocator; the former static_asserts are FW_ASSERTs in configure().
Svc.Ccsds.TmFramer only serializes: asserts data.getSize() == TmPayloadCapacity, no idle insertion.
ComCfg.AggregationSize removed; new Svc.Ccsds.TmDataFieldSize / Svc.Ccsds.SdlsSaIndexSize; sizing lives in ComCcsdsConfig.Aggregator.aggregationSize (default TmDataFieldSize), wired by ComCcsds.fpp.
ComCcsdsSdls static_asserts aggregationSize + SdlsSaIndexSize == TmDataFieldSize; SDLS projects must set aggregationSize = Svc.Ccsds.TmDataFieldSize - Svc.Ccsds.SdlsSaIndexSize.
AOS untouched.

Rationale

Idle data was inserted by TmFramer after CcsdsSdlsFramer, so it was neither authenticated nor encrypted (#5779), and the idle logic was duplicated (#5836). With TmFramer no longer depending on the aggregate size, the size can leave the global config (#5928). Default (non-SDLS) wire format is unchanged.

Testing/Review Recommendations

fprime-util check in Svc/ComAggregator and Svc/Ccsds. Ref: 1024/2048/640-byte frames, spanning on/off, GDS integration suite, byte-level checks (frame size, single idle packet to trailer, CRC, FHP); SDLS-wired Ref shows the idle packet inside the encrypted region; invalid sizes fail at compile time or in configComponents.

Future Work

Projects using ComCcsdsSdls (e.g. fprime-encryption-reference) must override ComCcsdsConfig.Aggregator.aggregationSize.

AI Usage (see policy)

Implementation, unit tests, docs and integration testing by Devin under human direction.

IAMAI

Pushed by Devin AI

Move idle filling into Svc.ComAggregator for both spanning and non-spanning modes so every aggregate is exactly the configured size and idle data sits upstream of SDLS encryption. Svc.Ccsds.TmFramer becomes a pure fixed-size TM frame serializer that asserts on a partial data field. Replace the static ComCfg.AggregationSize with a per-instance runtime configure(aggregationSize, spanning, allocationId, allocator) + cleanup(), backed by Fw::MemAllocator. Add Svc.Ccsds.TmDataFieldSize and Svc.Ccsds.SdlsSaIndexSize constants and ComCcsdsConfig.Aggregator.aggregationSize. Fixes nasa#5836, nasa#5779, nasa#5928 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

…ic_assert topology sizing - cleanup() resets the frame buffer/serializer aliases before releasing storage - configure() rejects non-spanning sizes whose residual idle packet would overflow the SPP length field (IdlePacket::MAX_SIZE) - Non-spanning FHP is explicitly 0 - ComCcsds/ComCcsdsSdls phases static_assert aggregationSize against Svc.Ccsds.TmDataFieldSize (and SdlsSaIndexSize) - UTs for cleanup detach/reconfigure and the new upper bound Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

…assert anchor, docs) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

github-actions · 2026-09-11T00:49:18Z

Coverage report — base `devel`

Overall (line): 85.12% → 85.13% (+0.01)
Regression threshold: 0.50% (line).

Regressions

(none over threshold)

Modules changed

Module	Line	Δ	Function	Δ	Branch	Δ
`Fw/DataStructures`	97.60	-0.36	97.14	+0.00	82.12	-0.54
`Os/Generic/Types`	91.79	-0.35	92.86	+0.00	74.77	+0.00
`Os/Generic`	90.14	-0.10	89.13	+0.00	75.10	+0.00
`Svc/ComAggregator`	98.50	+0.33	96.43	+0.28	88.66	+2.41

New modules

Module	Line	Function	Branch
`Fw/Prm`	0.00	0.00	0.00

Modules without UTs

CFDP/Checksum/GTest, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/Ports/DataTypes, Drv/PosixUartDriver, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Obj, Fw/Port, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Ccsds/Types, Svc/Ccsds/Utils, Svc/FatalHandler, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComCcsdsSdls, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Subtopologies/FileHandlingCfdp, Svc/Subtopologies/FileHandlingCfdp/FileHandlingCfdpConfig, TestDeploymentsProject/Ref/DpDemo, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestConfigDeployment, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestDuplicateDirective, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestLinkDepends, cmake/test/data/TestDeployment/TestRelative, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent

…x; document encryptor overhead (nasa#5795) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Drop the reduced non-spanning capacity (aggregationSize - 7). Packets fill the whole aggregate; without spanning a packet is added only when it completes the aggregate exactly or leaves room for a minimum idle packet, which keeps idle fill whole without splitting packets. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

lestarch-autobot · 2026-09-11T02:13:22Z

    ASSERT_from_comStatusOut(1, inputStatus);  // at index 1, received FAILURE
 }

+void TmFramerTester ::fillDataField(U8* bufferData) {


[C++ Design] could fix New test helper takes a bare U8* with no paired length (CPP-21, cpp-c-style-array-in-interface).

fillDataField(U8* bufferData) relies on every caller passing exactly TmPayloadCapacity bytes; nothing in the signature enforces it. Since each call site already has an Fw::Buffer wrapping the array, take Fw::Buffer& (or Fw::ByteArray) and loop to getSize(), so the length travels with the data.

Fixed in 97cad9b: fillDataField(Fw::Buffer&), asserts the buffer is TmPayloadCapacity bytes.

[C++ Design] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:13:30Z

+    FW_ASSERT(this->m_frameSerializer.getSize() <= this->m_aggregationSize,
+              static_cast<FwAssertArgType>(this->m_frameSerializer.getSize()));
+    const FwSizeType residual = this->m_aggregationSize - this->m_frameSerializer.getSize();


[Maintainability] suggestion maint-code-duplication: fillResidualWithIdle() now inlines the body of remainingCapacity().

The base branch called this->remainingCapacity() here; this PR replaces it with a copy of that helper's assert and subtraction (see remainingCapacity() a few lines above). Two copies of the capacity invariant means the next change to it (e.g. accounting for a header or trailer) has to be made twice, and a drift between them is silent.

Suggested change

FW_ASSERT(this->m_frameSerializer.getSize() <= this->m_aggregationSize,

static_cast<FwAssertArgType>(this->m_frameSerializer.getSize()));

const FwSizeType residual = this->m_aggregationSize - this->m_frameSerializer.getSize();

const FwSizeType residual = this->remainingCapacity();

Fixed in 97cad9b: fillResidualWithIdle() uses remainingCapacity().

[Maintainability] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:13:30Z

+        // Restore the size in case a downstream consumer shrank the buffer before returning it
        this->m_frameBuffer.setSize(this->m_frameSerializer.getSize());


[Maintainability] could fix maint-misleading-comment: the new comment credits this setSize with a restore that doClear already performs.

Per AggregationMachine, doSend is only reachable after doClear (IS_GOOD_STATUS -> SPAN_CHECK -> FILL), and doClear sets m_frameBuffer back to m_aggregationSize (line 133). With the FW_ASSERT at line 164 this line is a no-op re-set of the same value, so a reader following the comment looks for a downstream-shrink path recovered here and finds none. Either drop the pair or reword the comment to say it is a belt-and-braces re-set.

Suggested change

// Restore the size in case a downstream consumer shrank the buffer before returning it

this->m_frameBuffer.setSize(this->m_frameSerializer.getSize());

Fixed in 97cad9b: the setSize and its comment are removed (doClear restores the size).

[Maintainability] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:13:35Z

-    const FwSizeType residual = TmPayloadCapacity - data.getSize();
-    FW_ASSERT(residual == 0 || residual >= Utils::IdlePacket::MIN_SIZE, static_cast<FwAssertArgType>(residual));
+    // The data must fill the data field exactly: idle filling (Standard 4.2.2.5) is done upstream by Svc::ComAggregator
+    FW_ASSERT(data.getSize() == TmPayloadCapacity, static_cast<FwAssertArgType>(data.getSize()));


[Security] could fix ground-reachable-assert (narrowed tolerance). This assert now requires an exact size where the previous check accepted any size leaving 0 or >= IdlePacket::MIN_SIZE residual. In the ComCcsdsSdls path data.getSize() is the encryptor output plus the SA index, and the encryptor is selected by SdlsSaRouter from the SA index, which CcsdsSdlsFramer takes from the ground-settable SA_INDEX parameter 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 a SA_INDEX parameter 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 (returning data and signalling comStatusOut) instead of asserting, or validating the size where the SA is resolved.

cc @LeStarch @thomas-bc @bitWarrior — low-confidence finding, please confirm.

[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 the ComCcsdsSdls static_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.

[Operational] Concur — also in scope for configuration extremes: in the plain ComCcsds TM stack any Aggregator.aggregationSize below 1016 (e.g. an SDLS value of 1014/986 carried into a non-SDLS build) passes the <= static_assert and aborts the FSW here on the first frame, where the previous framer tolerated any residual of 0 or >= 7; the ComCcsdsConfig.Aggregator.aggregationSize comment should state "must equal Svc.Ccsds.TmDataFieldSize exactly unless a layer between aggregator and TmFramer adds bytes".

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 TmFramer appended an unauthenticated plaintext idle packet after the ciphertext (#5779). Documented in 97cad9b (ComCcsdsSdls SDD, ComCcsdsConfig): all encryptors behind SdlsSaRouter must 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.

[Security] Disagreement — escalating. I still flag this on 028da83; the contributor's response above indicates we disagree.

Re-checked the full TmFramer.cpp and the SDLS path on this head: the exact-size FW_ASSERT is unchanged and SA_INDEX is 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.

[Design] Disagreement — escalating. I still flag this on 028da83; the contributor's response above indicates we disagree.

Re-checked on this head: ComCcsds.fpp and ComCcsdsSdls.fpp both bound aggregationSize with <=, 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 behind SdlsSaRouter share 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.

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:13:35Z

+        this->m_frameSerializer.setExtBuffer(nullptr, 0);
+        this->m_frameBuffer.set(nullptr, 0);
+        this->m_aggregationSize = 0;
+        this->m_allocator->deallocate(this->m_allocationId, this->m_allocation);


[Security] could fix general-vulnerability/use-after-free. cleanup() releases the aggregate storage without checking m_bufferState. m_frameBuffer points into this allocation and is handed downstream in doSend; if cleanup() runs while the buffer is NOT_OWNED (still held by the framer / com driver, or returned later on the caller's thread via dataReturnIn_handler), the downstream holder is left with a dangling pointer. The previous static member storage could not dangle. Asserting ownership before the free makes the lifecycle violation visible rather than silent (best-effort fix; verify before applying).

Suggested change

this->m_allocator->deallocate(this->m_allocationId, this->m_allocation);

FW_ASSERT(this->m_bufferState == Fw::Buffer::OwnershipState::OWNED,

static_cast<FwAssertArgType>(this->m_bufferState.load()));

this->m_allocator->deallocate(this->m_allocationId, this->m_allocation);

cc @LeStarch @thomas-bc @bitWarrior — low-confidence finding, please confirm.

[Design] Concur — also in scope for design fault handling: the SDD makes "downstream framer has returned the aggregate" a precondition of cleanup(), but nothing enforces it; asserting m_bufferState == OWNED here makes the lifecycle contract checkable at teardown.

Fixed in 97cad9b: cleanup() asserts m_bufferState == OWNED before detaching/deallocating; doc-comment states the lifecycle precondition; new Lifecycle.CleanupWhileHeld death test.

[Security] Fixed in 028da83.

[Design] Fixed in 97cad9b.

lestarch-autobot · 2026-09-11T02:13:54Z

-        ASSERT_EQ(outBuffer.getData()[i], idlePattern)
-            << "Idle data at index " << i << " does not match expected idle pattern";
+    // The frame is header + the data field exactly as delivered + trailer
+    ASSERT_EQ(TMHeader::SERIALIZED_SIZE + sizeof(bufferData) + TMTrailer::SERIALIZED_SIZE, expectedFrameSize);


[Test Quality] could fix This size assertion is true by construction.

sizeof(bufferData) is TmFramer::TmPayloadCapacity, which is defined as TmFrameFixedSize - (TMHeader::SERIALIZED_SIZE + TMTrailer::SERIALIZED_SIZE), so the left side always reduces to TmFrameFixedSize (expectedFrameSize). It cannot fail regardless of framer behavior; the byte-for-byte loop below is the real check and TmFramer.hpp already static_asserts the TmDataFieldSize relation.

Suggested change

ASSERT_EQ(TMHeader::SERIALIZED_SIZE + sizeof(bufferData) + TMTrailer::SERIALIZED_SIZE, expectedFrameSize);

Fixed in 97cad9b: assertion removed.

[Test Quality] Fixed in 97cad9b.

lestarch-autobot · 2026-09-11T02:13:54Z


 Fw::SerializeStatus serialize(Fw::SerialBufferBase& serializer, FwSizeType size) {
    FW_ASSERT(size >= MIN_SIZE, static_cast<FwAssertArgType>(size));
+    FW_ASSERT(size <= MAX_SIZE, static_cast<FwAssertArgType>(size));


[Test Quality] could fix New MAX_SIZE upper-bound assertion has no test coverage.

Svc/Ccsds/Utils has no unit test, and no existing test drives IdlePacket::serialize at or past MAX_SIZE (65542): the ComAggregator tests only exercise the aggregator's own configure-time bound (ComAggregator.cpp), then cleanup() without emitting an aggregate; residual idle packets in the suite stay below 4096 bytes. Consider a death test for MAX_SIZE + 1 and a positive serialize at exactly MAX_SIZE asserting the emitted SPP length token is 0xFFFF.

Not addressed in this PR: Svc/Ccsds/Utils has no UT module and the aggregator cannot reach a residual of exactly MAX_SIZE (non-spanning max aggregate is 65543, spanning max 2046). Adding a Svc/Ccsds/Utils UT is a reasonable follow-up; happy to add it here if maintainers want it in scope.

[Test Quality] Disagreement — escalating. I still flag this on 028da83; the contributor's response above indicates we disagree.

Re-checked the head: IdlePacket::serialize still has no test anywhere reaching the new MAX_SIZE assert (Svc/Ccsds/Utils has no UT; the aggregator only exercises its configure-time bound). Agreed the aggregator cannot produce a MAX_SIZE residual in practice, which is why this is tagged could fix; a direct Svc/Ccsds/Utils UT (death at MAX_SIZE + 1, length token 0xFFFF at MAX_SIZE) remains the only way to observe the guard, and the contributor has offered to add it in scope if maintainers want.

cc @LeStarch @thomas-bc — needs human adjudication.

lestarch-autobot · 2026-09-11T02:13:54Z

@@ -523,10 +639,104 @@ void ComAggregatorTester ::test_spanning_idle_span() {
 void ComAggregatorTester ::test_configure_after_fill_asserts() {
    this->test_initial();
    (void)this->test_fill(false);


[Test Quality] could fix The fill step no longer contributes to what this test verifies.

configure() now asserts on m_allocation != nullptr, i.e. on any second call without an intervening cleanup(), whether or not data was aggregated. The test_fill preamble and the name/doc ("asserts once data has been aggregated") describe the old contract; the test passes for a reason unrelated to filling. Consider renaming to reflect "reconfigure without cleanup asserts" (or dropping the fill), so the test documents the behavior it actually pins.

Fixed in 97cad9b: renamed to ReconfigureWithoutCleanup and the fill step dropped.

[Test Quality] Fixed in 97cad9b.

lestarch-autobot · 2026-09-11T02:15:25Z

+(idle-filled) and the buffer starts the next one. A single buffer may therefore be at most `aggregationSize - 7`
+bytes. `configure()` asserts unless a full-size `Fw::ComBuffer` or file buffer Space Packet fits within that limit,
+and unless the largest possible residual (`aggregationSize - 1`) is expressible in the idle packet's SPP length field
+(`aggregationSize <= 65544`).


[Documentation] suggestion SDD limit is off by one relative to the configure() assertion

configure() asserts (aggregationSize - 1) <= Ccsds::Utils::IdlePacket::MAX_SIZE, and IdlePacket::MAX_SIZE is SpacePacketHeader::SERIALIZED_SIZE + 1 + 65535 = 65542, so the largest accepted aggregationSize is 65543, not 65544.

Suggested change

(`aggregationSize <= 65544`).

(`aggregationSize <= 65543`).

[Operational] Concur — also in scope for operational doc-vs-reality: a project sizing to the documented 65544 boots into the configure() FW_ASSERT (65543 <= IdlePacket::MAX_SIZE = 65542 fails) in configComponents; severity from my scope: must fix.

Fixed in 97cad9b: 65543.

[Documentation] Fixed in 028da83.

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:15:25Z

-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.


[Documentation] could fix Intro (line 7) still describes TmFramer as "receiving data from an upstream Svc::ComQueue"

(Anchored below the offending line; the diff does not include line 7.)

With this PR dataIn_handler asserts unless the payload is exactly Svc.Ccsds.TmDataFieldSize, so a ComQueue/SpacePacketFramer → TmFramer stack as described in the intro now asserts on the first frame; only Svc::ComAggregator (or an equivalent idle-filling stage) can feed dataIn. Suggest rewording line 7 to "receiving fixed-size aggregates from an upstream Svc::ComAggregator" so the intro agrees with the Internals paragraph.

Fixed in 97cad9b.

[Documentation] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:15:25Z

 - **SdlsFileKeyManager** — Supplies encryption keys read from a configured file.
 - **ClearTextEncryptor / ClearTextDecryptor** — Pass-through default crypto components (**no security**); the defaults selected by the `Svc.ComCcsdsSdls` subtopology configuration.
- **AesGcmEncryptor / AesGcmDecryptor** — AES-256-GCM authenticated encryption (OpenSSL 3.x), producing/consuming an `IV (12) | ciphertext | MAC (16)` security payload with the VC and SA index authenticated as additional data. The decryptor reports a failed MAC check as `MAC_VERIFICATION_FAILURE`, distinct from `DECRYPTION_FAILURE`. Each frame's 28-byte overhead must be subtracted from `ComCfg.AggregationSize` (see the `Svc.ComCcsdsSdls` SDD).
+- **AesGcmEncryptor / AesGcmDecryptor** — AES-256-GCM authenticated encryption (OpenSSL 3.x), producing/consuming an `IV (12) | ciphertext | MAC (16)` security payload with the VC and SA index authenticated as additional data. The decryptor reports a failed MAC check as `MAC_VERIFICATION_FAILURE`, distinct from `DECRYPTION_FAILURE`. Each frame's 28-byte overhead must be subtracted from `ComCcsdsConfig.Aggregator.aggregationSize` (see the `Svc.ComCcsdsSdls` SDD).


[Documentation] could fix "Protocol Layering" (line 65) still gives Space Packet Framer → TM Framer as the typical downlink path

(Anchored above the offending line; the diff does not include line 65.)

The TM section above now says the data field is delivered complete by ComAggregator, and TmFramer asserts on any other payload size, so the layering example describes a stack that no longer works. Suggest data source → Space Packet Framer → ComAggregator → TM Framer → byte stream driver (with the SDLS layer between the aggregator and the TM Framer).

Fixed in 97cad9b: layering now reads Space Packet Framer -> ComAggregator -> TM Framer (SDLS optional in between).

[Documentation] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:15:25Z

 | SVC-COMCCSDS-004 | Provide a **subtopology variant that supplies `Svc::ComStub`** designed to connect to a ByteStream driver.     | Inspection |
 | SVC-COMCCSDS-005 | Provide a **subtopology variant that expects an external `Svc::ComInterface`** supplied by the deployment.     | Inspection |
-| SVC-COMCCSDS-006 | Support **configurable instance properties** (IDs, queue sizes, stack sizes, priorities, CPU affinities, packet spanning) via `ComCcsdsConfig`. | Inspection |
+| SVC-COMCCSDS-006 | Support **configurable instance properties** (IDs, queue sizes, stack sizes, priorities, CPU affinities, aggregation size, packet spanning) via `ComCcsdsConfig`. | Inspection |


[Documentation] could fix docs/reference/system-functional/subtopology-com-ccsds.md does not mention the new Aggregator.aggregationSize knob

(Anchored on the SDD requirement this reference page mirrors; the reference page is not in the diff.)

Its "Configuration" bullet (line 58) lists only "Base IDs, queue sizes, stack sizes, priorities, and CPU affinities via ComCcsdsConfig", while this requirement now adds aggregation size and packet spanning — and aggregationSize must be overridden for any ComCcsdsSdls deployment. Preexisting on the same page: the "Downlink Path" and "Included Components" sections omit the aggregator stage between the Space Packet Framer and the TM Framer.

Fixed in 97cad9b: Aggregator.aggregationSize added to the configuration list in subtopology-com-ccsds.md.

[Documentation] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:15:25Z

+                   Fw::MemAllocator& allocator    //!< Fw::MemAllocator used to acquire the aggregation buffer
    );

+    //! Deallocate the aggregation buffer


[Documentation] suggestion cleanup() doc-comment omits its call-ordering precondition

cleanup() releases the memory backing m_frameBuffer while a downstream framer may still hold that buffer, and dataIn_handler asserts on m_allocation != nullptr afterwards. The SDD states it must only be called once the aggregate has been returned and no further data or status can arrive (after the task has stopped); the public API comment should carry the same contract.

Suggested change

//! Deallocate the aggregation buffer

//! Deallocate the aggregation buffer

//!

//! Must only be called once the downstream framer has returned the aggregate and no further data or status

//! can arrive (i.e. after the component's task has stopped); configure() may be called again afterwards.

Fixed in 97cad9b.

[Documentation] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:16:48Z

    # Aggregator configuration constants
    module Aggregator {
+        @ Size in bytes of every aggregate emitted by the aggregator instance: the TM Transfer Frame Data Field.
+        @ Aggregates are always idle-filled to this size, including in the frame-less SpacePacket topology.


[Design] could fix design-behavioral-regression: the frame-less ComCcsds.SpacePacket topology now emits fixed-size, idle-padded aggregates; the PR's "default (non-SDLS) wire format is unchanged" holds only for the TM stack.

aggregator lives in SpacePacketFraming, so a deployment on ComCcsds.SpacePacket (no TmFramer) previously sent variable-size aggregates of whole packets and now sends Svc.Ccsds.TmDataFieldSize (1016) bytes per flush — including a timeout flush carrying one small packet — with SPP idle packets (APID 0x7FF) the ground side must discard. This SDD note acknowledges it, but the PR description does not assess it, and the frame-less topology cannot opt out other than by shrinking aggregationSize (bounded below by MIN_NON_SPANNING_AGGREGATION_SIZE). Please state the change and its link-rate / ground-side impact in the PR, or consider making exact-size filling a configure() option so the packet-only layer keeps its previous behavior.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[Operational] Concur — also in scope for link-budget consequences: on ComCcsds.SpacePacket every timeout flush now costs 1016 bytes; at Ref's 1 Hz aggregatorTimeout that is an ~8.1 kbit/s idle floor (~85% of a 9600-baud UART) for a lone telemetry packet, and the SDD warning should carry that number.

Correct: ComCcsds.SpacePacket now emits fixed-size idle-padded aggregates (timeout flushes included). That is inherent to making the aggregator the single idle filler (#5836); an opt-out would reintroduce the partial-aggregate mode this PR removes, so I did not add one. The GDS space-packet deframer already discards APID 0x7FF. PR description updated to state the impact; ComCfg comment (97cad9b) covers the packing/link-rate consequence.

[Design] Disagreement — escalating. I still flag this on 028da83; the contributor's response above indicates we disagree.

Re-read the PR description at this head: it still states "Default (non-SDLS) wire format is unchanged" and says nothing about ComCcsds.SpacePacket, so the description update referenced above is not present. The new ComCfg / ComCcsdsConfig comments cover sizing and link cost, not the fact that the frame-less topology's wire format changed. I accept the no-opt-out rationale; the unacknowledged wire-format change for that topology is what remains.

cc @LeStarch @thomas-bc — needs human adjudication.

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:20:56Z

+        // Allocation identifier is 0 as the MallocAllocator discards it
+        ComCcsds::aggregator.configure(ComCcsdsConfig::Aggregator::aggregationSize,
+                                       ComCcsdsConfig::Aggregator::enablePacketSpanning,
+                                       0,


[Operational] suggestion ops-contract-trace: new Fw::MemAllocator call site reuses identifier 0, already used by comQueue.configure(..., 0, ...) and commsBufferManager.setup(..., 0, ...) in this file.

Fw::MemAllocator::allocate documents the identifier as unique per entity (implementations may tie an id to a memory segment). Scenario: a project replaces ComCcsds::Allocation::memAllocator (it lives in the overridable ComCcsdsConfig/ directory) with a segment- or budget-keyed allocator; the aggregator's 1016-byte request then lands in the same segment as comQueue's queue storage and the buffer manager's pool, and cleanup() frees id 0 memory the other two still use. MallocAllocator ignores the id, so only non-malloc deployments (RTOS/static) are affected. Operational judgment call. Smallest remedy: a distinct id, as frameAccumulator already does with 1.

Suggested change

0,

2,

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

Fixed in 97cad9b: allocation id 2 (0 = comQueue/bufferManager, 1 = frameAccumulator).

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:20:56Z

+        static_assert(static_cast<FwSizeType>(ComCcsdsConfig::Aggregator::aggregationSize) <=
+                          static_cast<FwSizeType>(Svc::Ccsds::TmDataFieldSize),
+                      "ComCcsdsConfig.Aggregator.aggregationSize must fit the TM Transfer Frame Data Field");


[Operational] suggestion ops-config-extreme: the phase static_assert covers only the upper bound; the lower bound and the spanning FHP range are FW_ASSERTs inside configure(), so a legal-looking build aborts in configComponents instead of failing to compile.

Scenario: a project raises FW_FILE_BUFFER_MAX_SIZE above 1003 (973 with ComCcsdsSdls + AES-GCM) while keeping the 1024-byte frame, or enables spanning with TmFrameFixedSize > 2054; the first boot dies at ComAggregator::configure (aggregationSize >= MIN_NON_SPANNING_AGGREGATION_SIZE / <= 2046). Default headroom is 1016 - 525 = 491 bytes. Both operands are compile-time constants, so the same checks can be static_asserts here.

Suggested change

static_assert(static_cast<FwSizeType>(ComCcsdsConfig::Aggregator::aggregationSize) <=

static_cast<FwSizeType>(Svc::Ccsds::TmDataFieldSize),

"ComCcsdsConfig.Aggregator.aggregationSize must fit the TM Transfer Frame Data Field");

static_assert(static_cast<FwSizeType>(ComCcsdsConfig::Aggregator::aggregationSize) <=

static_cast<FwSizeType>(Svc::Ccsds::TmDataFieldSize),

"ComCcsdsConfig.Aggregator.aggregationSize must fit the TM Transfer Frame Data Field");

static_assert(ComCcsdsConfig::Aggregator::enablePacketSpanning ||

(static_cast<FwSizeType>(ComCcsdsConfig::Aggregator::aggregationSize) >=

Svc::ComAggregator::MIN_NON_SPANNING_AGGREGATION_SIZE),

"ComCcsdsConfig.Aggregator.aggregationSize must hold a full com/file buffer Space Packet plus a minimum idle packet");

static_assert((!ComCcsdsConfig::Aggregator::enablePacketSpanning) ||

(static_cast<FwSizeType>(ComCcsdsConfig::Aggregator::aggregationSize) <=

static_cast<FwSizeType>(Svc::Ccsds::TMSubfields::FHP_IDLE_DATA_ONLY)),

"ComCcsdsConfig.Aggregator.aggregationSize must not exceed the TM First Header Pointer range with spanning");

Not applicable as written: FPP bool constants are emitted as extern const bool (see ComCcsdsConfig/FppConstantsAc.hpp), so enablePacketSpanning is not usable in a static_assert, and an unconditional lower bound would reject legal spanning configurations. The upper bound stays compile-time; lower/FHP bounds remain FW_ASSERTs in configure() at configComponents and are documented in ComCcsdsConfig.fpp (97cad9b).

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:20:56Z

 `ComCcsdsSdlsConfig` supplies the `BASE_ID` for the SDLS instances and the `decryptor` and `encryptor` instance definitions (see 2.4). The reused packet and transfer frame layers are configured through `ComCcsdsConfig` (queue sizes, priorities, buffer sizing, memory allocator), exactly as when using `ComCcsds` directly.

-`Svc.Ccsds.CcsdsSdlsFramer` prepends a 2-byte SA index to each aggregate, so projects must set `ComCfg.AggregationSize = TmFrameFixedSize - 6 - 2 - 2` (TM header, trailer, and SA index) for the spanning-enabled aggregator output to fit the TM data field. With spanning disabled, the maximum aggregate is `ComCfg.AggregationSize - 7`; the First Header Pointer is relative to the data following the SA index (the SDLS security header is not part of the TM data field per CCSDS 355.0-B). With this value, spanning aggregates plus the SA index fill the TM data field exactly. Any other value that leaves 1–6 bytes of residual asserts in `TmFramer::dataIn_handler` on the first frame. A real encryptor adds its own overhead to every frame and must be subtracted as well: the AES-256-GCM pair adds 28 bytes (12-byte IV plus 16-byte MAC), so `ComCfg.AggregationSize = TmFrameFixedSize - 6 - 2 - 2 - 28` (986 for the default 1024-byte frame).
+`Svc.Ccsds.CcsdsSdlsFramer` prepends a 2-byte SA index to each aggregate, so projects must set `ComCcsdsConfig.Aggregator.aggregationSize = Svc.Ccsds.TmDataFieldSize - Svc.Ccsds.SdlsSaIndexSize` minus any per-frame overhead the encryptor adds, so that the encrypted aggregate plus the SA index fills the TM data field exactly. With the size-preserving clear-text default that is `TmDataFieldSize - SdlsSaIndexSize`; the AES-256-GCM pair adds 28 bytes (12-byte IV plus 16-byte MAC), so `aggregationSize = Svc.Ccsds.TmDataFieldSize - Svc.Ccsds.SdlsSaIndexSize - 28` (986 for the default 1024-byte frame). Because the TM frame is fixed-size, every encryptor reachable through the encryption SA router must produce the same output size. The aggregator idle-fills each aggregate to `aggregationSize` before it reaches the encryptor, so idle data is authenticated and encrypted along with the packets; the First Header Pointer is relative to the data following the SA index (the SDLS security header is not part of the TM data field per CCSDS 355.0-B). The subtopology checks at compile time (`static_assert` in the `sdlsFramer` instance's `configComponents` phase) that `aggregationSize + SdlsSaIndexSize` fits the TM data field; the exact fit is asserted by `TmFramer::dataIn_handler` on the first frame.


[Operational] could fix ops-failure-path: "the exact fit is asserted by TmFramer::dataIn_handler on the first frame" does not hold for the AES-GCM path; the failure there is a silent, permanent drop.

Scenario: AES-GCM encryptor selected with any aggregationSize in 987..1014 (all pass the <= static_assert). AesGcmEncryptor::encryptIn_handler computes size + 28 > MAX_OUTPUT_SIZE (1014) and calls failFrame; CcsdsSdlsFramer logs EncryptionFailed, drops the frame, and re-arms com status, so every frame is dropped: 100% downlink loss, no assert, and the WARNING_HI events cannot reach the ground. Operational judgment call (depends on the project's encryptor choice). Smallest remedy: state here that an oversize aggregate with AES-GCM drops every frame rather than asserting, and add a project-side static_assert(aggregationSize + SdlsSaIndexSize + 28 <= TmDataFieldSize) example next to the encryptor instance selection.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

Fixed in 97cad9b: SDD now describes the AesGcmEncryptor path (oversize aggregate -> EncryptionFailed, frame dropped before TmFramer) and suggests pinning the fit next to the encryptor selection.

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:20:56Z

+    @ Fixed size of CCSDS TM frames. The data field (Svc.Ccsds.TmDataFieldSize = TmFrameFixedSize - 8) is the
+    @ aggregate size expected by Svc.Ccsds.TmFramer; see ComCcsdsConfig.Aggregator.aggregationSize for the sizing
+    @ constraints. Without packet spanning the data field must hold a full com buffer or file buffer Space Packet
+    @ next to a minimum idle packet: at least max(FW_COM_BUFFER_MAX_SIZE, FW_FILE_BUFFER_MAX_SIZE) + 6 + 7 bytes.


[Operational] could fix ops-doc-reality: the new sizing guidance gives only the minimum; it omits the packing consequence of the default values.

With the defaults (FW_FILE_BUFFER_MAX_SIZE = 512, data field = 1016) a full file packet is 518 bytes and two of them need 1036, so without spanning at most one file packet fits per 1024-byte frame: file downlink is capped at ~50% of the link rate, the other 498 bytes carrying telemetry/events or idle. This is preexisting behavior, but this is the line operators will read when sizing the frame. Smallest remedy: one sentence of guidance.

Suggested change

@ next to a minimum idle packet: at least max(FW_COM_BUFFER_MAX_SIZE, FW_FILE_BUFFER_MAX_SIZE) + 6 + 7 bytes.

@ next to a minimum idle packet: at least max(FW_COM_BUFFER_MAX_SIZE, FW_FILE_BUFFER_MAX_SIZE) + 6 + 7 bytes.

@ Throughput: without spanning, N full file packets per frame need N * (FW_FILE_BUFFER_MAX_SIZE + 6) bytes exactly

@ or plus 7; with the defaults (512, 1024) only one 518-byte file packet fits per 1016-byte data field (~50% link

@ efficiency for file downlink). Enable packet spanning or size the data field as a multiple of the file packet.

Fixed in 97cad9b: comment now states the default packing (one 518-byte file packet per 1016-byte frame) and how to size for N file packets or enable spanning.

[Operational] Fixed in 028da83.

lestarch-autobot · 2026-09-11T02:20:56Z

-    FW_ASSERT(this->m_spanning || data.getSize() <= this->m_capacity, static_cast<FwAssertArgType>(data.getSize()));
+    FW_ASSERT(this->m_allocation != nullptr);
+    // Without spanning, any packet must fit in an empty aggregate next to a minimum idle packet
+    FW_ASSERT(this->m_spanning || (data.getSize() + Ccsds::Utils::IdlePacket::MIN_SIZE) <= this->m_aggregationSize,


[Operational] future work ops-failure-path: an oversize downlink buffer aborts the flight software rather than being dropped (preexisting; this PR only rewrites the bound).

Scenario (non-spanning default): any producer wired to ComCcsds.bufferQueueIn hands ComQueue an Fw::Buffer larger than aggregationSize - 13 (1003 bytes with defaults, e.g. a project data-product or image buffer); SpacePacketFramer frames it and this assert fires on the comQueue thread, taking down every component in the deployment during whatever sequence is running. MIN_NON_SPANNING_AGGREGATION_SIZE only guards FW_COM_BUFFER_MAX_SIZE / FW_FILE_BUFFER_MAX_SIZE, not arbitrary Fw::Buffer producers. Remedy when revisited: return the buffer via dataReturnOut with a WARNING_HI event and comStatusOut re-arm instead of asserting, or document the 1003-byte producer limit in ComCcsds guidance.

Agreed, preexisting; the bound is unchanged in nature and the 1003-byte non-spanning producer limit is now stated in ComCcsdsConfig.fpp. Graceful drop left for a follow-up.

lestarch-autobot

Automated review summary (run 1)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	2	0	2	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	0	1	0	1	Go
Documentation Currency	0	2	3	0	5	Go
Design	0	0	3	0	3	Go
Architecture	0	0	0	0	0	Go
Test Quality	0	0	3	0	3	Go
Correctness	0	0	0	0	0	Go
Operational	1	2	3	1	7	No-Go
Maintainability	0	1	1	0	2	Go
CI safety	—	—	—	—	—	Go
Totals	1	5	16	1	23	No-Go

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Outstanding must-fix items (1)

Operational

Svc/ComAggregator/docs/sdd.md documents aggregationSize <= 65544 but configure() accepts at most 65543; a project sizing to the documented value aborts in configComponents — escalated concurrence on the Documentation thread — #5939 (comment)

Merge readiness

Merge readiness: No-Go — Operational has 1 outstanding must-fix item (escalated concurrence on the Documentation thread).

One off-by-one between the aggregator and its SDD — trim it and this frame is ready for downlink.

- cleanup() asserts the aggregate is not held downstream before freeing it; document the lifecycle requirement - fillResidualWithIdle() reuses remainingCapacity(); drop redundant setSize in doSend - ComCcsds: distinct allocation id (2) for the aggregator - SDD: non-spanning upper bound is 65543, not 65544 - Docs: TmFramer SDD intro, CCSDS layering, subtopology reference page, ComCcsdsConfig sizing comments, ComCfg packing note, SDLS AES-GCM oversize behavior - UTs: fillDataField takes a sized Fw::Buffer, drop tautological assert, rename reconfigure test, add CleanupWhileHeld death test, return outstanding aggregates before teardown Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

fpp requires every transition into a choice to carry the same type. PAUSE_CHECK is entered by both hostResumeI32 (I32) and DEQUEUE_CHECK (FwIndexType); fpp 3.3.0 only accepted this by symbol-order luck, and the Ref fpp-to-json check fails once unrelated symbols are added. Route the I32 resume through an identical RESUME_I32_PAUSE_CHECK choice. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

thomas-bc-autowot · 2026-09-11T04:16:13Z

@LeStarch — the External Repo: cFS Reference failure on this PR is a real downstream break, not a config gap, and I'd like your call on it.

TmFramer now requires a complete, already idle-filled data field (idle filling lives in Svc.ComAggregator, both spanning and non-spanning). The cFS GDS bridge in fprime-community/fprime_gds wires tlmStripper.dataOut -> ComCcsdsNoRouter.framer.dataIn, i.e. single space packets straight into TmFramer, relying on the old per-frame idle padding — so the FSW asserts on the first frame:

Assert: ".../Svc/Ccsds/TmFramer/TmFramer.cpp:30" 27

Options:

Keep TmFramer strict (this PR) and add a non-spanning Svc.ComAggregator in front of framer in GdsBridge/ComCcsdsNoRouter (a short patch in fprime_gds; I can draft it).
Keep an idle-padding fallback in TmFramer for short inputs. Simpler for downstream, but it re-opens TM Framer layering #5779 on the SDLS path (plaintext idle bytes appended after the encrypted payload).

Option 1 is consistent with #5836 / #5779; which do you prefer?

For the record, the other two external failures are: Zephyr — its ComCcsdsConfig.fpp override needs the new Aggregator.aggregationSize (expected #5928 fallout); YAMCS — the runner's OpenSSL 3.0.2 vs the AES-GCM >= 3.5 requirement, failing identically on unrelated PRs.

lestarch-autobot · 2026-09-11T09:27:36Z

+                      "ComCcsdsConfig.Aggregator.aggregationSize must fit the TM Transfer Frame Data Field");
+        ComCcsds::aggregator.configure(ComCcsdsConfig::Aggregator::aggregationSize,
+                                       ComCcsdsConfig::Aggregator::enablePacketSpanning,
+                                       2,


[Maintainability] suggestion maint-clever-code: the allocation id 2 is only correct because 0 (comQueue, commsBufferManager) and 1 (frameAccumulator) are taken elsewhere in this file, but that scheme is written nowhere in the code.

The next engineer adding an allocator client has to grep every configure/setup call to learn which ids are free; a one-line note at the newest site records the scheme.

Suggested change

2,

2, // Allocation id: 0 = comQueue/commsBufferManager, 1 = frameAccumulator

Fixed in dd414e1 (comment added as suggested).

[Maintainability] Fixed in dd414e1.

lestarch-autobot · 2026-09-11T09:29:30Z

+
+            # Same check as PAUSE_CHECK. All transitions into a choice must carry the same type:
+            # PAUSE_CHECK is entered with FwIndexType (via DEQUEUE_CHECK), so the I32 resume needs its own.
+            choice RESUME_I32_PAUSE_CHECK {


[Documentation] could fix New interpreter choice not reflected in the SDD state-machine diagram: Svc/WasmSequencer/docs/InterpreterStateMachine.svg (embedded in docs/sdd.md §Interpreter) still routes hostResumeI32 into PAUSE_CHECK and has no RESUME_I32_PAUSE_CHECK node. Behavior is unchanged, so this is diagram drift only; please regenerate the diagram (or note the equivalent choice in the SDD text) so the model and its SDD stay in sync.

Moot here: the WasmSequencer change was reverted in 51f6826 and will be a separate PR (with the diagram brought in sync there).

[Documentation] Fixed in dd414e1.

…r-instance

lestarch-autobot

Automated review summary (run 7)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	2	0	1	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	1	1	0	1	Go
Documentation Currency	0	2	5	0	0	Go
Design	0	1	4	0	3	Go
Architecture	0	0	0	0	0	Go
Test Quality	0	0	5	0	3	Go
Correctness	0	0	0	0	0	Go
Operational	1	2	5	1	1	Go
Maintainability	0	2	1	0	0	Go
CI safety	—	—	—	—	—	Go
Totals	1	8	23	1	9	Go

Since last run

Agent	resolved	still open	newly added	incorrect-fix follow-ups	improperly resolved	disagreements escalated
Security Vulnerabilities	0	1	0	0	0	0
Supply Chain / Runner Safety	0	0	0	0	0	0
F Prime C/C++ Design	0	1	0	0	0	0
Documentation Currency	0	0	0	0	0	0
Design	0	3	0	0	0	0
Architecture	0	0	0	0	0	0
Test Quality	0	3	0	0	0	0
Correctness	0	0	0	0	0	0
Operational	0	2	0	0	0	0
Maintainability	0	0	0	0	0	0

Duplicates consolidated this run: 0 (threads closed by the §5h post-pass)

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Merge readiness

Merge readiness: Go — all 10 reviewers completed with zero outstanding must-fix; head af8acbb is the prior reviewed head plus a devel merge with no PR-authored changes. Nine non-blocking threads (suggestion / could-fix) remain open for the contributor's discretion.

Steady as she goes — a quiet devel sync, no new findings, and the deck is clear for maintainer review.

cleanup() no longer invokes dataReturnOut: no other component emits on an output port during teardown, and the held packet's owner reclaims it at teardown. Documented in the cleanup() doxygen comment and sdd.md, including the caveat that the aggregation state machine is not reset by cleanup(). Removed dead death-test assertions (they run in the child process and can never fail in the parent) and renamed test_cleanup_returns_held_packet to test_cleanup_drops_held_packet with updated expectations. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

lestarch-autobot · 2026-09-22T18:28:26Z

+further data or status can arrive (i.e. after the component's task has stopped). A packet held for the next
+aggregate is dropped rather than returned through `dataReturnOut` (its owner reclaims it at teardown) and the
+buffer, held-packet, and idle-fill state is reset. The aggregation state machine is not reset, so a subsequent
+`configure()` is allowed but data flow afterwards depends on the state the machine was left in.


[Operational] could fix ops-doc-reality: "a subsequent configure() is allowed" is not reachable when a packet was held. Scenario: cleanup() runs with m_held valid (a non-spanning aggregate left a packet for the next frame, the common case at a quiet shutdown) and now drops it without dataReturnOut. The held Fw::Buffer came from ComQueue::sendComBuffer/sendBuffer, which flipped m_buffer_state to UNOWNED and only flips it back in ComQueue::dataReturnIn_handler. Consequence: after cleanup(); configure(...), the first ComQueue::processQueue() hits FW_ASSERT(m_buffer_state == OWNED) (ComQueue.cpp:487 / :417) — an FSW abort on the first downlink item, not the "data flow depends on the machine state" the SDD describes; for file-type buffers the owner (FileDownlink) is never given its buffer back either. Teardown-only use is fine (BufferManager releases its pool unconditionally). Smallest remedy: narrow the claim to teardown-only, or state that re-configure() is valid only if no packet was held (or restate that the upstream sender's ownership flag is never returned). Judgment call: only matters if in-flight reconfiguration is an intended use.

Suggested change

`configure()` is allowed but data flow afterwards depends on the state the machine was left in.

`configure()` is allowed only at teardown-style reuse where no packet was held: a dropped held packet leaves its

upstream sender (`Svc.ComQueue`) waiting for a return that never comes, so the next send asserts.

lestarch-autobot · 2026-09-22T18:29:50Z

 fprime-visual==1.0.2
 fprime-xtce==0.1.3
-fprime-yamcs==0.2.1
+fprime-yamcs==0.2.2


[Design] could fix design-scope-creep: an unrelated GDS dependency bump (fprime-yamcs 0.2.1 → 0.2.2) rides in this CCSDS aggregation PR.

Commit 6abb7a1 changes only requirements.txt; neither the PR description nor the linked issues (#5836, #5779, #5928) mention YAMCS. If 0.2.2 is needed so the YAMCS deframer handles the aggregator's idle packets / fixed-size aggregates, say so in the description (it is then a deliberate part of the change); otherwise it belongs in its own PR so it is reviewed and bisectable on its own.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

lestarch-autobot

Automated review summary (run 8)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	2	0	1	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	1	1	0	0	Go
Documentation Currency	0	2	5	0	0	Go
Design	0	1	5	0	3	Go
Architecture	0	0	0	0	0	Go
Test Quality	0	0	5	0	1	Go
Correctness	0	0	0	0	0	Go
Operational	1	2	6	1	2	Go
Maintainability	0	2	1	0	0	Go
CI safety	—	—	—	—	—	Go
Totals	1	8	25	1	7	Go

Since last run

Agent	resolved	still open	newly added
Security Vulnerabilities	0	1	0
Supply Chain / Runner Safety	0	0	0
F Prime C/C++ Design	1	0	0
Documentation Currency	0	0	0
Design	1	2	1
Architecture	0	0	0
Test Quality	2	1	0
Correctness	0	0	0
Operational	0	2	1
Maintainability	0	0	0

Duplicates consolidated this run: 0 (threads closed by the §5h post-pass)

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Merge readiness

Merge readiness: Go — all 10 reviewers completed with zero outstanding must-fix; the 7 outstanding items are below-must-fix (2 new could-fix this run: the unrelated fprime-yamcs 0.2.1→0.2.2 bump riding in this PR, and the SDD "subsequent configure() is allowed" claim vs. cleanup() dropping a held packet; plus the open TmFramer exact-size assert disagreement awaiting maintainer adjudication).

Eighth pass, telemetry still nominal — the aggregator crew clears this frame for downlink.

thomas-bc and others added 3 commits September 11, 2026 00:16

ComAggregator: review follow-ups (test cleanup, SDLS SA-index static_…

00f9b7f

…assert anchor, docs) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

ComCcsdsSdls: bound aggregate size by the TM data field minus SA inde…

7a9b11d

…x; document encryptor overhead (nasa#5795) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

devin-ai-integration Bot force-pushed the devin/1789073969-comaggregator-idle-fill-per-instance branch from 4397b4d to 7a9b11d Compare September 11, 2026 00:56

thomas-bc commented Sep 11, 2026

View reviewed changes

Comment thread Svc/ComAggregator/ComAggregator.cpp Outdated

lestarch-autobot reviewed Sep 11, 2026

View reviewed changes

lestarch-autobot suggested changes Sep 11, 2026

View reviewed changes

thomas-bc and others added 2 commits September 11, 2026 03:35

lestarch-autobot reviewed Sep 11, 2026

View reviewed changes

thomas-bc-autowot mentioned this pull request Sep 17, 2026

Update for nasa/fprime#5939: bump fprime_gds to pr-5939 fprime-community/fprime_cfs_reference#32

Open

Merge branch 'devel' into devin/1789073969-comaggregator-idle-fill-pe…

af8acbb

…r-instance

lestarch-autobot reviewed Sep 22, 2026

View reviewed changes

lestarch-autobot approved these changes Sep 22, 2026

View reviewed changes

devin-ai-integration Bot mentioned this pull request Sep 22, 2026

Look up dictionary constants by name JPL-Devin/fprime-yamcs#2

Closed

thomas-bc-autowot mentioned this pull request Sep 22, 2026

Look up dictionary constants by name fprime-community/fprime-yamcs#29

Merged

Update fprime-yamcs version to 0.2.2

6abb7a1

thomas-bc commented Sep 22, 2026

View reviewed changes

Comment thread Svc/ComAggregator/ComAggregator.cpp Outdated

lestarch-autobot reviewed Sep 22, 2026

View reviewed changes

lestarch-autobot approved these changes Sep 22, 2026

View reviewed changes

		// Restore the size in case a downstream consumer shrank the buffer before returning it
		this->m_frameBuffer.setSize(this->m_frameSerializer.getSize());

-    @ next to a minimum idle packet: at least max(FW_COM_BUFFER_MAX_SIZE, FW_FILE_BUFFER_MAX_SIZE) + 6 + 7 bytes.
+    @ next to a minimum idle packet: at least max(FW_COM_BUFFER_MAX_SIZE, FW_FILE_BUFFER_MAX_SIZE) + 6 + 7 bytes.
+    @ Throughput: without spanning, N full file packets per frame need N * (FW_FILE_BUFFER_MAX_SIZE + 6) bytes exactly
+    @ or plus 7; with the defaults (512, 1024) only one 518-byte file packet fits per 1016-byte data field (~50% link
+    @ efficiency for file downlink). Enable packet spanning or size the data field as a multiple of the file packet.

	2,
	2, // Allocation id: 0 = comQueue/commsBufferManager, 1 = frameAccumulator

	`configure()` is allowed but data flow afterwards depends on the state the machine was left in.
	`configure()` is allowed only at teardown-style reuse where no packet was held: a dropped held packet leaves its
	upstream sender (`Svc.ComQueue`) waiting for a return that never comes, so the next send asserts.

Conversation

thomas-bc commented Sep 11, 2026

Change Description

Rationale

Testing/Review Recommendations

Future Work

AI Usage (see policy)

Uh oh!

github-actions Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report — base devel

Regressions

Modules changed

New modules

Modules without UTs

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions Bot commented Sep 11, 2026 •

edited

Loading

Coverage report — base `devel`