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
74 stars 18 forks source link

Add a few convenience functions to the utils module #59

Closed ddasilva closed 1 year ago

ddasilva commented 1 year ago

These are convenience functions, for debugging purposes by both users and package developers.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 96.90% and project coverage change: +0.53 :tada:

Comparison is base (6a1e931) 94.29% compared to head (524b6b3) 94.82%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #59 +/- ## ========================================== + Coverage 94.29% 94.82% +0.53% ========================================== Files 5 6 +1 Lines 403 464 +61 ========================================== + Hits 380 440 +60 - Misses 23 24 +1 ``` | [Impacted Files](https://codecov.io/gh/ddasilva/ccsdspy/pull/59?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva) | Coverage Δ | | |---|---|---| | [ccsdspy/decode.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/59?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS9kZWNvZGUucHk=) | `94.28% <93.02%> (-0.52%)` | :arrow_down: | | [ccsdspy/constants.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/59?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS9jb25zdGFudHMucHk=) | `100.00% <100.00%> (ø)` | | | [ccsdspy/utils.py](https://codecov.io/gh/ddasilva/ccsdspy/pull/59?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Daniel+da+Silva#diff-Y2NzZHNweS91dGlscy5weQ==) | `94.02% <100.00%> (+11.27%)` | :arrow_up: | 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.

ddasilva commented 1 year ago

Doc build for this branch: https://ccsdspy.readthedocs.io/en/added_utils/api.html

ehsteve commented 1 year ago

These functions seem like they would be useful. They could really use some examples though because it is not very clear exactly what they do. For count_packets, how about issuing a warning if the last packet is not complete? Also some of the code is being repeated from other areas in the code base? Would it be possible to reduce duplication? Final question, do these functions work on both fixed length and variable length packets?

ddasilva commented 1 year ago

Yes, these functions work with mixed APID streams which can include both variable length and fixed length packets. I added something about this in the function doc now to make this clear.

I also wrote a user guide page with examples: https://ccsdspy.readthedocs.io/en/added_utils/user-guide/utils.html

I reduced a bit of code duplication in utils.py. Right now count_packets() uses its own loop because it gives the option of returning the number of missing bytes at the end (I want a way to calculate this programmatically without having to intercept a warning message). I'm not sure how to bolt on that extra return value onto iter_packet_bytes(), which is a generator.

I started down having the variable length decoder (decode.py) use the iter_packet_bytes() function but stopped because (a) it generates circular imports (b) it only saves a few lines of code. Do you feel strongly about this? If so, it might make more sense to put iter_packet_bytes() in ccsdspy/core.py to resolve the circular dependency issue.

ehsteve commented 1 year ago

One other quick comment which could be applied to the entire package actually, there are many places that use magic numbers like how many bytes or bits are in the ccsds header. These numbers are used in calculations without comments. Someone new to the package (and our future selves) could find these numbers confusing. I would recommend they be defined as constants. Since many are used throughout the package, they could be defined in __init__ potentially and imported when needed.

ddasilva commented 1 year ago

That's a reasonable comment-- I reviewed the code and most of the magic numbers are things that can be put in a file like ccsdspy/constants.py, eg the number of bytes in the primary header. In some cases, there are thingslike x*256 which are effectively doing a left bitshift, and may be written better with the << operator using a left shift amount defined by a constant. I have some things to clean up here.

ddasilva commented 1 year ago

I cleaned up the majority of the magic numbers in the code, and introduced ccsdspy/constants.py the currently has BITS_PER_BYTE and PRIMARY_HEADER_NUM_BYTES. I edited the code to use these as much as possible.

Per the duplication (especially in finding the total number of bytes and APID of the packet), I created _get_packet_total_bytes(primary_header_bytes) and _get_packet_apid(primary_header_bytes). I also expose these through utils, because they might be useful when debugging packet creation.

Let me know if you think this is good to merge. I expect that the desire to avoid duplication and magic numbers will be an ongoing effort and not just limited to this PR.

ddasilva commented 1 year ago

All changes addressed; let me know if you have any other feedback or if we're ready to merge