diff --git a/src/fsfw/cfdp/handler/DestHandler.cpp b/src/fsfw/cfdp/handler/DestHandler.cpp index 94b17485..2557da6f 100644 --- a/src/fsfw/cfdp/handler/DestHandler.cpp +++ b/src/fsfw/cfdp/handler/DestHandler.cpp @@ -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) { diff --git a/src/fsfw/cfdp/handler/DestHandler.h b/src/fsfw/cfdp/handler/DestHandler.h index bbffdc3e..57800b25 100644 --- a/src/fsfw/cfdp/handler/DestHandler.h +++ b/src/fsfw/cfdp/handler/DestHandler.h @@ -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); diff --git a/unittests/cfdp/handler/testDestHandler.cpp b/unittests/cfdp/handler/testDestHandler.cpp index a1b69ac8..bf3f0798 100644 --- a/unittests/cfdp/handler/testDestHandler.cpp +++ b/unittests/cfdp/handler/testDestHandler.cpp @@ -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(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);