From 9fe8579377d345d2ce0e1fb6300bcfac592d1ee7 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Thu, 25 May 2023 15:11:56 +0200 Subject: [PATCH] CFDP bugfix --- CHANGELOG.md | 5 ++++- src/fsfw/cfdp/pdu/HeaderCreator.cpp | 4 ++-- src/fsfw/cfdp/pdu/HeaderReader.cpp | 4 ++-- unittests/cfdp/pdu/testCfdpHeader.cpp | 10 +++++----- unittests/cfdp/pdu/testFileData.cpp | 2 +- unittests/cfdp/pdu/testFileDirective.cpp | 4 ++-- unittests/cfdp/testCfdp.cpp | 4 ++-- 7 files changed, 18 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 693e59e7..1c7c290a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## Fixes +- Important bugfix in CFDP PDU header format: The entity length field and the transaction sequence + number fields stored the actual length of the field instead of the length minus 1 like specified + in the CFDP standard. - PUS Health Service: Size check for set health command. - Perform operation completion for announce health command. + Perform operation completion for announce health command. https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/746 - Linux OSAL `getUptime` fix: Check validity of `/proc/uptime` file before reading uptime. https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/745 diff --git a/src/fsfw/cfdp/pdu/HeaderCreator.cpp b/src/fsfw/cfdp/pdu/HeaderCreator.cpp index 29688575..2db3953c 100644 --- a/src/fsfw/cfdp/pdu/HeaderCreator.cpp +++ b/src/fsfw/cfdp/pdu/HeaderCreator.cpp @@ -24,8 +24,8 @@ ReturnValue_t HeaderCreator::serialize(uint8_t **buffer, size_t *size, size_t ma *buffer += 1; **buffer = pduDataFieldLen & 0x00ff; *buffer += 1; - **buffer = segmentationCtrl << 7 | pduConf.sourceId.getWidth() << 4 | segmentMetadataFlag << 3 | - pduConf.seqNum.getWidth(); + **buffer = segmentationCtrl << 7 | ((pduConf.sourceId.getWidth() - 1) << 4) | + segmentMetadataFlag << 3 | (pduConf.seqNum.getWidth() - 1); *buffer += 1; *size += 4; ReturnValue_t result = pduConf.sourceId.serialize(buffer, size, maxSize, streamEndianness); diff --git a/src/fsfw/cfdp/pdu/HeaderReader.cpp b/src/fsfw/cfdp/pdu/HeaderReader.cpp index 9edf2394..de3d2906 100644 --- a/src/fsfw/cfdp/pdu/HeaderReader.cpp +++ b/src/fsfw/cfdp/pdu/HeaderReader.cpp @@ -78,11 +78,11 @@ cfdp::SegmentationControl PduHeaderReader::getSegmentationControl() const { } cfdp::WidthInBytes PduHeaderReader::getLenEntityIds() const { - return static_cast((pointers.fixedHeader->fourthByte >> 4) & 0x07); + return static_cast(((pointers.fixedHeader->fourthByte >> 4) & 0b111) + 1); } cfdp::WidthInBytes PduHeaderReader::getLenSeqNum() const { - return static_cast(pointers.fixedHeader->fourthByte & 0x07); + return static_cast((pointers.fixedHeader->fourthByte & 0b111) + 1); } cfdp::SegmentMetadataFlag PduHeaderReader::getSegmentMetadataFlag() const { diff --git a/unittests/cfdp/pdu/testCfdpHeader.cpp b/unittests/cfdp/pdu/testCfdpHeader.cpp index 5f81bec9..1fc7dfd4 100644 --- a/unittests/cfdp/pdu/testCfdpHeader.cpp +++ b/unittests/cfdp/pdu/testCfdpHeader.cpp @@ -97,7 +97,7 @@ TEST_CASE("CFDP Header", "[cfdp]") { REQUIRE(creator.serialize(&serTarget, &serSize, serBuf.size(), SerializeIF::Endianness::BIG) == returnvalue::OK); CHECK(serBuf[0] == 0x3f); - CHECK(serBuf[3] == 0x99); + CHECK(serBuf[3] == 0x88); REQUIRE(creator.getCrcFlag() == true); REQUIRE(creator.getDirection() == cfdp::Direction::TOWARDS_SENDER); REQUIRE(creator.getLargeFileFlag() == true); @@ -127,7 +127,7 @@ TEST_CASE("CFDP Header", "[cfdp]") { REQUIRE(creator.getTransmissionMode() == cfdp::TransmissionMode::UNACKNOWLEDGED); REQUIRE(creator.getSegmentationControl() == true); // Last three bits are 2 now (length of seq number) and bit 1 to bit 3 is 4 (len entity IDs) - REQUIRE(serBuf[3] == 0b11001010); + REQUIRE(serBuf[3] == 0b10111001); uint32_t entityId = 0; size_t deSerSize = 0; SerializeAdapter::deSerialize(&entityId, serBuf.data() + 4, &deSerSize, @@ -175,7 +175,7 @@ TEST_CASE("CFDP Header", "[cfdp]") { REQUIRE(serBuf[1] == 0); REQUIRE(serBuf[2] == 0); // Entity and Transaction Sequence number are 1 byte large - REQUIRE(serBuf[3] == 0b00010001); + REQUIRE(serBuf[3] == 0b00000000); // Source ID REQUIRE(serBuf[4] == 0); // Transaction Seq Number @@ -220,7 +220,7 @@ TEST_CASE("CFDP Header", "[cfdp]") { REQUIRE(serBuf[1] == 0); REQUIRE(serBuf[2] == 0); // Entity and Transaction Sequence number are 1 byte large - REQUIRE(serBuf[3] == 0b00010001); + REQUIRE(serBuf[3] == 0b00000000); REQUIRE(serSize == 7); // Deser call not strictly necessary auto reader = PduHeaderReader(serBuf.data(), serBuf.size()); @@ -270,7 +270,7 @@ TEST_CASE("CFDP Header", "[cfdp]") { REQUIRE(reader.parseData() == returnvalue::OK); // Everything except version bit flipped to one now REQUIRE(serBuf[0] == 0x3f); - REQUIRE(serBuf[3] == 0b11001010); + REQUIRE(serBuf[3] == 0b10111001); REQUIRE(reader.getWholePduSize() == 14); REQUIRE(reader.getCrcFlag() == true); diff --git a/unittests/cfdp/pdu/testFileData.cpp b/unittests/cfdp/pdu/testFileData.cpp index 258ef9c1..39139378 100644 --- a/unittests/cfdp/pdu/testFileData.cpp +++ b/unittests/cfdp/pdu/testFileData.cpp @@ -68,7 +68,7 @@ TEST_CASE("File Data PDU", "[cfdp][pdu]") { // Bits 1 to 3 length of enitity IDs is 2 // Bit 4: Segment metadata flag is set // Bit 5 to seven: length of transaction seq num is 2 - REQUIRE(fileDataBuffer[3] == 0b10101010); + REQUIRE(fileDataBuffer[3] == 0b10011001); REQUIRE((fileDataBuffer[10] >> 6) & 0b11 == cfdp::RecordContinuationState::CONTAINS_START_AND_END); // Segment metadata length diff --git a/unittests/cfdp/pdu/testFileDirective.cpp b/unittests/cfdp/pdu/testFileDirective.cpp index e1158a1a..17e0d699 100644 --- a/unittests/cfdp/pdu/testFileDirective.cpp +++ b/unittests/cfdp/pdu/testFileDirective.cpp @@ -30,7 +30,7 @@ TEST_CASE("CFDP File Directive", "[cfdp][pdu]") { REQUIRE(serBuf[1] == 0); REQUIRE(serBuf[2] == 5); // Entity and Transaction Sequence number are 1 byte large - REQUIRE(serBuf[3] == 0b00010001); + REQUIRE(serBuf[3] == 0b00000000); // Source ID REQUIRE(serBuf[4] == 0); // Transaction Seq Number @@ -82,4 +82,4 @@ TEST_CASE("CFDP File Directive", "[cfdp][pdu]") { // Invalid file directive REQUIRE(fdDeser.parseData() == cfdp::INVALID_DIRECTIVE_FIELD); } -} \ No newline at end of file +} diff --git a/unittests/cfdp/testCfdp.cpp b/unittests/cfdp/testCfdp.cpp index eacc83de..467b5b2d 100644 --- a/unittests/cfdp/testCfdp.cpp +++ b/unittests/cfdp/testCfdp.cpp @@ -33,8 +33,8 @@ TEST_CASE("CFDP Base", "[cfdp]") { // PDU data field length is 5 (4 + Directive code octet) REQUIRE(serBuf[1] == 0); REQUIRE(serBuf[2] == 5); - // Entity and Transaction Sequence number are 1 byte large - REQUIRE(serBuf[3] == 0b00010001); + // Entity and Transaction Sequence number are 1 byte large, value minus one is stored + REQUIRE(serBuf[3] == 0b00000000); // Source ID REQUIRE(serBuf[4] == 0); // Transaction Seq Number