Document generic (non-CCSDS) packet parsing with a custom generator example - #288
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
=======================================
Coverage 94.62% 94.62%
=======================================
Files 49 49
Lines 4203 4203
=======================================
Hits 3977 3977
Misses 226 226 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate socket-handling issues remain, along with requested documentation and regression-test improvements.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a runnable non-CCSDS packet-parsing example with custom generator documentation.
Changes:
- Adds sync-marker and length-based packet parsing example.
- Documents generator contracts and custom length handling.
- Links the example and updates the changelog.
File summaries
| File | Summary | Final comments |
|---|---|---|
examples/parsing_non_ccsds_packets.py |
Custom non-CCSDS generator example | Moderate (3 votes): socket handling mismatches the advertised API. Nit (1): add stronger regression assertions. |
docs/source/user_guide/generators.md |
Generator contracts and usage guidance | Moderate (2 votes): socket handling is inconsistent with the documented source types. Nit (1): the PKT_APID grouping statement is too broad. |
docs/source/examples.md |
Links the new example | None. |
CHANGELOG.md |
Adds the unreleased feature entry | None. |
Review details
Suppressed comments (3)
docs/source/user_guide/generators.md:118
- If the input ends after a complete sync marker but before its length byte, this indexing raises
IndexErrorinstead of taking the documented truncated-stream path. Add a header-length check before readingpayload_length, as the runnable implementation does.
payload_length = buffer[start + len(sync_marker)]
docs/source/user_guide/generators.md:144
create_datasetdoes not inspect the parsedPKT_APID; it uses anapidproperty on the yielded bytes and falls back to 0 only when that property is absent (space_packet_parser/xarr.py:212-216). A custombytessubclass can therefore route packets with noPKT_APIDfield to a nonzero dataset, so this statement is too broad.
returns its Datasets keyed by APID, so packets that have no `PKT_APID` field are all grouped under
key `0`.
examples/parsing_non_ccsds_packets.py:165
- This new behavior is only smoke-tested: the loop prints parsed packets and later indexes
datasets[0], while CI runs plainpython, so a shortened-but-nonempty result or wrong packet values can still pass and warnings are not errors. Add assertions for the packet count/field values and dataset length (or a focused generator test) so the documented boundary and grouping behavior is actually regression-tested.
for packet_bytes in sync_marker_generator(stream):
packet = packet_definition.parse_bytes(packet_bytes, root_container_name=ROOT_CONTAINER_NAME)
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add examples/parsing_non_ccsds_packets.py, a runnable demonstration of parsing a packet format that has no CCSDS header. The example defines a made-up sensor format delimited by a 0xDEADBEEF sync marker with a packet-defined length byte, supplies a custom packet bytes generator that finds packet boundaries from those two fields, parses the packets both one at a time and through create_dataset, and shows that a definition whose root container is not named "CCSDSPacket" must pass root_container_name to parse_bytes. The XTCE is embedded inline rather than added to tests/test_data/ because the existing non-CCSDS fixtures do not demonstrate a packet-defined length field (test_xtce_4byte.xml has no length field, and udp_packet.xml is already served by the built-in udp_generator), and because the CI run-examples job executes every script in examples/ from that directory, so a self-contained script has no data-path dependency. This mirrors the inline-XTCE pattern already used by the Quickstart in getting_started.md. The "XTCE is not limited to CCSDS" prose already existed in the Packet Bytes Generators user guide page, so the doc changes are scoped to stating the generator contract, adding the length-field generator case, and linking the new script from the Examples page. Co-Authored-By: Claude Opus 5 <[email protected]>
- Drop the unimplemented socket.socket support from sync_marker_generator's signature/docstring in both the docs snippet and the runnable example; point readers to the socket-based IDEX waveform example and to ccsds_generator, which does support sockets - Add the missing header-length bounds check in the docs snippet so it matches the runnable example's truncation handling - Correct the create_dataset APID-grouping explanation: keyed by the apid property on the yielded bytes object, not the parsed PKT_APID field - Add assertions on parsed packet values and dataset length to the runnable example so it regression-tests the documented behavior Co-Authored-By: Claude Sonnet 5 <[email protected]>
6a28e58 to
e19b666
Compare
|
Replying to the three suppressed findings from the Copilot review (no separate threads were created for these):
🤖 AI-assisted comment, reviewed and approved by @medley56 before posting. |
Summary
Closes #190.
The library has always supported non-CCSDS packet formats via user-supplied packet bytes
generators, but there was no worked example of doing so. This adds one, plus the small amount of
documentation needed to make the mechanism discoverable.
New example:
examples/parsing_non_ccsds_packets.pyIt demonstrates, end to end and with no external data files:
| SYNC 0xDEADBEEF (4 B) | PAYLOAD_LENGTH (1 B) | COUNTER (2 B) | TEMPERATURE (2 B) |sync_marker_generator, that finds packet boundaries from async marker and a packet-defined length field — the "use a custom field in the packet to
determine its length" case named in the issue.
or
parse_byteswill warn about a bit-count mismatch.CCSDSPacketmust passroot_container_nametoparse_bytes—load_xtcedoes not accept that argument.create_datasetviapacket_bytes_generator, andthat packets with no
PKT_APIDfield are grouped under key0.The XTCE is embedded inline as a string rather than added as a fixture under
tests/test_data/.The existing non-CCSDS fixtures don't fit (
test_xtce_4byte.xmlhas no length field;udp_packet.xmlis already served by the built-inudp_generator), and a self-contained script hasno data-path dependency when CI runs it. This is the same inline-XTCE pattern the Quickstart in
getting_started.mdalready uses.Documentation
docs/source/user_guide/generators.mdalready carried the conceptual prose ("XTCE is not limitedto representing CCSDS packet structures..."), so this change is scoped to the missing example plus a
tightened generator contract. The "Writing Custom Generators" section now states what a generator
must satisfy (accepts a binary source; yields exactly one packet per iteration; each chunk contains
every byte the container describes; may yield a
bytessubclass carrying metadata forpacket_filter), adds the length-field generator case, and links the new script. The"XTCE is not CCSDS-specific" paragraph was promoted to its own heading so it is linkable. The page
is otherwise unchanged.
docs/source/examples.mdgets a bullet for the new script.Testing
The example is exercised by the CI
run-examplesjob, whichcd examplesand runs every top-level*.py. That is the test coverage for this change — the example is itself the test, and it failsloudly if the documented behavior regresses. Verified locally from both
examples/and the reporoot, and under
python -W error(clean — no bit-count-mismatch warning). Existing examples allstill run,
pre-commit run --all-filespasses, the docs build with no new warnings, and the fullsuite is 481 passed.
One design note: the example deliberately uses a single concrete container, with no abstract
containers or restriction criteria, because the
UnrecognizedPacketTypeErrormessage indefinitions.pyinterpolatespacket['PKT_APID']and would raiseKeyErroron a non-CCSDSdefinition. That is tracked separately as #276 and is out of scope here.
🤖 Generated with Claude Code