First cut at adding ability to create packets from FixedLength

[ehsteve]

@ddasilva wanted to get your opinion before heading too far with this as it adds a dependency.

To do

• [ ] Need to test if variable length packet and a non-variable array exists together.
• [ ] Add documentation pages

[ddasilva]

This is a great and welcome change. Thank you! I made some comments on the code to keep it clean for future generations. I think we should consider moving the packing logic to _encode_fixed_length() in encode.py to mimic what we're already doing with decode.py.

I think we could start by making this as an experimental API (accepted feature, but interface subject to change) when described in the docs, encase we want to tweak something later, and then in a minor revision or two we can mark it as stable. The reason being that I am thinking of ways of defining packets with optional fields that are only included when the secondary header flag is 1; doing this would require an API change in to_file()

[ehsteve]

@ddasilva sorry, i don't think i saw your code comments anywhere. I've updated the code to now also handle variable length packets. I worry that this is going to be pretty slow but at least it works.

[ddasilva]

@ehsteve Thanks for much for this. Would you mind if I pushed to your user branch to help out? I want to do some things like writing additional tests / doc and moving the bulk of the encoding logic to ccsdspy/encode.py (so it mirrors ccsdspy/decode.py). If there's a better way to do this, let me know.

I worry that this is going to be pretty slow but at least it works

What we've been doing is keeping all the decoder bit manipulation logic contained within a few key internal API methods (_decode_fixed_length and _decode_variable_length in ccsdspy/decode.py). That way, if someone wants to rewrite the decoder bit manipulation logic for performance, they only need to replace those two components and then run some end-to-end benchmarks. If we could do the same thing with the encoder, it would be nice.

[ehsteve]

Of course.

[codecov[bot]]

Codecov Report

Merging #54 (855079e) into main (6a8416d) will decrease coverage by 0.25%. The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
- Coverage   95.52%   95.28%   -0.25%     
==========================================
  Files           7        8       +1     
  Lines         604      679      +75     
==========================================
+ Hits          577      647      +70     
- Misses         27       32       +5     

Impacted Files	Coverage Δ
ccsdspy/packet_types.py	94.21% <85.71%> (-1.06%)	:arrow_down:
ccsdspy/encode.py	96.29% <96.29%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ddasilva]

I made some adjustments to the code to make the style consistent with the rest of the project. I moved the encoding logic into functions in _encode_fixed_length() and _encode_variable_length() in ccsdspy/encode.py. This function names mirror the functions in ccsdspy/decode.py. I renamed the save() function to to_file() so it mimics from_file() also. There's a few gaps (like missing docstrings) which I still need to fill out. I hope you don't mind me shuffling around your code.

I think we should make it so you can supply the primary header values as arrays, so you can control them on a per-packet basis. It might make sense to just pull these out of the dictionary you pass using the same keys we return with include_primary_header=True, and default to "reasonable" values when they're not there so one doesn't have to fuss with them for quick tests.

I'll continue on with this; feel free to pop in with comments along the way.

[ehsteve]

@ddasilva any updates? Just wanted to ping on this since it's been a while.

[ddasilva]

Sorry, got very busy real fast. I'm on vacation away from home but I'll look at this when I get back on Monday.

[ddasilva]

@ehsteve do you have an immediate use for this in your work? would be great if we could evaluate/test with a use case

[ehsteve]

We can use it right now by updating many of the tests to use this functionality to create the test packet files.

[ddasilva]

Sounds good, I'll take a look when I get back from vacation

[ddasilva]

@ehsteve I resolved the merge conflicts. Do you want to take a stab at using your code for the test packet generation?

[ehsteve]

Sure. Not sure much time I can devote to this right now but I'll try. Do you want to create a branch so we can work together on it and I can make smaller merge requests? Seems like a dev branch would be good for this development.

[ddasilva]

Sure. Not sure much time I can devote to this right now but I'll try. Do you want to create a branch so we can work together on it and I can make smaller merge requests? Seems like a dev branch would be good for this development.

Sounds good. I made the branch packet_encoding in the official repo (this one). I'm going to close this merge request.