Merge pull request 'CFDP Bugfix' (#144) from cfdp-bugfix-unittest-fixes into develop

Reviewed-on: #144
Reviewed-by: Marius Eggert <eggertm@irs.uni-stuttgart.de>

CHANGELOG.md

@@ -8,6 +8,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 # [unreleased]
 
+## Fixed
+
+- 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.
+
 ## Changed
 
 - Health functions are virtual now.

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 |
+  **buffer = segmentationCtrl << 7 | ((pduConf.sourceId.getWidth() - 1) << 4) |
-             pduConf.seqNum.getWidth();
+             segmentMetadataFlag << 3 | (pduConf.seqNum.getWidth() - 1);
   *buffer += 1;
   *size += 4;
   ReturnValue_t result = pduConf.sourceId.serialize(buffer, size, maxSize, streamEndianness);

src/fsfw/cfdp/pdu/HeaderReader.cpp

@@ -78,11 +78,11 @@ cfdp::SegmentationControl PduHeaderReader::getSegmentationControl() const {
 }
 
 cfdp::WidthInBytes PduHeaderReader::getLenEntityIds() const {
-  return static_cast<cfdp::WidthInBytes>((pointers.fixedHeader->fourthByte >> 4) & 0x07);
+  return static_cast<cfdp::WidthInBytes>(((pointers.fixedHeader->fourthByte >> 4) & 0b111) + 1);
 }
 
 cfdp::WidthInBytes PduHeaderReader::getLenSeqNum() const {
-  return static_cast<cfdp::WidthInBytes>(pointers.fixedHeader->fourthByte & 0x07);
+  return static_cast<cfdp::WidthInBytes>((pointers.fixedHeader->fourthByte & 0b111) + 1);
 }
 
 cfdp::SegmentMetadataFlag PduHeaderReader::getSegmentMetadataFlag() const {

unittests/CatchFactory.cpp

@@ -30,7 +30,7 @@
  */
 void Factory::produceFrameworkObjects(void* args) {
   setStaticFrameworkObjectIds();
-  new EventManager(objects::EVENT_MANAGER);
+  new EventManager(objects::EVENT_MANAGER, 120);
   new HealthTable(objects::HEALTH_TABLE);
   new InternalErrorReporter(objects::INTERNAL_ERROR_REPORTER);
 

unittests/action/TestActionHelper.cpp

@@ -27,12 +27,7 @@ TEST_CASE("Action Helper", "[action]") {
     CHECK(not testDhMock.executeActionCalled);
     REQUIRE(actionHelper.handleActionMessage(&actionMessage) == returnvalue::OK);
     CHECK(testDhMock.executeActionCalled);
-    // No message is sent if everything is alright.
     CHECK(not testMqMock.wasMessageSent());
-    store_address_t invalidAddress;
-    ActionMessage::setCommand(&actionMessage, testActionId, invalidAddress);
-    actionHelper.handleActionMessage(&actionMessage);
-    CHECK(testMqMock.wasMessageSent());
     const uint8_t* ptr = nullptr;
     size_t size = 0;
     REQUIRE(ipcStore->getData(paramAddress, &ptr, &size) ==
@@ -44,6 +39,10 @@ TEST_CASE("Action Helper", "[action]") {
     for (uint8_t i = 0; i < 3; i++) {
       REQUIRE(ptr[i] == (i + 1));
     }
+    // Action message without application data is also valid
+    store_address_t invalidAddress;
+    ActionMessage::setCommand(&actionMessage, testActionId, invalidAddress);
+    actionHelper.handleActionMessage(&actionMessage);
     testDhMock.clearBuffer();
   }
 
@@ -95,17 +94,5 @@ TEST_CASE("Action Helper", "[action]") {
     REQUIRE(ActionMessage::getActionId(&testMessage) == testActionId);
   }
 
-  SECTION("Missing IPC Data") {
-    ActionMessage::setCommand(&actionMessage, testActionId, store_address_t::invalid());
-    CHECK(not testDhMock.executeActionCalled);
-    REQUIRE(actionHelper.handleActionMessage(&actionMessage) == returnvalue::OK);
-    CommandMessage testMessage;
-    REQUIRE(testMqMock.getNextSentMessage(testMessage) == returnvalue::OK);
-    REQUIRE(testMessage.getCommand() == static_cast<uint32_t>(ActionMessage::STEP_FAILED));
-    REQUIRE(ActionMessage::getReturnCode(&testMessage) ==
-            static_cast<uint32_t>(StorageManagerIF::ILLEGAL_STORAGE_ID));
-    REQUIRE(ActionMessage::getStep(&testMessage) == 0);
-  }
-
   SECTION("Data Reply") {}
 }

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);

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

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);
   }
-}
+}

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
+    // Entity and Transaction Sequence number are 1 byte large, value minus one is stored
-    REQUIRE(serBuf[3] == 0b00010001);
+    REQUIRE(serBuf[3] == 0b00000000);
     // Source ID
     REQUIRE(serBuf[4] == 0);
     // Transaction Seq Number

unittests/datapoollocal/testLocalPoolManager.cpp

@@ -9,6 +9,7 @@
 
 #include <catch2/catch_approx.hpp>
 #include <catch2/catch_test_macros.hpp>
+#include <catch2/matchers/catch_matchers_floating_point.hpp>
 #include <iostream>
 
 #include "CatchDefinitions.h"
@@ -309,9 +310,7 @@ TEST_CASE("Local Pool Manager Tests", "[LocManTest]") {
     HousekeepingMessage::setCollectionIntervalModificationCommand(&hkCmd, lpool::testSid, 0.4,
                                                                   false);
     CHECK(poolOwner.poolManager.handleHousekeepingMessage(&hkCmd) == returnvalue::OK);
-    /* For non-diagnostics and a specified minimum frequency of 0.2 seconds, the
+    CHECK_THAT(poolOwner.dataset.getCollectionInterval(), Catch::Matchers::WithinAbs(0.4, 0.01));
-    resulting collection interval should be 1.0 second */
-    CHECK(poolOwner.dataset.getCollectionInterval() == 1.0);
     REQUIRE(poolOwnerMock.wasMessageSent());
     REQUIRE(poolOwnerMock.numberOfSentMessages() == 1);
     CHECK(poolOwnerMock.clearLastSentMessage() == returnvalue::OK);
@@ -348,14 +347,6 @@ TEST_CASE("Local Pool Manager Tests", "[LocManTest]") {
     REQUIRE(poolOwnerMock.numberOfSentMessages() == 1);
     CHECK(poolOwnerMock.clearLastSentMessage() == returnvalue::OK);
 
-    HousekeepingMessage::setCollectionIntervalModificationCommand(&hkCmd, lpool::testSid, 0.4,
-                                                                  false);
-    CHECK(poolOwner.poolManager.handleHousekeepingMessage(&hkCmd) ==
-          static_cast<int>(LocalDataPoolManager::WRONG_HK_PACKET_TYPE));
-    REQUIRE(poolOwnerMock.wasMessageSent());
-    REQUIRE(poolOwnerMock.numberOfSentMessages() == 1);
-    CHECK(poolOwnerMock.clearLastSentMessage() == returnvalue::OK);
-
     HousekeepingMessage::setStructureReportingCommand(&hkCmd, lpool::testSid, false);
     CHECK(poolOwner.poolManager.handleHousekeepingMessage(&hkCmd) ==
           static_cast<int>(LocalDataPoolManager::WRONG_HK_PACKET_TYPE));