From daf75547a45be74eb27b1456d76ac00704af98e9 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 3 Aug 2023 15:13:26 +0200 Subject: [PATCH] insidious bug --- src/fsfw/cfdp/VarLenFields.cpp | 2 ++ src/fsfw/cfdp/VarLenFields.h | 1 + src/fsfw/cfdp/handler/SourceHandler.cpp | 18 ++++++++++++------ src/fsfw/cfdp/handler/SourceHandler.h | 2 +- unittests/cfdp/handler/testSourceHandler.cpp | 3 ++- unittests/cfdp/pdu/testMetadataPdu.cpp | 4 +++- 6 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/fsfw/cfdp/VarLenFields.cpp b/src/fsfw/cfdp/VarLenFields.cpp index ee3ee76f..d0582467 100644 --- a/src/fsfw/cfdp/VarLenFields.cpp +++ b/src/fsfw/cfdp/VarLenFields.cpp @@ -153,3 +153,5 @@ bool cfdp::VarLenField::operator==(const cfdp::VarLenField &other) const { bool cfdp::VarLenField::operator!=(const cfdp::VarLenField &other) const { return not(*this == other); } + +void cfdp::VarLenField::setWidth(cfdp::WidthInBytes width_) { this->width = width_; } diff --git a/src/fsfw/cfdp/VarLenFields.h b/src/fsfw/cfdp/VarLenFields.h index f6dbc2b0..3143a857 100644 --- a/src/fsfw/cfdp/VarLenFields.h +++ b/src/fsfw/cfdp/VarLenFields.h @@ -32,6 +32,7 @@ class VarLenField : public SerializeIF { bool operator<(const VarLenField &other) const; ReturnValue_t setValueAndWidth(cfdp::WidthInBytes width, uint64_t value); + void setWidth(cfdp::WidthInBytes width); ReturnValue_t setValue(uint64_t value); ReturnValue_t serialize(uint8_t **buffer, size_t *size, size_t maxSize, diff --git a/src/fsfw/cfdp/handler/SourceHandler.cpp b/src/fsfw/cfdp/handler/SourceHandler.cpp index 4365b824..0b92fb74 100644 --- a/src/fsfw/cfdp/handler/SourceHandler.cpp +++ b/src/fsfw/cfdp/handler/SourceHandler.cpp @@ -19,12 +19,15 @@ cfdp::SourceHandler::SourceHandler(SourceHandlerParams params, FsfwParams fsfwPa : sourceParams(std::move(params)), fsfwParams(fsfwParams) { // The entity ID portion of the transaction ID will always remain fixed. transactionParams.id.entityId = sourceParams.cfg.localId; + transactionParams.pduConf.sourceId = sourceParams.cfg.localId; if (sourceParams.seqCountProvider.bitWidth() == 8) { - transactionParams.seqCountWidth = cfdp::WidthInBytes::ONE_BYTE; + transactionParams.pduConf.seqNum.setWidth(cfdp::WidthInBytes::ONE_BYTE); } else if (sourceParams.seqCountProvider.bitWidth() == 16) { - transactionParams.seqCountWidth = cfdp::WidthInBytes::TWO_BYTES; + transactionParams.pduConf.seqNum.setWidth(cfdp::WidthInBytes::TWO_BYTES); } else if (sourceParams.seqCountProvider.bitWidth() == 32) { - transactionParams.seqCountWidth = cfdp::WidthInBytes::FOUR_BYTES; + transactionParams.pduConf.seqNum.setWidth(cfdp::WidthInBytes::FOUR_BYTES); + } else if (sourceParams.seqCountProvider.bitWidth() == 64) { + transactionParams.pduConf.seqNum.setWidth(cfdp::WidthInBytes::EIGHT_BYTES); } else { #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "cfdp::SourceHandler: Seq count provider bit width " @@ -34,7 +37,7 @@ cfdp::SourceHandler::SourceHandler(SourceHandlerParams params, FsfwParams fsfwPa sourceParams.seqCountProvider.bitWidth()); #endif // Yeah, what am I supposed to do here? Can't throw an exception in the FSFW.. - transactionParams.seqCountWidth = cfdp::WidthInBytes::ONE_BYTE; + transactionParams.pduConf.seqNum.setWidth(cfdp::WidthInBytes::ONE_BYTE); } } @@ -172,10 +175,13 @@ ReturnValue_t cfdp::SourceHandler::transactionStart(PutRequest& putRequest, Remo if (not putRequest.getClosureRequested(transactionParams.closureRequested)) { transactionParams.closureRequested = cfg.closureRequested; } - transactionParams.pduConf.destId = putRequest.getDestId(); + const EntityId& destId = putRequest.getDestId(); + transactionParams.pduConf.destId = destId; + // Adapt source ID width to necessary width. The width of the source and destination ID must be + // the same. + transactionParams.pduConf.sourceId.setWidth(destId.getWidth()); // Only used for PDU forwarding, file is sent to file receiver regularly here. transactionParams.pduConf.direction = Direction::TOWARDS_RECEIVER; - transactionParams.pduConf.sourceId = sourceParams.cfg.localId; transactionParams.id.seqNum.setValue(sourceParams.seqCountProvider.getAndIncrement()); if (transactionParams.pduConf.mode == TransmissionMode::ACKNOWLEDGED) { diff --git a/src/fsfw/cfdp/handler/SourceHandler.h b/src/fsfw/cfdp/handler/SourceHandler.h index acac6dfd..ea7d95a4 100644 --- a/src/fsfw/cfdp/handler/SourceHandler.h +++ b/src/fsfw/cfdp/handler/SourceHandler.h @@ -76,7 +76,7 @@ class SourceHandler { RemoteEntityCfg remoteCfg; PduConfig pduConf; cfdp::TransactionId id{}; - cfdp::WidthInBytes seqCountWidth{}; + // cfdp::WidthInBytes seqCountWidth{}; void reset() { sourceNameSize = 0; diff --git a/unittests/cfdp/handler/testSourceHandler.cpp b/unittests/cfdp/handler/testSourceHandler.cpp index e0c3a7a7..1ab5cadc 100644 --- a/unittests/cfdp/handler/testSourceHandler.cpp +++ b/unittests/cfdp/handler/testSourceHandler.cpp @@ -61,7 +61,7 @@ TEST_CASE("CFDP Source Handler", "[cfdp]") { FilesystemParams srcFileName("/tmp/cfdp-test.txt"); fsMock.createFile(srcFileName); cfdp::StringLv srcNameLv(srcFileName.path, std::strlen(srcFileName.path)); - FilesystemParams destFileName("/tmp/cfdp-test.txt"); + FilesystemParams destFileName("/tmp/cfdp-test2.txt"); cfdp::StringLv destNameLv(destFileName.path, std::strlen(destFileName.path)); PutRequest putRequest(id, srcNameLv, destNameLv); CHECK(sourceHandler.transactionStart(putRequest, cfg) == OK); @@ -73,6 +73,7 @@ TEST_CASE("CFDP Source Handler", "[cfdp]") { auto accessor = tmStore.getData(tmtcMessage.getStorageId()); REQUIRE(accessor.first == OK); const uint8_t* pduPtr = accessor.second.data(); + CHECK(accessor.second.size() == 55); MetadataGenericInfo metadataInfo; MetadataPduReader metadataReader(pduPtr, accessor.second.size(), metadataInfo, nullptr, 0); size_t srcFileSize = 0; diff --git a/unittests/cfdp/pdu/testMetadataPdu.cpp b/unittests/cfdp/pdu/testMetadataPdu.cpp index fbf3da08..e13b4717 100644 --- a/unittests/cfdp/pdu/testMetadataPdu.cpp +++ b/unittests/cfdp/pdu/testMetadataPdu.cpp @@ -56,10 +56,12 @@ TEST_CASE("Metadata PDU", "[cfdp][pdu]") { REQUIRE(mdBuffer[24] == 'x'); REQUIRE(mdBuffer[25] == 't'); }; + SECTION("Serialize with empty dest name") { MetadataPduCreator serializer(pduConf, info, sourceFileName, destFileName, nullptr, 0); result = serializer.serialize(&buffer, &sz, mdBuffer.size(), SerializeIF::Endianness::NETWORK); REQUIRE(result == returnvalue::OK); + CHECK(sz == serializer.getSerializedSize()); // 10 byte heater + 1 byte PDU directive field + 1 byte PDU content + FSS field (4) + source // name field (10) + dest name field (1). REQUIRE(serializer.getWholePduSize() == 27); @@ -81,6 +83,7 @@ TEST_CASE("Metadata PDU", "[cfdp][pdu]") { // 10 byte heater + 1 byte PDU directive field + 1 byte PDU content + FSS field (4) + source // name field (10) + dest name field (11). REQUIRE(serializer.getWholePduSize() == 37); + CHECK(sz == serializer.getSerializedSize()); REQUIRE((mdBuffer[1] << 8 | mdBuffer[2]) == 27); REQUIRE(serializer.getSerializedSize() == serializer.getWholePduSize()); metadataCheckPartOne(); @@ -161,7 +164,6 @@ TEST_CASE("Metadata PDU", "[cfdp][pdu]") { info.setClosureRequested(true); serializer.updateDirectiveFieldLen(); - // info.setSourceFileName(sourceFileName); result = serializer.serialize(&buffer, &sz, mdBuffer.size(), SerializeIF::Endianness::NETWORK); REQUIRE(result == returnvalue::OK);