CCSDSPy / ccsdspy

I/O interface and utilities for CCSDS binary spacecraft data in Python. Library used in flight missions at NASA, NOAA, and SWRI
https://ccsdspy.org
BSD 3-Clause "New" or "Revised" License
75 stars 18 forks source link

Implementation of Variable Length Packets #38

Closed ddasilva closed 1 year ago

ddasilva commented 1 year ago

This pull request implements variable length packets, via a custom decoder and interface through the VariableLength class. For convenience, I put a build of the API docs at this URL so reviewers can read: https://danieldasilva.org/var_length/ccsdspy.html

This merge request makes the following assumptions, which I think should meet the majority of use cases that occur in practice. I'm curious about what @ehsteve thinks about these assumptions based the data they're using. It assumes:

  1. There is one variable length field per packet, and it always comes at the end
  2. It starts and ends cleanly on byte boundaries (it doesn't span an "odd" number of bits between bytes)

When you decode a variable length field, you get back an array of dtype=object, where each element is a reference to another array of the specified type (with different lengths each time). One thing to note is that this variable length decoder is slightly (but not impractically) slower than the FixedLength decoder, so we want to continue to support FixedLength.

Since the front page is kind of long, and the rules for this class are worth spelling out, I'm going to add a new page on the sidebar for this (and then link to it inline with the Fixed Length discussion). But for now I want to focus on making sure the decoder is right.

ddasilva commented 1 year ago

Closes #24

ehsteve commented 1 year ago

I worry about your first assumption. It feels like common practice to put a checksum at the end of a packet or some other "footer" data.

ddasilva commented 1 year ago

That’s fine, I just didn’t know. I’ll update the PR

ddasilva commented 1 year ago

I worry about your first assumption. It feels like common practice to put a checksum at the end of a packet or some other "footer" data.

@ehsteve I updated the pull request with this constraint removed, and the docs here have been updated https://danieldasilva.org/var_length/ccsdspy.html

ehsteve commented 1 year ago

Wow! Amazing! I'll have a closer look in the new year. Happy new year!

codecov-commenter commented 1 year ago

Codecov Report

Base: 95.72% // Head: 95.89% // Increases project coverage by +0.16% :tada:

Coverage data is based on head (0d5fc09) compared to base (31b2557). Patch coverage: 95.39% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #38 +/- ## ========================================== + Coverage 95.72% 95.89% +0.16% ========================================== Files 10 13 +3 Lines 562 731 +169 ========================================== + Hits 538 701 +163 - Misses 24 30 +6 ``` | [Impacted Files](https://codecov.io/gh/ddasilva/ccsdspy/pull/38?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva) | Coverage Δ | | |---|---|---| | [ccsdspy/utils.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS91dGlscy5weQ==) | `82.75% <ø> (ø)` | | | [ccsdspy/packet\_fields.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS9wYWNrZXRfZmllbGRzLnB5) | `85.71% <85.71%> (ø)` | | | [ccsdspy/packet\_types.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS9wYWNrZXRfdHlwZXMucHk=) | `94.44% <90.90%> (ø)` | | | [ccsdspy/decode.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS9kZWNvZGUucHk=) | `96.10% <98.80%> (+3.24%)` | :arrow_up: | | [ccsdspy/\_\_init\_\_.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | | | [ccsdspy/tests/test\_hs.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS90ZXN0cy90ZXN0X2hzLnB5) | `100.00% <100.00%> (ø)` | | | [ccsdspy/tests/test\_packet\_fields.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS90ZXN0cy90ZXN0X3BhY2tldF9maWVsZHMucHk=) | `100.00% <100.00%> (ø)` | | | [ccsdspy/tests/test\_packet\_types.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS90ZXN0cy90ZXN0X3BhY2tldF90eXBlcy5weQ==) | `100.00% <100.00%> (ø)` | | | [ccsdspy/tests/test\_primary\_header.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS90ZXN0cy90ZXN0X3ByaW1hcnlfaGVhZGVyLnB5) | `100.00% <100.00%> (ø)` | | | [ccsdspy/tests/test\_split.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS90ZXN0cy90ZXN0X3NwbGl0LnB5) | `98.66% <100.00%> (+1.40%)` | :arrow_up: | | ... and [2 more](https://codecov.io/gh/ddasilva/ccsdspy/pull/38/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jmbhughes commented 1 year ago

This should work great with PUNCH image packets I think. We have just one expanding data field which is (at the moment) always at the end.

ddasilva commented 1 year ago

Thanks @jmbhughes !