cfdp: parse and act on a Cancel EOF at the destination
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
This commit is contained in:
1 parent
9d609b6d2d
commit
e256fa4b92
3 files changed
+51
-2
No files matched your search
@@ -276,7 +276,7 @@ ReturnValue_t cfdp::DestHandler::handleEofPdu(const cfdp::PduPacketIF& info) {
|
||||
size_t pduSize = 0;
|
||||
const auto rawPdu = info.getRawPduData(pduSize);
|
||||
// Process EOF PDU
|
||||
EofInfo eofInfo(nullptr);
|
||||
EofInfo eofInfo(&eofFaultLocation);
|
||||
EofPduReader reader(rawPdu, pduSize, eofInfo);
|
||||
ReturnValue_t result = reader.parseData();
|
||||
if (result != OK) {
|
||||
@@ -306,6 +306,12 @@ ReturnValue_t cfdp::DestHandler::handleEofPdu(const cfdp::PduPacketIF& info) {
|
||||
}
|
||||
transactionParams.eofReceived = true;
|
||||
transactionParams.eofConditionCode = eofInfo.getConditionCode();
|
||||
if (eofInfo.getConditionCode() != ConditionCode::NO_ERROR) {
|
||||
// A Cancel EOF ends the transaction. Adopt its condition code so that transfer completion
|
||||
// reports the cancellation, instead of running a checksum pass over a file the sender has
|
||||
// already abandoned and then reporting a checksum failure for it.
|
||||
transactionParams.conditionCode = eofInfo.getConditionCode();
|
||||
}
|
||||
if (eofInfo.getConditionCode() == ConditionCode::NO_ERROR and
|
||||
transactionParams.fileSize.value() > transactionParams.progress) {
|
||||
// Everything between the highest offset seen so far and the file size announced by the EOF
|
||||
@@ -873,7 +879,7 @@ ReturnValue_t cfdp::DestHandler::ackInactiveEofPdu(const PduPacketIF& pduPacket)
|
||||
if (headerReader.getTransmissionMode() != TransmissionMode::ACKNOWLEDGED) {
|
||||
return DEST_NON_METADATA_PDU_AS_FIRST_PDU;
|
||||
}
|
||||
EofInfo eofInfo(nullptr);
|
||||
EofInfo eofInfo(&eofFaultLocation);
|
||||
EofPduReader eofReader(rawPdu, pduSize, eofInfo);
|
||||
result = eofReader.parseData();
|
||||
if (result != OK) {
|
||||
|
||||
@@ -18,6 +18,7 @@
|
||||
#include "fsfw/cfdp/pdu/MetadataPduReader.h"
|
||||
#include "fsfw/cfdp/pdu/NakInfo.h"
|
||||
#include "fsfw/cfdp/pdu/PduConfig.h"
|
||||
#include "fsfw/cfdp/tlv/EntityIdTlv.h"
|
||||
#include "fsfw/cfdp/tlv/MessageToUserTlv.h"
|
||||
#include "fsfw/storagemanager/StorageManagerIF.h"
|
||||
#include "fsfw/timemanager/Countdown.h"
|
||||
@@ -209,6 +210,11 @@ class DestHandler {
|
||||
Countdown nakTimer;
|
||||
//! Guards an incomplete transaction after EOF reception, see RemoteEntityCfg::checkLimit.
|
||||
Countdown checkTimer;
|
||||
//! Receives the fault location of an incoming EOF PDU. EofPduReader refuses to parse any EOF
|
||||
//! whose condition code is not NO_ERROR unless it has somewhere to put that TLV, so without
|
||||
//! these every Cancel EOF the sender emits would be dropped as unparseable.
|
||||
cfdp::EntityId eofFaultLocationId;
|
||||
EntityIdTlv eofFaultLocation{eofFaultLocationId};
|
||||
|
||||
ReturnValue_t startTransaction(const MetadataPduReader& reader);
|
||||
ReturnValue_t handleMetadataPdu(const PduPacketIF& pduPacket);
|
||||
|
||||
@@ -409,6 +409,43 @@ TEST_CASE("CFDP Dest Handler Acknowledged", "[cfdp]") {
|
||||
CHECK(destHandler.getTransactionStep() == DestHandler::TransactionStep::IDLE);
|
||||
}
|
||||
|
||||
SECTION("Cancel EOF is parsed, acknowledged and ends the transaction") {
|
||||
// A Cancel EOF carries a condition code other than NO_ERROR and a fault location TLV. The
|
||||
// reader refuses to parse one unless it is given somewhere to put that TLV, so this used to
|
||||
// fail with "Ca not deserialize fault location" and the whole PDU was dropped: the sender
|
||||
// never got its ACK and retransmitted to its positive ACK limit, while this handler held the
|
||||
// transaction open until its own check limit expired.
|
||||
auto metadataPdu = makeMetadataPdu();
|
||||
const DestHandler::FsmResult& res = destHandler.stateMachine(metadataPdu);
|
||||
REQUIRE(res.state == CfdpState::BUSY_CLASS_2_ACKED);
|
||||
// Only part of the file arrives before the sender gives up on it.
|
||||
auto fdPdu = makeFileDataPdu(0, 10);
|
||||
destHandler.stateMachine(fdPdu);
|
||||
|
||||
EntityId faultLocId(UnsignedByteField<uint16_t>(2));
|
||||
EntityIdTlv faultLoc(faultLocId);
|
||||
EofInfo cancelInfo(ConditionCode::CANCEL_REQUEST_RECEIVED, 0, Fss(fileData.size()), &faultLoc);
|
||||
EofPduCreator cancelCreator(conf, cancelInfo);
|
||||
REQUIRE(cancelCreator.serialize(pduBuf.data(), serLen, cancelCreator.getSerializedSize()) ==
|
||||
OK);
|
||||
auto cancelEof = OwnedPduPacket(cancelCreator.getPduType(), cancelCreator.getDirectiveCode(),
|
||||
pduBuf.data(), serLen);
|
||||
destHandler.stateMachine(cancelEof);
|
||||
|
||||
// The cancellation is acknowledged rather than ignored, and it is reported with the condition
|
||||
// code the sender gave instead of a checksum failure over the partial file.
|
||||
auto ackPdu = expectDirective(FileDirective::ACK);
|
||||
AckInfo ackInfo;
|
||||
AckPduReader ackReader(ackPdu.rawPdu.data(), ackPdu.rawPdu.size(), ackInfo);
|
||||
REQUIRE(ackReader.parseData() == OK);
|
||||
CHECK(ackInfo.getAckedDirective() == FileDirective::EOF_DIRECTIVE);
|
||||
CHECK(ackInfo.getAckedConditionCode() == ConditionCode::CANCEL_REQUEST_RECEIVED);
|
||||
expectDirective(FileDirective::FINISH);
|
||||
REQUIRE(userMock.finishedRecvd.size() == 1);
|
||||
CHECK(userMock.finishedRecvd.back().second.condCode == ConditionCode::CANCEL_REQUEST_RECEIVED);
|
||||
CHECK(userMock.finishedRecvd.back().second.deliveryCode == FileDeliveryCode::DATA_INCOMPLETE);
|
||||
}
|
||||
|
||||
SECTION("Dropped file data PDU is recovered via NAK") {
|
||||
auto metadataPdu = makeMetadataPdu();
|
||||
const DestHandler::FsmResult& res = destHandler.stateMachine(metadataPdu);
|
||||
|
||||
Reference in new issue
Block a user