diff --git a/CHANGELOG.md b/CHANGELOG.md index 9102d4c..d1e5fc6 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/spacepackets/cfdp/pdu/ack.py b/spacepackets/cfdp/pdu/ack.py index 8f89c02..232f631 100644 --- a/spacepackets/cfdp/pdu/ack.py +++ b/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 diff --git a/spacepackets/cfdp/pdu/file_data.py b/spacepackets/cfdp/pdu/file_data.py index e1647b5..8b9713a 100644 --- a/spacepackets/cfdp/pdu/file_data.py +++ b/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): diff --git a/spacepackets/cfdp/pdu/finished.py b/spacepackets/cfdp/pdu/finished.py index dd98a3e..f318c67 100644 --- a/spacepackets/cfdp/pdu/finished.py +++ b/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): diff --git a/spacepackets/cfdp/pdu/keep_alive.py b/spacepackets/cfdp/pdu/keep_alive.py index be04d18..ba95ac1 100644 --- a/spacepackets/cfdp/pdu/keep_alive.py +++ b/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 ) diff --git a/spacepackets/cfdp/pdu/metadata.py b/spacepackets/cfdp/pdu/metadata.py index c4bcb16..f10a26c 100644 --- a/spacepackets/cfdp/pdu/metadata.py +++ b/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 diff --git a/spacepackets/cfdp/pdu/nak.py b/spacepackets/cfdp/pdu/nak.py index 3a29422..9be30a3 100644 --- a/spacepackets/cfdp/pdu/nak.py +++ b/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 diff --git a/spacepackets/cfdp/pdu/prompt.py b/spacepackets/cfdp/pdu/prompt.py index 3e7c6fb..cfa9101 100644 --- a/spacepackets/cfdp/pdu/prompt.py +++ b/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 diff --git a/tests/cfdp/pdus/test_eof_pdu.py b/tests/cfdp/pdus/test_eof_pdu.py index eb80069..fb1b9b5 100644 --- a/tests/cfdp/pdus/test_eof_pdu.py +++ b/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() diff --git a/tests/cfdp/pdus/test_file_data.py b/tests/cfdp/pdus/test_file_data.py index d048a06..70b3621 100644 --- a/tests/cfdp/pdus/test_file_data.py +++ b/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( diff --git a/tests/cfdp/pdus/test_finished_pdu.py b/tests/cfdp/pdus/test_finished_pdu.py index 49841f2..ef4981c 100644 --- a/tests/cfdp/pdus/test_finished_pdu.py +++ b/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,7 +224,7 @@ 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()) @@ -215,7 +232,7 @@ def test_finished_pdu(self): # 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) diff --git a/tests/cfdp/pdus/test_keep_alive_pdu.py b/tests/cfdp/pdus/test_keep_alive_pdu.py index e278967..c90a9ec 100644 --- a/tests/cfdp/pdus/test_keep_alive_pdu.py +++ b/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() diff --git a/tests/cfdp/pdus/test_metadata.py b/tests/cfdp/pdus/test_metadata.py index 1e25258..8f8a4e0 100644 --- a/tests/cfdp/pdus/test_metadata.py +++ b/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) diff --git a/tests/cfdp/pdus/test_nak_pdu.py b/tests/cfdp/pdus/test_nak_pdu.py index 97178b4..ad5766d 100644 --- a/tests/cfdp/pdus/test_nak_pdu.py +++ b/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