Rest of roundtrip fixes with CRC + CHANGELOG

CHANGELOG.md

@@ -8,6 +8,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 # [unreleased]
 
+## Fixed
+
+- Unpacking / re-packing was buggy for some file directives when the PDU checksum was activated.
+  This was fixed for the following PDUs:
+  - NAK
+  - EOF
+  - File Data
+  - Metadata
+  - Finished
+
 # [v0.26.0] 2024-11-27
 
 - Python 3.8 is not supported anymore as it has reached end-of-life.

spacepackets/cfdp/pdu/ack.py

@@ -76,7 +76,9 @@ def directive_type(self) -> DirectiveType:
     def pdu_header(self) -> PduHeader:
         return self.pdu_file_directive.pdu_header
 
-    def __eq__(self, other: AckPdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, AckPdu):
+            return False
         return (
             self.pdu_file_directive == other.pdu_file_directive
             and self.directive_code_of_acked_pdu == other.directive_code_of_acked_pdu

spacepackets/cfdp/pdu/file_data.py

@@ -266,7 +266,9 @@ def unpack(cls, data: bytes) -> FileDataPdu:
     def packet_len(self) -> int:
         return self.pdu_header.packet_len
 
-    def __eq__(self, other: FileDataPdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, FileDataPdu):
+            return False
         return self.pdu_header == other.pdu_header and self._params == other._params
 
     def __repr__(self):

spacepackets/cfdp/pdu/finished.py

@@ -234,8 +234,11 @@ def unpack(cls, data: bytes) -> FinishedPdu:
         finished_pdu.condition_code = params.condition_code
         finished_pdu._params = params
         current_idx += 1
-        if len(data) > current_idx:
-            finished_pdu._unpack_tlvs(rest_of_packet=data[current_idx : finished_pdu.packet_len])
+        end_of_optional_tlvs_idx = finished_pdu.packet_len
+        if finished_pdu.pdu_header.crc_flag == CrcFlag.WITH_CRC:
+            end_of_optional_tlvs_idx -= 2
+        if end_of_optional_tlvs_idx > current_idx:
+            finished_pdu._unpack_tlvs(rest_of_packet=data[current_idx:end_of_optional_tlvs_idx])
         return finished_pdu
 
     def _unpack_tlvs(self, rest_of_packet: bytes) -> int:
@@ -263,7 +266,9 @@ def _unpack_tlvs(self, rest_of_packet: bytes) -> int:
             self.fault_location = fault_loc
         return current_idx
 
-    def __eq__(self, other: FinishedPdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, FinishedPdu):
+            return False
         return self._params == other._params and self.pdu_file_directive == other.pdu_file_directive
 
     def __repr__(self):

spacepackets/cfdp/pdu/keep_alive.py

@@ -109,7 +109,9 @@ def unpack(cls, data: bytes) -> KeepAlivePdu:
     def packet_len(self) -> int:
         return self.pdu_file_directive.packet_len
 
-    def __eq__(self, other: KeepAlivePdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, KeepAlivePdu):
+            return False
         return (
             self.pdu_file_directive == other.pdu_file_directive and self.progress == other.progress
         )

spacepackets/cfdp/pdu/metadata.py

@@ -265,7 +265,9 @@ def __repr__(self):
             f" options={self.options!r}, pdu_conf={self.pdu_file_directive.pdu_conf})"
         )
 
-    def __eq__(self, other: MetadataPdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, MetadataPdu):
+            return False
         return (
             self.pdu_file_directive == other.pdu_file_directive
             and self.params.closure_requested == other.params.closure_requested

spacepackets/cfdp/pdu/nak.py

@@ -264,15 +264,18 @@ def unpack(cls, data: bytes) -> NakPdu:
             data[current_idx : current_idx + struct_arg_tuple[1]],
         )[0]
         current_idx += struct_arg_tuple[1]
-        if current_idx < len(data):
-            packet_size_check = (len(data) - current_idx) % (struct_arg_tuple[1] * 2)
+        end_of_segment_req_idx = len(data)
+        if nak_pdu.pdu_header.crc_flag == CrcFlag.WITH_CRC:
+            end_of_segment_req_idx -= 2
+        if current_idx < end_of_segment_req_idx:
+            packet_size_check = (end_of_segment_req_idx - current_idx) % (struct_arg_tuple[1] * 2)
             if packet_size_check != 0:
                 raise ValueError(
                     "Invalid size for remaining data, "
                     f"which should be a multiple of {struct_arg_tuple[1] * 2}"
                 )
             segment_requests = []
-            while current_idx < len(data):
+            while current_idx < end_of_segment_req_idx:
                 start_of_segment = struct.unpack(
                     struct_arg_tuple[0],
                     data[current_idx : current_idx + struct_arg_tuple[1]],
@@ -289,7 +292,9 @@ def unpack(cls, data: bytes) -> NakPdu:
             nak_pdu.segment_requests = segment_requests
         return nak_pdu
 
-    def __eq__(self, other: NakPdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, NakPdu):
+            return False
         return (
             self.pdu_file_directive == other.pdu_file_directive
             and self._segment_requests == other._segment_requests

spacepackets/cfdp/pdu/prompt.py

@@ -83,7 +83,9 @@ def unpack(cls, data: bytes) -> PromptPdu:
         prompt_pdu.response_required = ResponseRequired((data[current_idx] & 0x80) >> 7)
         return prompt_pdu
 
-    def __eq__(self, other: PromptPdu):
+    def __eq__(self, other: object):
+        if not isinstance(other, PromptPdu):
+            return False
         return (
             self.pdu_file_directive == other.pdu_file_directive
             and self.response_required == other.response_required

tests/cfdp/pdus/test_eof_pdu.py

@@ -66,6 +66,7 @@ def test_with_crc(self):
         # verify we can round-trip pack/unpack
         eof_unpacked = EofPdu.unpack(data=eof_raw)
         self.assertEqual(eof_unpacked, eof)
+        self.assertEqual(eof_unpacked.pack(), eof.pack())
 
     def test_from_factory(self):
         eof_pdu_raw = self.eof_pdu.pack()

tests/cfdp/pdus/test_file_data.py

@@ -86,6 +86,7 @@ def test_pack_unpack_w_crc(self):
         self.assertEqual(packed, expected_bytes)
         unpacked = FileDataPdu.unpack(packed)
         self.assertEqual(unpacked, fd_pdu)
+        self.assertEqual(unpacked.pack(), fd_pdu.pack())
 
     def test_with_seg_metadata(self):
         fd_params = FileDataParams(

tests/cfdp/pdus/test_finished_pdu.py

@@ -9,7 +9,7 @@
     TlvType,
 )
 from spacepackets.cfdp.conf import PduConfig
-from spacepackets.cfdp.defs import DeliveryCode, Direction, FileStatus
+from spacepackets.cfdp.defs import CrcFlag, DeliveryCode, Direction, FileStatus
 from spacepackets.cfdp.pdu import FinishedPdu
 from spacepackets.cfdp.pdu.finished import (
     FinishedParams,
@@ -73,7 +73,7 @@ def test_unpack_basic(self):
             pdu_conf=self.pdu_conf,
         )
         finish_pdu_raw = finish_pdu.pack()
-        finish_pdu_unpacked = FinishedPdu.unpack(data=finish_pdu_raw)
+        finish_pdu_unpacked = FinishedPdu.unpack(data=bytes(finish_pdu_raw))
         self.assertEqual(finish_pdu_unpacked.finished_params, self.params)
         self.assertEqual(finish_pdu_unpacked.delivery_code, DeliveryCode.DATA_COMPLETE)
         self.assertEqual(finish_pdu_unpacked.direction, Direction.TOWARDS_SENDER)
@@ -93,7 +93,7 @@ def test_unpack_failure(self):
             pdu_conf=self.pdu_conf,
         )
         finish_pdu_raw = finish_pdu.pack()
-        finish_pdu_unpacked = FinishedPdu.unpack(data=finish_pdu_raw)
+        finish_pdu_unpacked = FinishedPdu.unpack(bytes(finish_pdu_raw))
         finish_pdu_repacked = finish_pdu_unpacked.pack()
         finish_pdu_repacked = finish_pdu_repacked[:-1]
         self.assertRaises(ValueError, FinishedPdu.unpack, data=finish_pdu_repacked)
@@ -104,7 +104,7 @@ def test_unpack_failure(self):
         finish_pdu_raw[1] = (current_size & 0xFF00) >> 8
         finish_pdu_raw[2] = current_size & 0x00FF
         with self.assertRaises(ValueError):
-            FinishedPdu.unpack(data=finish_pdu_raw)
+            FinishedPdu.unpack(bytes(finish_pdu_raw))
 
     def test_with_fault_location(self):
         # Now generate a packet with a fault location
@@ -134,6 +134,23 @@ def test_with_fault_location(self):
         self.assertEqual(len(finish_pdu_with_fault_loc.pack()), 13)
         self.assertEqual(finish_pdu_with_fault_loc.fault_location_len, 4)
 
+    def test_roundtrip_with_fault_location_and_crc(self):
+        params = FinishedParams(
+            delivery_code=DeliveryCode.DATA_INCOMPLETE,
+            file_status=FileStatus.DISCARDED_DELIBERATELY,
+            condition_code=ConditionCode.POSITIVE_ACK_LIMIT_REACHED,
+            fault_location=self.fault_location_tlv,
+        )
+        self.pdu_conf.crc_flag = CrcFlag.WITH_CRC
+        finish_pdu_with_fault_loc = FinishedPdu(
+            params=params,
+            pdu_conf=self.pdu_conf,
+        )
+        fd_raw = finish_pdu_with_fault_loc.pack()
+        finish_pdu_unpack = FinishedPdu.unpack(bytes(fd_raw))
+        self.assertEqual(finish_pdu_unpack, finish_pdu_with_fault_loc)
+        self.assertEqual(finish_pdu_unpack.pack(), finish_pdu_with_fault_loc.pack())
+
     def test_with_fs_response(self):
         # Now create a packet with filestore responses
         filestore_response_1_packed = self.filestore_reponse_1.pack()
@@ -170,7 +187,7 @@ def test_with_fs_response(self):
         expected_array = bytearray([0x28, 0x00, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x05, 0x44])
         expected_array.extend(filestore_response_1_packed)
         self.assertEqual(expected_array, pdu_with_response_raw)
-        pdu_with_response_unpacked = FinishedPdu.unpack(data=pdu_with_response_raw)
+        pdu_with_response_unpacked = FinishedPdu.unpack(bytes(pdu_with_response_raw))
         self.assertEqual(len(pdu_with_response_unpacked.file_store_responses), 1)
 
     def test_finished_pdu(self):
@@ -207,15 +224,15 @@ def test_finished_pdu(self):
         fs_responses = finish_pdu_two_responses_one_fault_loc.file_store_responses
         self.assertEqual(len(fs_responses), 2)
         complex_pdu_raw = finish_pdu_two_responses_one_fault_loc.pack()
-        complex_pdu_unpacked = FinishedPdu.unpack(data=complex_pdu_raw)
+        complex_pdu_unpacked = FinishedPdu.unpack(bytes(complex_pdu_raw))
         self.assertEqual(complex_pdu_unpacked.fault_location.pack(), self.fault_location_tlv.pack())
         self.assertEqual(self.filestore_reponse_1.pack(), fs_responses[0].pack())
         self.assertEqual(filestore_reponse_2.pack(), fs_responses[1].pack())
 
         # Change TLV type to make it invalid
         complex_pdu_raw[-5] = TlvType.FILESTORE_RESPONSE
         with self.assertRaises(ValueError):
-            FinishedPdu.unpack(data=complex_pdu_raw)
+            FinishedPdu.unpack(bytes(complex_pdu_raw))
 
     def test_from_factory(self):
         finish_pdu = FinishedPdu.success_pdu(pdu_conf=self.pdu_conf)

tests/cfdp/pdus/test_keep_alive_pdu.py

@@ -48,7 +48,7 @@ def test_keep_alive_pdu(self):
         keep_alive_pdu_large = KeepAlivePdu(pdu_conf=self.pdu_conf, progress=0)
         keep_alive_pdu_invalid = keep_alive_pdu_large.pack()[:-1]
         with self.assertRaises(ValueError):
-            KeepAlivePdu.unpack(data=keep_alive_pdu_invalid)
+            KeepAlivePdu.unpack(bytes(keep_alive_pdu_invalid))
 
     def test_unpack(self):
         keep_alive_pdu_raw = self.keep_alive_pdu.pack()

tests/cfdp/pdus/test_metadata.py

@@ -70,6 +70,7 @@ def test_metadata_with_crc(self):
         # verify a round-trip works w/ CRC set.
         metadata_unpacked = MetadataPdu.unpack(metadata_raw)
         self.assertEqual(metadata_pdu, metadata_unpacked)
+        self.assertEqual(metadata_pdu.pack(), metadata_unpacked.pack())
 
     def test_metadata_pdu(self):
         self.assertEqual(self.option_0.packet_len, 13)

tests/cfdp/pdus/test_nak_pdu.py

@@ -188,6 +188,17 @@ def test_with_file_segments_1(self):
         nak_unpacked = NakPdu.unpack(data=nak_packed)
         self.assertEqual(nak_unpacked.pack(), nak_packed)
 
+    def test_roundtrip_with_file_segments_and_crc(self):
+        self.nak_pdu.start_of_scope = 0
+        self.nak_pdu.end_of_scope = 200
+        segment_requests = [(20, 40), (60, 80)]
+        self.nak_pdu.pdu_header.crc_flag = CrcFlag.WITH_CRC
+        self.nak_pdu.segment_requests = segment_requests
+        nak_raw = self.nak_pdu.pack()
+        nak_unpacked = NakPdu.unpack(nak_raw)
+        self.assertEqual(nak_unpacked, self.nak_pdu)
+        self.assertEqual(nak_unpacked.pack(), self.nak_pdu.pack())
+
     def test_nak_pdu_errors(self):
         self.nak_pdu.start_of_scope = 0
         self.nak_pdu.end_of_scope = 200