Svc/Ccsds/AosFramer: continue striped packets on comStatusIn, one status per message by ZabaHD4K · Pull Request #6334 · nasa/fprime

ZabaHD4K · 2026-11-05T19:13:06Z


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

Change Description

Implements the fix @Willmac16 outlined on #5888: the packet-continue logic moves from dataReturnIn into comStatusIn, and a status only goes upstream when it is not answering an intermediate frame of a packet still being striped.

dataReturnIn_handler now only reclaims ownership of the frame buffer. Packing the outstanding tail there emitted a second SUCCESS for one transmitted frame whenever the tail did not fill the new frame (check_and_send_vc's not-full branch), on top of the unconditionally forwarded adapter status — which Svc::ComQueue::comStatusIn_handler asserts on when it arrives in READY.
comStatusIn_handler: a SUCCESS received while a tail is outstanding is consumed to continue that packet — the frame buffer is guaranteed owned at that point, since the communication adapter interface requires the dataReturnOut call before the comStatusOut call it applies to (asserted). If the tail fills the frame, the next frame goes out and its own status is awaited; if it leaves room, check_and_send_vc emits exactly one SUCCESS upstream asking for more data. With no tail outstanding, statuses pass through as before. FAILURE always passes through (pausing the queue); a subsequent recovery SUCCESS with a tail outstanding resumes striping it.
Net invariant, now documented in the SDD: exactly one status reaches Svc::ComQueue per dataIn message, whether the message produced 0, 1, or N frames. Previously a message striped across N frames forwarded N statuses.

Unit tests (no framing test asserted on comStatusOut or simulated comStatusIn before, as the issue notes):

testTailPackingSingleStatus: the exact repro — a packet whose tail spans a second frame yields exactly one forwarded status, in both the sendNow (tail completes a second frame) and non-sendNow (tail waits in a partial frame, one SUCCESS emitted) variants.
testTailPackingFailureThenRecovery: FAILURE with a tail outstanding is forwarded without packing; the recovery SUCCESS is consumed to resume striping; the completed frame's status is then forwarded.
testLongPacket now drives the adapter side (dataReturnIn then comStatusIn per frame, via a new ackFrame helper) and asserts one status upstream for the three-frame packet; testShortPackets asserts ten messages yield ten statuses.

Rationale

Fixes #5888 (CCB: Approved). One transmitted frame produced two SUCCESS statuses on comStatusOut when the driver returned the buffer synchronously and the packet tail did not fill the new frame; Svc::ComQueue asserts on the second status (ComQueue.cpp READY check), taking down the deployment.

Testing/Review Recommendations

fprime-util check in Svc/Ccsds/AosFramer passes on Linux x86_64 with the sanitizers enabled by the UT build.

Review focus: the status bookkeeping against the communication adapter interface — in particular that consuming the recovery SUCCESS after a FAILURE (rule 4) to resume an outstanding tail preserves the pause/resume semantics: the queue stays paused until the resumed frame's own status (or the partial-frame SUCCESS) reaches it.

Future Work

Independent of #6333 (EPP idle fill, same component, different functions); whichever merges second may need a trivial rebase of the test file.

Contributor Checklist

I have the rights to contribute this change set under the Apache-2.0 license.
I agree to be bound by the contributor guidelines.

AI Usage (see policy)

An AI coding assistant was used, under my direction, to draft the implementation, the unit tests, and the SDD update. All changes were reviewed by me and verified by building and running the full Svc/Ccsds/AosFramer unit test suite locally.

This contribution follows the F´ AI development skills (entered through AGENTS.md).

…tus per message When dataIn filled a frame, the frame was sent and the packet tail was packed on dataReturnIn; if the tail did not fill the new frame, check_and_send_vc emitted a SUCCESS on comStatusOut, and the adapter status for the transmitted frame was then forwarded unconditionally by comStatusIn. One transmitted frame therefore yielded two SUCCESS statuses, and Svc::ComQueue asserts when a status arrives in READY. A packet striped across N frames likewise forwarded N statuses for one message. Move the continue logic into comStatusIn, as the component maintainer outlined on the issue: a SUCCESS received while a tail is outstanding is consumed to continue the packet (the interface requires dataReturnOut before the comStatusOut it applies to, so the frame buffer is owned, asserted); a full frame goes out and awaits its own status, a partial one asks upstream for more data. Statuses with no tail outstanding, and every FAILURE, pass through; a recovery SUCCESS resumes striping an outstanding tail. Exactly one status now reaches the upstream queue per dataIn message. dataReturnIn only reclaims buffer ownership. Documents the protocol in the SDD; unit tests drive the adapter side and assert on comStatusOut, which no framing test did before. Fixes nasa#5888

lestarch-autobot · 2026-11-05T21:31:40Z

+        return;
+    }
+
+    // No tail outstanding (or a FAILURE, which pauses the upstream queue): forward the status.


[Design] future work Resuming an outstanding tail after FAILURE emits a continuation frame whose head frame the adapter already dropped.

Svc::ComStub does not retransmit on FAILURE (its RETRY_LIMIT / reinitialize paths return the buffer and skip the data), so the frame carrying the start of the packet is gone; the recovery SUCCESS then stripes the remaining bytes into an M_PDU continuation-only frame the deframer can only discard. Preexisting (the old dataReturnIn path did the same), but this PR makes it the documented design; dropping the tail (returning its buffer) on FAILURE and passing the recovery SUCCESS through would save the dead frame while keeping one SUCCESS per message.

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

lestarch-autobot · 2026-11-05T21:35:18Z

+        // requires the dataReturnOut call before the comStatusOut call it applies to
+        FW_ASSERT(currentVc.frame.state == BufferOwnershipState::OWNED,
+                  static_cast<FwAssertArgType>(currentVc.frame.state));
+        this->pack_pad_send(currentVc.outstanding.packet, currentVc.outstanding.context, currentVc.outstanding.offset);


[Operational] future work ops-timing-resource: with a synchronous adapter, a striped packet recurses one call-stack level per frame on the ComQueue thread.

Scenario: Svc::ComStub (sync drvSendOut) and Svc::ComRetry emit comStatusOut from inside their dataIn call, so this pack_pad_send re-enters dataOut_out → adapter dataIn → comStatusOut → comStatusIn_handler before the previous level unwinds; depth = ceil(packet_bytes / frame_payload). At the default 1536-byte frame (1526 payload with FECF) a 512-byte FW_COM_BUFFER_MAX_SIZE message nests 1 level, but at the minimum legal configure size (11 bytes, 1 payload byte) the same message nests 512 levels of adapter+framer+driver send() frames — a task-stack overflow on an RTOS, not an assert. Preexisting: the old dataReturnIn path nested identically, and this PR neither deepens nor shortens it. Operational judgment call; smallest remedy is an SDD WARNING that frame payload must be sized against the largest dataIn message when a synchronous adapter is used.

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

lestarch-autobot

Automated review summary (run 1)

Per-agent results

Findings open at the reviewed head, by tag; everything ever raised is under Since last run.

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	0	0	0	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	0	0	0	0	Go
Documentation Currency	1	0	0	1	2	Go (metadata) → No-Go after §5j promotion
Design	1	0	1	1	3	Go (metadata) → No-Go after §5j promotion
Architecture	0	0	0	0	0	Go
Test Quality	0	3	0	0	3	Go
Correctness	0	0	0	0	0	Go
Operational	1	0	1	1	3	Go (metadata) → No-Go after §5j promotion
Maintainability	0	0	4	0	4	Go
CI safety	—	—	—	—	—	Go
Totals	3	3	5	3	14	No-Go

Totals count each of the 14 open threads once: the Operational could fix is a concurrence on the Design dataIn guard thread (column sum 15). The Design AosFramer.hpp:111 and Operational docs/sdd.md:29 must-fix items describe the same overstated invariant at two different sites (distinct site-keys, so not consolidated); one fix should close both.

Duplicates consolidated this run: 0

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 (3)

Documentation Currency

docs/sdd.md port-table row for comStatusOut ("passes status through") is superseded by the new Internals paragraph (promoted from could fix) — #6334 (comment)

Design

AosFramer.hpp Doxygen "exactly one status reaches the upstream queue per dataIn message" is false on the FAILURE-then-recovery path (promoted from could fix) — #6334 (comment)

Operational

docs/sdd.md:29 "exactly one status reaches Svc::ComQueue per dataIn message" is false on the failure path (promoted from suggestion) — #6334 (comment)

Severity reconciliation (3 promoted)

Finding	Reviewer tag	Summary tag	Consequence	Link
SDD port-table row "passes status through" superseded by new Internals paragraph	could fix	must fix	superseded `docs/sdd.md` sentence left beside its replacement (§14 row 3)	#6334 (comment)
`AosFramer.hpp` Doxygen one-status invariant overstated	could fix	must fix	Doxygen claim stated false on the FAILURE path (§14 row 3)	#6334 (comment)
`docs/sdd.md:29` "exactly one status reaches `Svc::ComQueue`"	suggestion	must fix	SDD claim stated false on the failure path (§14 row 3)	#6334 (comment)
`dataReturnIn_handler` comment narrates removed behavior	could fix	could fix (not promoted)	inline implementation comment, not an SDD/user-manual/Doxygen claim; rationale frames it as past-tense history, not a false statement	#6334 (comment)
dead `outContext` in `testLongPacket`	could fix	could fix (not promoted)	no §14 row — dead test local	#6334 (comment)
dead `outContext` in `testShortPackets`	could fix	could fix (not promoted)	no §14 row — dead test local	#6334 (comment)
`ackFrame` defined outside the Helper functions section	could fix	could fix (not promoted)	no §14 row — file organization	#6334 (comment)
`dataIn` has no guard for an outstanding tail (Operational concurs: buffer leak in the same window)	could fix	could fix (not promoted)	consequence conditioned on a hypothetical mis-wired topology / early-return adapter; no ground- or hardware-driven entry point or error path named	#6334 (comment)
count-only `comStatusOut` assertions in `testShortPackets`	suggestion	suggestion (not promoted)	no §14 row — test assertion strength	#6334 (comment)
no test asserts `dataReturnOut` on the moved continue path	suggestion	suggestion (not promoted)	no §14 row — test coverage gap	#6334 (comment)
new `FW_ASSERT(frame.state == OWNED)` has no death test	suggestion	suggestion (not promoted)	no §14 row — test coverage gap	#6334 (comment)
reference Framer Status Protocol table lacks an N-frame row	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
continuation frame emitted after `FAILURE` dropped its head frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
synchronous-adapter recursion depth per striped frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)

Merge readiness

Merge readiness: No-Go — 3 outstanding must-fix items after severity reconciliation (Documentation Currency 1, Design 1, Operational 1); all ten lenses completed at af42b99 and reported Go on their own tags.

The status protocol now holds one SUCCESS per message — true up the three sentences that say otherwise and this framer is cleared for the next burn.

- State the invariant as one SUCCESS per dataIn message (a FAILURE is forwarded and followed by its recovery SUCCESS) in the SDD, the handler doc and the handler comment; update the stale comStatusOut port-table row - Assert in dataIn that no packet tail is outstanding, so a topology that bypasses the framer with the adapter status fails loudly instead of silently overwriting the tail - Trim the dataReturnIn comment to the current behavior - Tests: assert dataReturnOut hand-off around the consumed status, death test for a status arriving before the frame buffer returns, assert the tenth status value in testShortPackets and correct its narration, drop two dead locals, move ackFrame beside the other helpers

ZabaHD4K · 2026-11-06T19:42:05Z

All actionable run-1 findings addressed in a2ffe3a:

[Summary-promoted must fix ×3] the one-status quantifier and the stale port row → the invariant is now stated as exactly one SUCCESS per dataIn message (a FAILURE is always forwarded and is followed by the recovery SUCCESS) in the handler Doxygen, the handler comment and the SDD Internals paragraph; the comStatusOut port-table row now reads "Sends status to upstream Svc::ComQueue, exactly one SUCCESS per dataIn message, per the Framer Status Protocol".
[Design, could fix] dataIn with a tail outstanding (adapter status mis-wired past the framer) → new FW_ASSERT(!outstanding.packet.isValid()) beside the existing OWNED assert, so the mis-wiring fails loudly instead of silently dropping the tail bytes.
[Maintainability, could fix ×3] past-tense dataReturnIn comment trimmed to the current behavior; both dead outContext locals deleted; ackFrame definition moved down to the Helper functions section.
[Test Quality, suggestion ×3] testShortPackets now asserts the tenth status value and its narration is corrected (the 6-byte remainder stripes the SPP idle tail, so that SUCCESS is emitted by check_and_send_vc after the consumed ack, not forwarded); testTailPackingSingleStatus pins the dataReturnOut hand-off (0 while the tail is outstanding, incremented once it is packed) and ends with the death test for a SUCCESS arriving before the frame buffer returns.

Not addressed here, deliberately:

[Documentation, future work] the missing N-frame row in the communication-adapter-interface.md protocol table predates this PR — happy to file it as a follow-up docs PR if wanted.
[Design, future work] dropping the tail on FAILURE instead of resuming it (the head frame is already lost) — preexisting behavior this PR keeps; awaiting maintainer direction on the cc'd thread.
[Operational, future work] call-stack recursion with synchronous adapters at tiny frame sizes — preexisting, orthogonal to the status fix.

fprime-util check passes locally after the changes (9/9, sanitizers on).

lestarch-autobot · 2026-11-06T21:34:38Z

@@ -26,6 +26,8 @@ The AOS protocol specifies a fixed frame size. The maximum for all AOS framers c

 The `Svc::Ccsds::AosFramer` uses an internal (member) buffer to hold the fixed size frame. The buffer **must** be returned to the AosFramer via the `dataReturnIn` port once it has been used or consumed. When the buffer returns to the AosFramer 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 AosFramer, data should be copied before the original buffer is returned to the AosFramer via the `dataReturnIn` port. 


[Operational] could fix ops-doc-reality: the Internals section documents the consume-and-continue mechanism but not the two fail-loud outcomes this PR adds (AosFramer.cpp:85 and :135).

Scenario: a topology that wires the adapter's comStatusOut straight to Svc::ComQueue (bypassing the framer), or an adapter that reports status before returning the frame, now asserts deterministically at the first message longer than the frame payload (1526 bytes at the default 1536-byte FECF frame) or the first sendNow frame left with 1–6 spare bytes (idle-tail spill) — a deployment-wide FW_ASSERT at integration where the pre-PR code ran (and silently corrupted). Below must-fix: the asserts are the intended remedy for a wiring error and no in-repo topology instantiates AosFramer; operational judgment call — the smallest remedy is one SDD sentence stating the two ordering constraints and that they are asserted.

Suggested change

The `Svc::Ccsds::AosFramer` uses an internal (member) buffer to hold the fixed size frame. The buffer **must** be returned to the AosFramer via the `dataReturnIn` port once it has been used or consumed. When the buffer returns to the AosFramer 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 AosFramer, data should be copied before the original buffer is returned to the AosFramer via the `dataReturnIn` port.

The `Svc::Ccsds::AosFramer` uses an internal (member) buffer to hold the fixed size frame. The buffer **must** be returned to the AosFramer via the `dataReturnIn` port once it has been used or consumed, and it must be returned before the status that applies to it is reported on `comStatusIn`; the adapter's status must reach `Svc::ComQueue` only through this framer. Both orderings are asserted: a `SUCCESS` arriving for an outstanding packet tail before the frame is returned, or a `dataIn` message arriving while a tail is still outstanding (adapter status wired past the framer), trips an `FW_ASSERT` at the first message longer than the frame payload (1526 bytes at the default 1536-byte frame with FECF) or the first `sendNow` frame left with 1–6 spare bytes. When the buffer returns to the AosFramer 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 AosFramer, data should be copied before the original buffer is returned to the AosFramer via the `dataReturnIn` port.

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

Superseded by re-review run 2.

lestarch-autobot

Automated review summary (run 2)

Per-agent results

Findings open at the reviewed head, by tag; everything ever raised is under Since last run.

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

All three run-1 must-fix items (the §5j-promoted one-status claims in docs/sdd.md:29, docs/sdd.md:40 and AosFramer.hpp:111) were fixed in a2ffe3a and their threads resolved; no thread is shared by concurrence or consolidation this run, so Totals equal the column sums.

Since last run

Agent	raised (all runs)	resolved	still open	newly added	incorrect-fix follow-ups
Security Vulnerabilities	0	0	0	0	0
Supply Chain / Runner Safety	0	0	0	0	0
F Prime C/C++ Design	0	0	0	0	0
Documentation Currency	2	1	1	0	0
Design	3	2	1	0	0
Architecture	0	0	0	0	0
Test Quality	5	3	0	2	1
Correctness	0	0	0	0	0
Operational	4	2	1	1	0
Maintainability	4	4	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

Severity reconciliation (0 promoted)

Finding	Reviewer tag	Summary tag	Consequence	Link
`dataReturnOut` is counted but its payload (buffer, context) is never asserted on the moved continue path	suggestion	suggestion (not promoted)	no §14 row — test assertion strength	#6334 (comment)
new `FW_ASSERT(!currentVc.outstanding.packet.isValid())` in `dataIn_handler` has no death test	suggestion	suggestion (not promoted)	no §14 row — test coverage gap	#6334 (comment)
SDD Internals omits the two new fail-loud ordering asserts (`comStatusIn` before `dataReturnIn`; `dataIn` with a tail outstanding)	could fix	could fix (not promoted)	rationale describes a missing sentence, not a claim the PR makes false (§14 row 3 unmet); the asserts it names are driven by topology wiring, not a ground- or hardware-supplied value (§14 row 1 unmet); reviewer explicitly places it below must-fix	#6334 (comment)
reference Framer Status Protocol table lacks an N-frame row	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
continuation frame emitted after `FAILURE` dropped its head frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
synchronous-adapter recursion depth per striped frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)

Merge readiness

Merge readiness: Go — all ten lenses completed at a2ffe3a and report Go; zero outstanding must-fix items after severity reconciliation (the three run-1 promotions were addressed and resolved). Six below-must-fix threads remain open for the maintainers' discretion: two Test Quality suggestions, one Operational could-fix (SDD wording), and three future-work notes. Core maintainers @LeStarch and @thomas-bc have been requested for human review.

Agents that did not run on this PR

none — every registry reviewer ran at a2ffe3a.

One SUCCESS per message, every frame accounted for — this framer is go for launch; over to mission control for the final poll.

- Assert the dataReturnOut payload: the message buffer and context that come back on the moved continue path are the ones the message arrived with - Death-test the new dataIn guard: returning the frame without acking it leaves the tail outstanding, and a new message in that state asserts

ZabaHD4K · 2026-11-08T19:33:22Z

Applied both run-2 test suggestions in the latest commit: the dataReturnOut payload (buffer and context) is now asserted on the consumed-status continue path, and the new dataIn tail-outstanding guard is death-tested (frame returned without its ack, then a new message). fprime-util check passes locally (9/9). The remaining open items are the three future-work threads noted above.

lestarch-autobot

Automated review summary (run 3)

Per-agent results

Findings open at the reviewed head, by tag; everything ever raised is under Since last run.

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

The delta since run 2 (a2ffe3a → ecd5b92) is one test-only commit that adopted both run-2 Test Quality suggestions (dataReturnOut payload asserts, dataIn tail-outstanding death test); both threads are resolved. No thread is shared by concurrence or consolidation this run, so Totals equal the column sums.

Since last run

Agent	raised (all runs)	resolved	still open
Security Vulnerabilities	0	0	0
Supply Chain / Runner Safety	0	0	0
F Prime C/C++ Design	0	0	0
Documentation Currency	2	0	1
Design	3	0	1
Architecture	0	0	0
Test Quality	5	2	0
Correctness	0	0	0
Operational	4	0	2
Maintainability	4	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

Severity reconciliation (0 promoted)

Finding	Reviewer tag	Summary tag	Consequence	Link
SDD Internals omits the two new fail-loud ordering asserts (`comStatusIn` before `dataReturnIn`; `dataIn` with a tail outstanding)	could fix	could fix (not promoted)	rationale describes a missing sentence, not a claim the PR makes false (§14 row 3 unmet); the asserts it names are driven by topology wiring / adapter ordering, not a ground- or hardware-supplied value (§14 row 1 unmet); reviewer explicitly places it below must-fix	#6334 (comment)
reference Framer Status Protocol table lacks an N-frame row	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
continuation frame emitted after `FAILURE` dropped its head frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
synchronous-adapter recursion depth per striped frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)

Merge readiness

Merge readiness: Go — all ten lenses completed at ecd5b92 and report Go; zero outstanding must-fix items after severity reconciliation. Four below-must-fix threads remain open for the maintainers' discretion: one Operational could-fix (SDD wording) and three future-work notes. Core maintainers @LeStarch and @thomas-bc were requested for human review on run 2 and remain requested.

Agents that did not run on this PR

none — every registry reviewer ran at ecd5b92.

Telemetry nominal on the third pass: the test deck is buttoned up and this framer still reads go — mission control, the poll is yours.

ZabaHD4K · 2026-11-10T14:31:23Z

@LeStarch @thomas-bc gentle ping — this one is ready for human review whenever you get a chance: three automated review runs are green (no open must-fix/suggestion items, only the three future-work threads), and per @Willmac16's guidance it's the single active PR from me (#6333 is parked in draft until this merges). Happy to rebase or adjust if anything is needed. CI workflows are also pending first-time-contributor approval.

The protocol table listed only the 0- and 1-frame cases; a message striped across N frames had its rule written down nowhere but the AosFramer SDD. State it in the reference catalog: consume the SUCCESS statuses of the first N-1 frames to continue the message, pass through the Nth, so exactly one SUCCESS reaches Svc::ComQueue per message.

ZabaHD4K · 2026-11-11T15:34:01Z

Folded the documentation future-work item into this PR: the Framer Status Protocol table in docs/reference/communication-adapter-interface.md now has the N-frame striping row (consume the first N-1 statuses, pass through the Nth — one SUCCESS per message), since it documents exactly the behavior this PR implements. The other two future-work threads (dropping the tail on FAILURE, and recursion depth with synchronous adapters) are design questions I'm leaving for maintainer direction, as noted.

lestarch-autobot · 2026-11-11T18:29:21Z

 |-----------------|----------------------------------------|----------------------------------------------------------|
 | 0               | Send one `Fw::Success::SUCCESS` status | Data must continue to flow to produce a frame            |
 | 1               | Pass through received status from ComQueue | Frame produced and communication adapter produces status |
+| N > 1 (one message striped across N frames) | Consume the `SUCCESS` statuses of the first N-1 frames to continue the message; pass through the status of the Nth | Exactly one `SUCCESS` must reach `Svc::ComQueue` per message; intermediate-frame statuses drive the striping, not the queue (see `Svc::Ccsds::AosFramer`) |


[Operational] could fix ops-doc-reality: the N > 1 row says the Nth frame's status is passed through, but that holds only when the tail exactly fills (or sendNow idle-fills) frame N.

Scenario: an 86-byte tail lands in a partial frame N without sendNow → check_and_send_vc (AosFramer.cpp:285) emits a framer-generated SUCCESS upstream immediately (the 0-frame row), and frame N's own status arrives only after a later message fills it and is passed through for that message (testTailPackingSingleStatus, second packet: dataOut 3, comStatusOut 2). Operational consequence: the one SUCCESS Svc::ComQueue receives for a striped message can precede transmission of its last bytes, which wait in the framer's partial frame until the next message or a sendNow. Below must-fix because the rationale column's invariant (exactly one SUCCESS per message) holds in both cases; only the attribution of which status is incomplete. Smallest remedy: state the partial-tail composition in the row.

Suggested change

| N > 1 (one message striped across N frames) | Consume the `SUCCESS` statuses of the first N-1 frames to continue the message; pass through the status of the Nth | Exactly one `SUCCESS` must reach `Svc::ComQueue` per message; intermediate-frame statuses drive the striping, not the queue (see `Svc::Ccsds::AosFramer`) |

| N > 1 (one message striped across N frames) | Consume the `SUCCESS` statuses of the first N-1 frames to continue the message. If the tail exactly fills (or `sendNow` idle-fills) the Nth frame, pass through the status of the Nth; if the tail leaves the Nth frame partially filled, send one `SUCCESS` directly (as in the 0-frame row) and pass through the Nth frame's eventual status for the later message that completes that frame | Exactly one `SUCCESS` must reach `Svc::ComQueue` per message; intermediate-frame statuses drive the striping, not the queue. The `SUCCESS` for a message whose tail sits in a partial frame precedes transmission of that tail (see `Svc::Ccsds::AosFramer`) |

lestarch-autobot

Automated review summary (run 4)

Per-agent results

Findings open at the reviewed head, by tag; everything ever raised is under Since last run.

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

The delta since run 3 (ecd5b92 → 0c74b06) is one documentation-only commit adding the N > 1 striping row to the Framer Status Protocol table in docs/reference/communication-adapter-interface.md; it adopts the run-1 Documentation future-work note (now resolved) and drew one new Operational could-fix on the wording of that row. No thread is shared by concurrence or consolidation this run, so Totals equal the column sums.

Since last run

Agent	raised (all runs)	resolved	still open	newly added
Security Vulnerabilities	0	0	0	0
Supply Chain / Runner Safety	0	0	0	0
F Prime C/C++ Design	0	0	0	0
Documentation Currency	2	1	0	0
Design	3	0	1	0
Architecture	0	0	0	0
Test Quality	5	0	0	0
Correctness	0	0	0	0
Operational	5	0	2	1
Maintainability	4	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

Severity reconciliation (0 promoted)

Finding	Reviewer tag	Summary tag	Consequence	Link
reference `N > 1` row attributes the message's `SUCCESS` to the Nth frame's pass-through even when a partial tail yields a framer-generated `SUCCESS`	could fix	could fix (not promoted)	rationale states the row's invariant (exactly one `SUCCESS` per message) holds in both cases and that only the attribution is incomplete — an omitted case, not a claim stated false (§14 row 3 unmet); no other row applies	#6334 (comment)
SDD Internals omits the two new fail-loud ordering asserts (`comStatusIn` before `dataReturnIn`; `dataIn` with a tail outstanding)	could fix	could fix (not promoted)	rationale describes a missing sentence, not a claim the PR makes false (§14 row 3 unmet); the asserts it names are driven by topology wiring / adapter ordering, not a ground- or hardware-supplied value (§14 row 1 unmet); reviewer explicitly places it below must-fix	#6334 (comment)
continuation frame emitted after `FAILURE` dropped its head frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)
synchronous-adapter recursion depth per striped frame	future work	future work (not promoted)	future-work tier is never promoted (§5j guardrail)	#6334 (comment)

Merge readiness

Merge readiness: Go — all ten lenses completed at 0c74b06 and report Go; zero outstanding must-fix items after severity reconciliation. Four below-must-fix threads remain open for the maintainers' discretion: two Operational could-fixes (SDD Internals sentence; reference-table row wording) and two future-work notes. Core maintainers @LeStarch and @thomas-bc were requested for human review on run 2 and remain requested.

Agents that did not run on this PR

none — every registry reviewer ran at 0c74b06.

Fourth orbit, one new line in the flight manual, and every console still reads green — handing back to the crew.

ZabaHD4K mentioned this pull request Nov 5, 2026

Svc/Ccsds/AosFramer: tail-packing after dataReturnIn emits a second SUCCESS for one frame (ComQueue protocol violation) #5888

Open

lestarch-autobot self-requested a review November 5, 2026 19:55

lestarch-autobot reviewed Nov 5, 2026

View reviewed changes

Comment thread Svc/Ccsds/AosFramer/AosFramer.cpp Resolved

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

lestarch-autobot reviewed Nov 5, 2026

View reviewed changes

Comment thread Svc/Ccsds/AosFramer/docs/sdd.md Resolved

Comment thread Svc/Ccsds/AosFramer/docs/sdd.md Resolved

lestarch-autobot reviewed Nov 5, 2026

View reviewed changes

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

lestarch-autobot reviewed Nov 5, 2026

View reviewed changes

lestarch-autobot previously requested changes Nov 5, 2026

View reviewed changes

thomas-bc-autowot added the bug label Nov 6, 2026

lestarch-autobot reviewed Nov 6, 2026

View reviewed changes

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

Comment thread Svc/Ccsds/AosFramer/test/ut/AosFramerTester.cpp Resolved

lestarch-autobot reviewed Nov 6, 2026

View reviewed changes

lestarch-autobot requested review from LeStarch and thomas-bc November 6, 2026 21:46

lestarch-autobot previously approved these changes Nov 6, 2026

View reviewed changes

This was referenced Nov 8, 2026

Svc/Ccsds/AosFramer: EPP idle-fill branch swallows the condition and leaves frames unpadded #5881

Open

Svc/Ccsds/AosFramer: implement EPP idle fill #6333

Draft

ZabaHD4K dismissed lestarch-autobot’s stale review via ecd5b92 November 8, 2026 19:33

lestarch-autobot previously approved these changes Nov 8, 2026

View reviewed changes

ZabaHD4K dismissed lestarch-autobot’s stale review via 0c74b06 November 11, 2026 15:34

lestarch-autobot reviewed Nov 11, 2026

View reviewed changes

lestarch-autobot approved these changes Nov 11, 2026

View reviewed changes

Merge remote-tracking branch 'origin/devel' into aosframer-tail-status

aef3dc7

		@@ -26,6 +26,8 @@ The AOS protocol specifies a fixed frame size. The maximum for all AOS framers c

		The `Svc::Ccsds::AosFramer` uses an internal (member) buffer to hold the fixed size frame. The buffer must be returned to the AosFramer via the `dataReturnIn` port once it has been used or consumed. When the buffer returns to the AosFramer 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 AosFramer, data should be copied before the original buffer is returned to the AosFramer via the `dataReturnIn` port.

Conversation

ZabaHD4K commented Nov 5, 2026

Change Description

Rationale

Testing/Review Recommendations

Future Work

Contributor Checklist

AI Usage (see policy)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lestarch-autobot Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lestarch-autobot Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

lestarch-autobot left a comment

Choose a reason for hiding this comment

Automated review summary (run 1)

Per-agent results

Merge readiness

Uh oh!

ZabaHD4K commented Nov 6, 2026

Uh oh!

Uh oh!

Uh oh!

lestarch-autobot Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

lestarch-autobot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Automated review summary (run 2)

Per-agent results

Merge readiness

Agents that did not run on this PR

Uh oh!

ZabaHD4K commented Nov 8, 2026

Uh oh!

lestarch-autobot left a comment

Choose a reason for hiding this comment

Automated review summary (run 3)

Per-agent results

Merge readiness

Agents that did not run on this PR

Uh oh!

ZabaHD4K commented Nov 10, 2026

Uh oh!

ZabaHD4K commented Nov 11, 2026

Uh oh!

lestarch-autobot Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

lestarch-autobot left a comment

Choose a reason for hiding this comment

Automated review summary (run 4)

Per-agent results

Merge readiness

Agents that did not run on this PR

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lestarch-autobot left a comment •

edited

Loading