diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e3add1e..06959b5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Make `CITATION.cff` conform to CFF 1.2.0 so citation exports work, and update the metadata consistency check to use CFF contacts while keeping package descriptions separate from the citation abstract. [#285](https://github.com/lasp/space_packet_parser/issues/285) +- Raise `UnrecognizedPacketTypeError` (with `partial_data` populated) when an abstract container has + no valid inheritors, even if the definition has no `PKT_APID` parameter. Previously the error + message was built by subscripting `packet['PKT_APID']`, so definitions that do not use CCSDS + naming got a `KeyError` from inside the library instead of the documented exception. The message + still reports the APID when the packet has one. As part of this, `str(CCSDSPacketBytes)` no longer + raises `IndexError` for inputs shorter than a full six-byte primary header and instead renders the + bytes it has, so it is safe to use in diagnostic messages. + [#276](https://github.com/lasp/space_packet_parser/issues/276) ## [6.2.0] - 2026-09-13 diff --git a/space_packet_parser/exceptions.py b/space_packet_parser/exceptions.py index 71b06f94..1d41f9e4 100644 --- a/space_packet_parser/exceptions.py +++ b/space_packet_parser/exceptions.py @@ -38,7 +38,7 @@ class InvalidParameterTypeError(Exception): class UnrecognizedPacketTypeError(Exception): - """Error raised when we can't figure out which kind of packet we are dealing with based on the header""" + """Error raised when the packet type cannot be determined from the parsed data and the container restriction criteria""" def __init__(self, *args, partial_data: dict = None): """ diff --git a/space_packet_parser/generators/ccsds.py b/space_packet_parser/generators/ccsds.py index 59cdedc5..8a6430a7 100644 --- a/space_packet_parser/generators/ccsds.py +++ b/space_packet_parser/generators/ccsds.py @@ -40,6 +40,10 @@ class CCSDSPacketBytes(bytes): HEADER_LENGTH_BYTES = 6 def __str__(self) -> str: + if len(self) < self.HEADER_LENGTH_BYTES: + # Too short to hold a full primary header. Render what we have rather than raising an IndexError + # from the header properties so this stays usable in diagnostic messages. + return f"CCSDSPacket Header: (incomplete, {len(self)} of {self.HEADER_LENGTH_BYTES} bytes: {self.hex()})" return ( f"CCSDSPacket Header: ({self.version_number=}, {self.type=}, " f"{self.secondary_header_flag=}, {self.apid=}, {self.sequence_flags=}, " diff --git a/space_packet_parser/xtce/definitions.py b/space_packet_parser/xtce/definitions.py index bd499d74..aa02e1b6 100644 --- a/space_packet_parser/xtce/definitions.py +++ b/space_packet_parser/xtce/definitions.py @@ -450,12 +450,19 @@ def parse_bytes(self, binary_data: bytes, *, root_container_name: str | None = N if len(valid_inheritors) == 0: if current_container.abstract: - raise UnrecognizedPacketTypeError( - f"Detected an abstract container with no valid inheritors by restriction criteria. " - f"This might mean this packet type is not accounted for in the provided packet definition. " - f"APID={packet['PKT_APID']}.", - partial_data=packet, + message = ( + f"Detected an abstract container ({current_container.name}) with no valid inheritors by " + f"restriction criteria. This might mean this packet type is not accounted for in the " + f"provided packet definition." ) + # XTCE has no notion of the CCSDS standard, so a PKT_APID parameter is not guaranteed to exist. + # Report the APID when it is available because it is the most useful diagnostic for CCSDS + # packets, but never let the error report itself fail on a non-CCSDS definition. + if "PKT_APID" in packet: + message += f" APID={packet['PKT_APID']}." + elif isinstance(packet.binary_data, ccsds.CCSDSPacketBytes): + message += f" {packet.binary_data}." + raise UnrecognizedPacketTypeError(message, partial_data=packet) break raise UnrecognizedPacketTypeError( diff --git a/tests/unit/test_generators/test_ccsds.py b/tests/unit/test_generators/test_ccsds.py index b587bb1e..ee1ef9f9 100644 --- a/tests/unit/test_generators/test_ccsds.py +++ b/tests/unit/test_generators/test_ccsds.py @@ -337,3 +337,19 @@ def test_ccsds_generator_packet_length_math(data_length_bytes): parsed_p = next(space_packet_parser.generators.ccsds_generator(p)) assert len(parsed_p) == data_length_bytes + 6 assert parsed_p.data_length == data_length_bytes - 1 + + +def test_ccsds_packet_bytes_str_short_input(): + """Test that str(CCSDSPacketBytes) does not raise when there are fewer bytes than a full primary header + + The string form is used in diagnostic messages, so it must never fail on the failure path. + """ + short_packet = ccsds.CCSDSPacketBytes(b"\x07") + rendered = str(short_packet) + assert "incomplete" in rendered + assert "1 of 6 bytes" in rendered + assert "07" in rendered + + # A full header renders the parsed fields as before + full_header = ccsds.create_ccsds_packet(data=b"\x00", apid=11) + assert "apid=11" in str(full_header) diff --git a/tests/unit/test_xtce/test_definitions.py b/tests/unit/test_xtce/test_definitions.py index a549c38c..e1364c6a 100644 --- a/tests/unit/test_xtce/test_definitions.py +++ b/tests/unit/test_xtce/test_definitions.py @@ -8,8 +8,37 @@ import space_packet_parser as spp import space_packet_parser.generators.ccsds import space_packet_parser.xtce.parameter_types +from space_packet_parser.exceptions import UnrecognizedPacketTypeError from space_packet_parser.xtce import comparisons, containers, definitions, encodings, parameters +# Minimal definition with no CCSDS header and no PKT_APID parameter (see issue #276) +NO_CCSDS_XTCE = """ + + + + + + + + + + + + + + + + + + + + + + + + +""" + def test_xtce_definition_from_xtce_inputs(test_data_dir): """Test that we can create an XtcePacketDefinition from various inputs""" @@ -649,3 +678,58 @@ def test_parse_packet_too_few_bytes(test_data_dir): r"Tried to read 32 bits from position 504 in a packet of length 528 bits.", ): xdef.parse_bytes(too_short_packet_data) + + +def test_parse_bytes_unrecognized_packet_without_pkt_apid(): + """Test that an unrecognized packet raises UnrecognizedPacketTypeError for a definition with no PKT_APID + + XTCE has no notion of the CCSDS standard, so the error path must not assume a parameter named PKT_APID exists. + Regression test for https://github.com/lasp/space_packet_parser/issues/276 + """ + xdef = definitions.XtcePacketDefinition.from_xtce(io.StringIO(NO_CCSDS_XTCE), root_container_name="AbstractRoot") + + # COUNTER=7, so the Child restriction criteria (COUNTER == 99) fail and AbstractRoot has no valid inheritors + with pytest.raises(UnrecognizedPacketTypeError, match=r"abstract container \(AbstractRoot\)") as exc_info: + xdef.parse_bytes(b"\x07") + + # The message must not pretend to know an APID that does not exist in this definition + assert "APID" not in str(exc_info.value) + # The partially parsed packet is the main diagnostic the error exists to carry + assert exc_info.value.partial_data is not None + assert exc_info.value.partial_data["COUNTER"] == 7 + + +def test_parse_bytes_unrecognized_packet_reports_apid(test_data_dir): + """Test that an unrecognized CCSDS packet still reports its APID in the UnrecognizedPacketTypeError message""" + xdef = definitions.XtcePacketDefinition.from_xtce(test_data_dir / "test_xtce.xml") + + # APID 2047 is not defined in test_xtce.xml + unknown_apid_packet = space_packet_parser.generators.ccsds.create_ccsds_packet( + data=bytes(65), apid=2047, sequence_flags=space_packet_parser.generators.ccsds.SequenceFlags.UNSEGMENTED + ) + + with pytest.raises(UnrecognizedPacketTypeError, match="APID=2047") as exc_info: + xdef.parse_bytes(unknown_apid_packet) + + assert exc_info.value.partial_data["PKT_APID"] == 2047 + + +def test_parse_bytes_unrecognized_packet_ccsds_bytes_without_pkt_apid(): + """Test that the CCSDS header printout is used when the bytes are CCSDSPacketBytes but no PKT_APID is parsed + + Regression test for the fallback branch added in https://github.com/lasp/space_packet_parser/pull/282 + """ + xdef = definitions.XtcePacketDefinition.from_xtce(io.StringIO(NO_CCSDS_XTCE), root_container_name="AbstractRoot") + + # COUNTER=7 fails the Child restriction criteria. Pad to a full 6-byte header so the printout has real values. + ccsds_bytes = space_packet_parser.generators.ccsds.CCSDSPacketBytes(b"\x07" + bytes(7)) + with pytest.raises(UnrecognizedPacketTypeError, match="CCSDSPacket Header") as exc_info: + xdef.parse_bytes(ccsds_bytes) + + assert "APID=" not in str(exc_info.value) + assert exc_info.value.partial_data["COUNTER"] == 7 + + # Even a CCSDSPacketBytes too short for a full header must not break error reporting + with pytest.raises(UnrecognizedPacketTypeError, match="incomplete, 1 of 6 bytes") as exc_info: + xdef.parse_bytes(space_packet_parser.generators.ccsds.CCSDSPacketBytes(b"\x07")) + assert exc_info.value.partial_data["COUNTER"] == 7