A metadata only transaction - a proxy put request, for instance - carries no
file data and completes the moment its metadata arrives. handleTransferCompletion
took the branch for a null checksum, which sets the condition code and touches
neither delivery field, so both were reported at their reset defaults: a
transaction that succeeded announced itself as
Finish Condition: No Error (0)
File delivery code: Data Incomplete (1)
File delivery status: Discard deliberately (0)
which contradicts itself, and goes out in the Finished PDU to the sender, not
only into the OBSW log. It was noticed on the flatsat, where every CFDP
downlink begins with exactly this kind of transaction carrying the proxy put
request, and each one reported a failed delivery on the console.
Nothing was expected of it and nothing is missing, so the delivery code is
DATA_COMPLETE; there is no file whose status could be reported, so the status
is FILE_STATUS_UNREPORTED.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4eBFnanCACYWdKCzhHMcC
printHex and printDec built a variable length array of (size + 1) * 7 + 1 and
size * 4 + 1 + lines bytes respectively, on the stack, from the caller's
buffer size. Nothing bounded that against the stack it ran on.
This reset an iOBC on the flatsat. A CFDP uplink driven without inter packet
spacing filled the USLP receive buffer with about 12 KB in one 300 ms cycle,
a frame parse error asked for the serial stream to be dumped, and printHex
tried to place an 84 KB array on an 8 KB task stack. FreeRTOS caught it as
STACK OVERFLOW DETECTED in USLP_RX and restarted the OBC.
The trigger needs a parse error, so it hid for as long as the link stayed
clean: USLP_RX sat at 800 bytes of its 8 KB, and the first corrupted frame
with a full receive buffer behind it was fatal.
Emit the output in fixed 128 byte chunks instead, flushing as it is built.
The rendered text is unchanged - verified byte for byte against the previous
implementation for sizes 0 to 4096 and several line widths, including the
line break boundaries.
printBin was already safe.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4eBFnanCACYWdKCzhHMcC
handleEofPdu built its EofInfo with a null fault location TLV pointer.
EofPduReader refuses to parse any EOF whose condition code is not NO_ERROR
unless it has somewhere to put that TLV, so every Cancel EOF a sender emits
was rejected with "Ca not deserialize fault location" and dropped before the
handler saw it.
The consequences were invisible from the ground until now: the sender's
cancellation was never acknowledged, so it retransmitted the EOF to its
positive ACK limit and declared a fault, while this handler kept the
transaction open until its own check limit expired. A flatsat uplink hit
exactly that, twice ten seconds apart, which is the sender's retransmission
interval.
Give both EOF parse sites a real EntityIdTlv, held by the handler so no
allocation happens per PDU, and adopt a non-NO_ERROR condition code into the
transaction. Without the second part the cancellation would parse but then
fall through to transfer completion, which would run a checksum pass over a
file the sender has already abandoned and report a checksum failure instead of
the cancellation.
The new test fails without the fix in the same way the flatsat did: no ACK is
emitted at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4eBFnanCACYWdKCzhHMcC
FileDataReader takes the two CRC bytes out of the parsed range before it
reads its payload. The three readers with a variable length tail did not,
and each of them walks that tail until the range is exhausted:
MetadataPduReader over its option TLVs, NakPduReader over its segment
requests, FinishPduReader over its filestore and fault location TLVs.
With crcOnTransmission set at the sender, the CRC is therefore parsed as
one more TLV or segment request and the PDU is rejected, metadata and
Finished with INVALID_TLV_TYPE. Reception of CRC bearing PDUs only ever
worked for the PDUs which have no tail at all, which is why it went
unnoticed: a plain file uplink's metadata carries no options. A proxy put
request always carries one, and in acknowledged mode so do the NAK and
Finished PDUs a ground source sends, so this broke the OBSW as a
destination for any request carrying a message to user, and as a source
for every acknowledged downlink.
MetadataPduReader had a partial guard for this - an early return when the
CRC was the only thing left - which covered the no-options case and hid
the defect for the case that has them. It is replaced by the same up front
subtraction the other two now do.
The tests build CRC bearing PDUs by hand through the new PduCrcHelper,
because the creators cannot produce one: they append the CRC without
counting it in the directive data field length, which is a separate defect
left alone here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4eBFnanCACYWdKCzhHMcC
Review of the class 2 implementation turned up seven defects, all in the paths
that only run when something has already gone wrong on the link.
Send failures were still being treated as successful sends in two places the
earlier fix missed. SourceHandler::servicePendingRetransmissions() advanced its
cursor past a segment whose sendFileDataPdu() failed and cleared
metadataPending before the metadata PDU had gone out, so a NAK answered while
the TM store was full dropped exactly the data the peer asked for. Both now
only advance on success, matching the forward-only path.
A NAK carrying more segment requests than the reader's array can hold was a
complete no-op rather than a partial one: NakPduReader::parseData() returns
NAK_CANT_PARSE_OPTIONS without ever calling setSegmentRequestLen(), so the
length stayed at the 0 set before the loop and handleNakPdu() acted on nothing.
Every early return in that loop now reports the number of complete requests
parsed. handleNakPdu() also resets its retransmit state before parsing, because
the reader writes straight into the segment array and a hard parse error used
to leave the previous NAK's indices pointing into a half overwritten one.
handleFinishedPdu() marked the transaction finished and copied the delivery
result before checking whether the PDU had parsed at all, so a Finished PDU
truncated before its condition code byte completed the transfer and reported
the default constructed DATA_COMPLETE - a fabricated success. Only a failure
inside the optional TLVs is tolerated now.
Neither handler checked which transaction an incoming ACK or Finished PDU
belonged to, and CfdpHandler routes on direction and directive alone. A late
ACK from a previous transaction therefore drove whichever one was running now,
up to and including finishing it. Both handlers now compare the PDU's source
entity ID and sequence number against the running transaction, by value rather
than with operator==, which also compares the encoded width.
The Finished PDU send is now retried on failure instead of being assumed sent,
in both transmission modes, bounded by maxFinishedPduSendAttempts. Class 1 used
to finish() regardless, so the peer never heard that a transfer which actually
succeeded had completed and had no way to ask again. Bounding it is the point:
a busy destination handler discards incoming metadata PDUs, so retrying
forever would mean no later uplink could start. Exhausting the budget releases
the transaction without declaring a fault - the file is complete on disk and
the local user already got its indication, only the notification is lost.
Finally, DestHandler's NAK segment scratch buffer is sized to hold at least one
request, so a maxSegmentRequestsPerNakPdu of 0 is a clamped configuration
rather than an out of bounds write.
Eleven new test sections cover all of it. PduSenderMock gains failSendAtIdx and
failSendsFromIdx alongside failNextSend, for the cases where the call under
test emits several PDUs or where the downstream stays broken.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZpKfWUzvTkBMXEv9NEoTM
SourceHandler::sendGenericPdu() and DestHandler::sendFinishedPdu() discarded
sendPdu()'s return value, so a downstream send failure (e.g. a full TM store)
was invisible to the state machine: transactionParams.progress advanced past
file data that was never actually enqueued for downlink, and a retransmit hit
the same swallowed-error path. Both now propagate the result so a failed send
retries the same segment instead of being silently treated as sent.
Adds a failure-injection knob to PduSenderMock and a SourceHandler test
covering the retry.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZpKfWUzvTkBMXEv9NEoTM
Class 1 has no retransmission at all: on a lossy uplink a single lost PDU
corrupts a transfer and a lost metadata PDU strands it completely. The PDU
layer for class 2 was already complete and unit tested, but neither handler
implemented the procedures on top of it - the destination handler had a
warning stub for BUSY_CLASS_2_ACKED and a SENDING_ACK_PDU step no branch
serviced, and the source handler discarded every incoming PDU and parked
permanently in BUSY_CLASS_2_ACKED.
RemoteEntityCfg gains the positive ACK, NAK and check timer parameters. The
names mirror cfdppy.mib.RemoteEntityConfig field for field so both ends of a
link can be configured from the same numbers. The defaults are inert for
class 1.
Destination handler:
- tracks received segments in the lostSegmentsContainer that was already
plumbed through but never read, so completion is "no gaps and EOF seen"
rather than the class 1 "progress reached the file size"
- emits the ACK for the EOF PDU before the transfer completion step, because
the checksum pass reads the whole file back from the SD card and would
otherwise run inside the sender's positive ACK timer
- runs the deferred lost segment procedure: one NAK sequence when the EOF
arrives, re-issued on NAK timer expiry, NAK_LIMIT_REACHED on the limit.
Segment requests are batched per PDU and the remainder carried over
- retains the transaction until its Finished PDU is acknowledged, retransmits
it on positive ACK timeout, POSITIVE_ACK_LIMIT_REACHED on the limit
- can start a transaction from a file data PDU when the metadata was lost and
request the metadata with a NAK of scope 0 to 0
- acknowledges an EOF PDU for an inactive transaction, otherwise the sender
declares a fault at the end of an otherwise successful transfer
- runs the check timer after EOF so an incomplete file is cancelled instead
of pinning the handler forever
Source handler:
- consumes incoming PDUs instead of dropping them on the floor
- waits for the ACK of its EOF PDU and retransmits on timeout
- answers NAK PDUs by retransmitting the requested segments, one PDU per
state machine call, and the metadata PDU for a scope 0 to 0 request. The
read and send path is split from the forward-only progress cursor for this
- implements WAIT_FOR_FINISH properly: parses the Finished PDU, acknowledges
it and reports the received condition and delivery codes instead of a
hardcoded NO_ERROR / DATA_COMPLETE. This also fixes class 1 with closure,
which reported success for a transfer the receiver had rejected. The wait
is bounded so a lost Finished PDU cannot pin the handler
AckPduCreator and NakPduCreator get the `using FileDirectiveCreator::serialize`
that FinishedPduCreator already had, so the convenience overloads are usable.
crcOnTransmission stays unusable and unused: the CRC sizing bug in the PDU
creators is a separate, self-contained fix.
Previously the printer used a 2d array mechanism to queue up messages,
by selecting a free slot with help of a etl::bitset for tracking. A
message is then sent to the callback what slot is filled up and the
callback then drains the entire queue and prints out the messages.
This was prone to deadline issues, since the entire queue was always
flushed and often I noticed a deadline missed messages when developing
other stuff. Also memory is inefficiently used with the 2d array.
This new version fixes the above using a ring buffer datastructure. We
use a flat array and 2 integer pointers for storing and tracking the
bytes to print. So now there isn't any wasted space between messages.
Also with the design of the ring buffer messages can be written in and
read out at the same time, so we have minimal mutex use (just for
updating the integer pointers). To address the deadline issue, the
callback also only prints out a limited number of bytes per cycle.
(Also the printer code in general has been optimized a bit, since it was
quite needlessly big)
This allows creating and serializing direct
command PUS packets. This functionality is needed
in SOURCE, where OBC prepares TC[8, 128] packets
for Payload Computer (PLOC).
Additionally, expose some setters and
datastructures to facilitate this use case.
tbaumgartl
muellerr
blochm
ritzmannc
bertschs
phoffmann