Fail Safe on Corrupted Packets, Generate Error Report

[nischayn99]

Test Data + Code.zip

[nischayn99]

Hi @ddasilva,

I've enclosed the test file and the code we were using for parsing the packets.

[ddasilva]

Hi @nischayn99 and @tloubrieu-jpl, can you review a bit of background on this for the myself and the ticket? When I split this file, it has 47 different APIDs in it, but you're trying to parse every APID with the same definition. Do all these APIDs really have the same definition?

A warning is issued that says the file is corrupt, which you mentioned was expected. I think what makes sense to do here is add a keyword argument split_by_apid(mixed_file, truncate_corrupt=True) (it can actually be True by default). When truncate_corrupt=True, it will throw out the extra incomplete packet at the end. This won't save you if data in the middle of your file is corrupt, but this is probably what people want 99% of the time, and if they don't, they can pass truncate_corrupt=False.

As far as error reports, you can convert warnings to exceptions using the warnings module (see how here https://stackoverflow.com/a/30368735). It might help if we throw specific subclasses of CCSDSPyWarning instead of just UserWarning all the time, so you can filter on specific warnings you want to be exceptions. Alternatively, you could route CCSDSPy warnings to a separate log file, and we could work to make sure they are informative.

In general though, the philosophy I think makes sense is to automatically recover with a warning when possible, and throw an exception if not. But we do want to make it easy to collect warnings in a centralize way, so that things are smooth operationally. Let me know any of your thoughts.

[nischayn99]

Hi @ddasilva,

We tried to apply generic definitions of default packets we found definition in ccsdspy (pkt = VariableLength(
[
PacketArray(
name="unused", data_type="uint", bit_length=BITS_PER_BYTE, array_shape="expand"
)
]
)).

Thought it would work for all packets, and defined specific detailed packet definition for one of the APID as a first development step.

Regarding the corrupted packet in the middle of the file, can we find a way to retrieve the next packet from a marker?

We like the idea of CCSDSPy throwing exceptions and let the user decide if they want to catch the error or not.

[ddasilva]

It's generating an exception because it is sometimes using the housekeeping packet definition for a byte stream that doesn't contain packets that match that. When I replace the code to always use the flexible definition, it doesn't raise an exception:

for apid, stream in stream_by_apid.items():
    pkt = default_pkt
    output[apid] = pkt.load(stream)`

Regarding the corrupted packet in the middle of the file, can we find a way to retrieve the next packet from a marker?

I don't think this is deterministically possible given the CCSDS specification. If you and @tloubrieu-jpl have a way of doing this, I would love to hear it.

We like the idea of CCSDSPy throwing exceptions and let the user decide if they want to catch the error or not.

That sounds good then-- we can make this ticket about that. What I'll do for this issue is two things:

1. Implement truncate_corrupt=True option for split_by_apid() and a few other functions where it makes sense
2. Add raise_on_warning=True option for pkt.load(), split_by_apid(), and a few other places where it makes sense

[Firedeus]

I'd like to pick this up if no one's on it. Confirming the plan from the thread still holds on main: neither truncate_corrupt nor raise_on_warning exists yet, and a truncated trailing packet is currently handled in iter_packet_bytes() by log.warning(...) + break (silently dropped), so this is still unimplemented.

Proposed scope, following the agreed design:

• split_by_apid(mixed_file, valid_apids=None, truncate_corrupt=True) — True keeps today's behaviour (drop the incomplete trailing packet); False raises instead.

• raise_on_warning=False on FixedLength.load / VariableLength.load (and split_by_apid) to let callers escalate warnings to exceptions.

One implementation question before I code. The load-time warnings (missing packets, out-of-order sequence, APID mismatch in _inspect_primary_header_fields, and the truncation warning in iter_packet_bytes) currently go through the logging module (log.warning), not warnings.warn. For raise_on_warning I see two paths:

(a) route these through Python's warnings module, so raise_on_warning just installs a filterwarnings("error") and users can filter idiomatically; or
(b) keep logging and add explicit if raise_on_warning: raise <PacketError> at each site.

(a) is more Pythonic but changes how current users capture these messages; (b) is more contained. Do you have a preference?

I'll add tests with corrupt/truncated fixtures and update the docs. BSD-3, of course — happy to be assigned.

[ddasilva]

Thanks for offering to work on this @Firedeus ! I'm partial to option (b) because we've already made an effort to move away from the warning's module and going back to using it seems like going backwards.