Repository: nasa/fprime (language: python)

Issue:
FileUplink packet sequence repeat and CRC
| | |
|:---|:---|
|**_F´ Version_**| 3.0 |
|**_Affected Component_**| Svc/FileUplink |
---
## Feature Description

Add duplicate packet detection based on sequence number and don't add to CRC computation

## Rationale

If the radio link is marginal, a radio can do a retry and send the same packet. In that case, you get the following warning:

```
FileUplink_PacketOutOfOrder: Received packet 342 after packet 342
```

The repeated packet still gets added to the CRC computation, so even with a successful retry, you get this warning at the end:

```
FileUplink_BadChecksum: Bad checksum value during receipt of file XXXXX: computed 0x6A4F6972, read 0x92A51AAA
```

The update would to be to not update the CRC only in the case of a repeated packet sequence number. This wouldn't be to try to track all cases of dropped and retried packets.


Discussion / hints:
@timcanham -- In the case of a repeated packet, should the repeated packet still get written to the file? In that case, I'll simply protect against updating the checksum by wrapping [THIS LINE](https://github.com/nasa/fprime/blob/2c8d0db8411fe955476925fad8e046e8100653ff/Svc/FileUplink/File.cpp#L56) in a condition. 

If we don't want to write the repeated packet to the file, then I'll wrap [THIS BLOCK OF CODE](https://github.com/nasa/fprime/blob/2c8d0db8411fe955476925fad8e046e8100653ff/Svc/FileUplink/FileUplink.cpp#L131-L138) in a condition that uses the same logic as `checkSequenceIndex()`.
@DJKessler I would skip the second write if the first copy was received successfully.

Fix patch:
diff --git a/Svc/FileUplink/Events.fppi b/Svc/FileUplink/Events.fppi
index 3949d90fb5..357b2b381f 100644
--- a/Svc/FileUplink/Events.fppi
+++ b/Svc/FileUplink/Events.fppi
@@ -63,10 +63,19 @@ event PacketOutOfOrder(
   format "Received packet {} after packet {}" \
   throttle 20
 
+@ The File Uplink component encountered a duplicate packet during file receipt
+event PacketDuplicate(
+                        packetIndex: U32 @< The sequence index of the duplicate packet
+                      ) \
+  severity warning high \
+  id 7 \
+  format "Received a duplicate of packet {}" \
+  throttle 20
+
 @ The File Uplink component received a CANCEL packet
 event UplinkCanceled \
   severity activity high \
-  id 7 \
+  id 8 \
   format "Received CANCEL packet"
 
 @ Error decoding file packet
@@ -74,5 +83,5 @@ event DecodeError(
                    status: I32 @< The sequence index of the out-of-order packet
                  ) \
   severity warning high \
-  id 8 \
+  id 9 \
   format "Unable to decode file packet. Status: {}"
diff --git a/Svc/FileUplink/FileUplink.cpp b/Svc/FileUplink/FileUplink.cpp
index 2eea06d78a..6c19af7a2e 100644
--- a/Svc/FileUplink/FileUplink.cpp
+++ b/Svc/FileUplink/FileUplink.cpp
@@ -25,6 +25,7 @@ namespace Svc {
       FileUplinkComponentBase(name),
       m_receiveMode(START),
       m_lastSequenceIndex(0),
+      m_lastPacketWriteStatus(Os::File::MAX_STATUS),
       m_filesReceived(this),
       m_packetsReceived(this),
       m_warnings(this)
@@ -120,7 +121,15 @@ namespace Svc {
       this->m_warnings.invalidReceiveMode(Fw::FilePacket::T_DATA);
       return;
     }
+
     const U32 sequenceIndex = dataPacket.asHeader().getSequenceIndex();
+
+    // skip this packet if it is a duplicate and it has already been written
+    if (this->m_lastPacketWriteStatus == Os::File::OP_OK &&
+        this->checkDuplicatedPacket(sequenceIndex)) {
+        return;
+    }
+
     this->checkSequenceIndex(sequenceIndex);
     const U32 byteOffset = dataPacket.getByteOffset();
     const U32 dataSize = dataPacket.getDataSize();
@@ -136,6 +145,8 @@ namespace Svc {
     if (status != Os::File::OP_OK) {
       this->m_warnings.fileWrite(this->m_file.name);
     }
+
+    this->m_lastPacketWriteStatus = status;
   }
 
   void FileUplink ::
@@ -174,6 +185,18 @@ namespace Svc {
     this->m_lastSequenceIndex = sequenceIndex;
   }
 
+  bool FileUplink ::
+    checkDuplicatedPacket(const U32 sequenceIndex)
+  {
+    // check for duplicate packet
+    if (sequenceIndex == this->m_lastSequenceIndex) {
+      this->m_warnings.packetDuplicate(sequenceIndex);
+      return true;
+    }
+
+    return false;
+  }
+
   void FileUplink ::
     compareChecksums(const Fw::FilePacket::EndPacket& endPacket)
   {
@@ -194,6 +217,7 @@ namespace Svc {
     this->m_file.osFile.close();
     this->m_receiveMode = START;
     this->m_lastSequenceIndex = 0;
+    this->m_lastPacketWriteStatus = Os::File::MAX_STATUS;
   }
 
   void FileUplink ::
@@ -201,6 +225,7 @@ namespace Svc {
   {
     this->m_receiveMode = DATA;
     this->m_lastSequenceIndex = 0;
+    this->m_lastPacketWriteStatus = Os::File::MAX_STATUS;
   }
 
 }
diff --git a/Svc/FileUplink/FileUplink.hpp b/Svc/FileUplink/FileUplink.hpp
index ce3b6793b5..ca626bc2c5 100644
--- a/Svc/FileUplink/FileUplink.hpp
+++ b/Svc/FileUplink/FileUplink.hpp
@@ -161,6 +161,11 @@ namespace Svc {
               const U32 lastSequenceIndex
           );
 
+          //! Record a Duplicate Packet warning
+          void packetDuplicate(
+              const U32 sequenceIndex
+          );
+
           //! Record a File Write warning
           void fileWrite(Fw::LogStringArg& fileName);
 
@@ -246,6 +251,9 @@ namespace Svc {
       //! Check sequence index
       void checkSequenceIndex(const U32 sequenceIndex);
 
+      //! Check if a received packet is a duplicate
+      bool checkDuplicatedPacket(const U32 sequenceIndex);
+
       //! Compare checksums
       void compareChecksums(const Fw::FilePacket::EndPacket& endPacket);
 
@@ -267,6 +275,9 @@ namespace Svc {
       //! The sequence index of the last packet received
       U32 m_lastSequenceIndex;
 
+      //! The write status of the last packet received
+      Os::File::Status m_lastPacketWriteStatus;
+
       //! The file being assembled
       File m_file;
 
diff --git a/Svc/FileUplink/Warnings.cpp b/Svc/FileUplink/Warnings.cpp
index da1ee4831c..fbbbb9ce19 100644
--- a/Svc/FileUplink/Warnings.cpp
+++ b/Svc/FileUplink/Warnings.cpp
@@ -57,6 +57,17 @@ namespace Svc {
     this->warning();
   }
 
+  void FileUplink::Warnings ::
+    packetDuplicate(
+        const U32 sequenceIndex
+    )
+  {
+    this->m_fileUplink->log_WARNING_HI_PacketDuplicate(
+        sequenceIndex
+    );
+    this->warning();
+  }
+
   void FileUplink::Warnings ::
     fileWrite(Fw::LogStringArg& fileName)
   {


Test patch:
diff --git a/Svc/FileUplink/test/ut/FileUplinkMain.cpp b/Svc/FileUplink/test/ut/FileUplinkMain.cpp
index 46feb643a5..fd0f288aa9 100644
--- a/Svc/FileUplink/test/ut/FileUplinkMain.cpp
+++ b/Svc/FileUplink/test/ut/FileUplinkMain.cpp
@@ -49,6 +49,11 @@ TEST(FileUplink, PacketOutOfOrder) {
   tester.packetOutOfOrder();
 }
 
+TEST(FileUplink, PacketDuplicated) {
+  Svc::FileUplinkTester tester;
+  tester.packetDuplicated();
+}
+
 TEST(FileUplink, CancelPacketInStartMode) {
   Svc::FileUplinkTester tester;
   tester.cancelPacketInStartMode();
diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.cpp b/Svc/FileUplink/test/ut/FileUplinkTester.cpp
index 1163807e7f..5ab98cf569 100644
--- a/Svc/FileUplink/test/ut/FileUplinkTester.cpp
+++ b/Svc/FileUplink/test/ut/FileUplinkTester.cpp
@@ -377,6 +377,64 @@ namespace Svc {
 
   }
 
+  void FileUplinkTester ::
+    packetDuplicated()
+  {
+    const char *const sourcePath = "source.bin";
+    const char *const destPath = "dest.bin";
+    U8 packetData[] = { 5, 6, 7, 8, 9 };
+    const size_t fileSize = 2 * PACKET_SIZE;
+
+    // Send the start packet (packet 0)
+    this->sendStartPacket(sourcePath, destPath, fileSize);
+    ASSERT_TLM_SIZE(1);
+    ASSERT_TLM_PacketsReceived(
+        0,
+        ++this->expectedPacketsReceived
+    );
+    ASSERT_EVENTS_SIZE(0);
+
+    ASSERT_EQ(0, component.m_lastSequenceIndex);
+    ASSERT_EQ(1, this->sequenceIndex);
+    ASSERT_EQ(Os::File::MAX_STATUS, component.m_lastPacketWriteStatus);
+
+    // Send data packet 1
+    const size_t byteOffset = 0;
+    this->sendDataPacket(byteOffset, packetData);
+    ASSERT_TLM_SIZE(1);
+    ASSERT_TLM_PacketsReceived(
+        0,
+        ++this->expectedPacketsReceived
+    );
+    ASSERT_TLM_Warnings_SIZE(0);
+
+    // capture the checksum after sending the first packet
+    const ::CFDP::Checksum expectedChecksum(component.m_file.m_checksum);
+
+    // Simulate duplication of packet 1
+    --this->sequenceIndex;
+
+    ASSERT_EQ(this->sequenceIndex, component.m_lastSequenceIndex);
+    ASSERT_EQ(Os::File::OP_OK, component.m_lastPacketWriteStatus);
+
+    // Send data packet 1 again
+    this->sendDataPacket(byteOffset, packetData);
+    ASSERT_TLM_SIZE(2);
+    ASSERT_TLM_PacketsReceived(
+        0,
+        ++this->expectedPacketsReceived
+    );
+    ASSERT_TLM_Warnings(0, 1);
+
+    ASSERT_EVENTS_SIZE(1);
+    ASSERT_EVENTS_PacketDuplicate(0, component.m_lastSequenceIndex);
+
+    // verify that the checksum hasn't changed
+    ASSERT_EQ(expectedChecksum, component.m_file.m_checksum);
+
+    this->removeFile("test.bin");
+  }
+
   void FileUplinkTester ::
     cancelPacketInStartMode()
   {
diff --git a/Svc/FileUplink/test/ut/FileUplinkTester.hpp b/Svc/FileUplink/test/ut/FileUplinkTester.hpp
index 21acb06a47..19703c4770 100644
--- a/Svc/FileUplink/test/ut/FileUplinkTester.hpp
+++ b/Svc/FileUplink/test/ut/FileUplinkTester.hpp
@@ -80,6 +80,10 @@ namespace Svc {
       //!
       void packetOutOfOrder();
 
+      //! Send a file with an duplicated packet
+      //!
+      void packetDuplicated();
+
       //! Send a CANCEL packet in START mode
       //!
       void cancelPacketInStartMode();