search

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

Provides access to the CCSDS primary header #22

Closed ehsteve closed 1 year ago

ehsteve commented 1 year ago

This closes issue #15.

@ddasilva, I'll admit that I am not totally sure why this worked. I had assumed that I would have to modify _decode_fixed_length to let it know NOT to skip the primary header as it usually does but I did not have to do that.

Sorry if my test file is not very elegant. I did not want to make changes to the other test files.

ddasilva commented 1 year ago

I think this breaks if you specify the bit offsets in the body fields. Can you test? If it does break, may be as simple as adding the primary header length to each body field offset.

ddasilva commented 1 year ago

What is your justification for the new section about the CCSDS standard on the home page? It's nice to have somewhere.. but maybe it would be better if we just added a new page with a link on the sidebar called "CCSDS Standard". The home page is already getting a bit long.

ddasilva commented 1 year ago

Also, in the future would it be possible to not include the modifications to whitespace with your commit? It's not a huge deal but it confuses git blame and makes the diffs harder to read.

ehsteve commented 1 year ago

What is your justification for the new section about the CCSDS standard on the home page? It's nice to have somewhere.. but maybe it would be better if we just added a new page with a link on the sidebar called "CCSDS Standard". The home page is already getting a bit long.

Unfortunately the definition of the primary header is buried in the blue book and there isn't a way to link directly to it. Given that it is now a potential output, I thought it best to give some documentation about how to interpret the fields. If you don't like it on the docs home page as it is getting too long, we could move it to another page in the docs. What do you think?

ehsteve commented 1 year ago

Also, in the future would it be possible to not include the modifications to whitespace with your commit? It's not a huge deal but it confuses git blame and makes the diffs harder to read.

The explicit white space fixes were to fix flake8 violations which I saw you were testing or planning on testing in your github actions. Sorry if it makes the PR confusing. It seems like the community is dealing with this issue by using black which I love and it ensures that everyone is using the same standards that way you don't see these style changes in diffs.

ehsteve commented 1 year ago

I think this breaks if you specify the bit offsets in the body fields. Can you test? If it does break, may be as simple as adding the primary header length to each body field offset.

I didn't know you could be explicit about the bit offsets. I will change it and test.

ddasilva commented 1 year ago

What is your justification for the new section about the CCSDS standard on the home page? It's nice to have somewhere.. but maybe it would be better if we just added a new page with a link on the sidebar called "CCSDS Standard". The home page is already getting a bit long.

Unfortunately the definition of the primary header is buried in the blue book and there isn't a way to link directly to it. Given that it is now a potential output, I thought it best to give some documentation about how to interpret the fields. If you don't like it on the docs home page as it is getting too long, we could move it to another page in the docs. What do you think?

That’s what I meant, sorry if I worded my reply poorly. A separate page on the docs would be great

ddasilva commented 1 year ago

The explicit white space fixes were to fix flake8 violations which I saw you were testing or planning on testing in your github actions. Sorry if it makes the PR confusing. It seems like the community is dealing with this issue by using black which I love and it ensures that everyone is using the same standards that way you don't see these style changes in diffs.

This seems reasonable, feel free to proceed with the whitespace changes. Thanks!

ehsteve commented 1 year ago

@ddasilva I included a fix to the github action that I submitted in that other PR. I had the name of the primary branch incorrect so it broke the testing. FYI, using master as the primary branch name has fallen out of favor. The standard is now main.

ehsteve commented 1 year ago

Looks like your numpy version requirement (>=1.20) is not compatible with Python 3.6 so I removed it from the testing suite.

ehsteve commented 1 year ago

@ddasilva i believe i made all of the changes you requested so this PR is ready for you to review again!

ddasilva commented 1 year ago

@ddasilva I included a fix to the github action that I submitted in that other PR. I had the name of the primary branch incorrect so it broke the testing. FYI, using master as the primary branch name has fallen out of favor. The standard is now main.

The primary branch has been renamed

ddasilva commented 1 year ago

Looks good to me, just update the ci file back to main and then will merge

ehsteve commented 1 year ago

Done, sorry, more automatic space fixes from my IDE...