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