Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed xtce typo and updated codice xtce files #185

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Fixed xtce typo and updated codice xtce files #185

merged 4 commits into from
Oct 5, 2023

Conversation

GFMoraga
Copy link
Contributor

@GFMoraga GFMoraga commented Oct 4, 2023

Change Summary

Fixed a typo in the ccsds_header_xtce_generator.py, ccsds-header.xml and re-ran xtce_generator_codice.py

Overview

This is a simple typo fix

Updated Files

  • ccsds_header_xtce_generator.py

    • changed SEG_FLGS to SEQ_FLGS
  • ccsds-header.xml

    • changed SEG_FLGS to SEQ_FLGS
  • codice xtce files

    • re-ran xtce_generator_codice.py to update all xtce files to fix typo

@GFMoraga GFMoraga added the Level: L0 Level 0 processing label Oct 4, 2023
@GFMoraga GFMoraga self-assigned this Oct 4, 2023
@GFMoraga GFMoraga requested a review from maxinelasp October 4, 2023 16:23
Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about the BinaryParameterType -> IntegerParameterType change, is that intentional? I updated telemetery generator to create the BinaryParameterType because I think that's the correct thing, and I need it that way in the GLOWS XTCE. Is that a conflict between the codice requirements?

@@ -39,14 +39,9 @@
<xtce:IntegerParameterType name="UINT32" signed="false">
<xtce:IntegerDataEncoding sizeInBits="32" encoding="unsigned" />
</xtce:IntegerParameterType>
<xtce:BinaryParameterType name="BYTE4325376">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change? telemetry generator should do this BinaryParameterType, and not the integer type which it changed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm...I can look into it, I didn't touch anything except the typo issue. The def create_parameter_types didn't change, so I honestly do not know currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! So.. @maxinelasp @greglucas the code is correct on my local, but I figured out thet Maxine added or "INT" when she updated the telemetry_generator.py in line 118.
I simply took this away, and codice returned to BinaryParameterType like before. Solutions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to leave it with Maxines current update. Since this is only codice related right now..we will handle this error when we run into it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you working from the TLM_COD.xlsx spreadsheet that is in the repo? I see BYTE in the dataType field under the P_COD_HI_INSTRUMENT_COUNTERS tab, so I think that should still fall down to BinaryParameterType.

p.s. if you click on the line number, then expand the three dots you can copy permalink to get a reference to the code spot you're talking about.
image

And view the git blame directly in the UI if that is helpful as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I see now that I look closer at the code.

"SINT" or "INT" in parameter_type_ref_name

will evaluate to "SINT" if "INT" not in parameter_type_ref_name. Which then in turn evaluates to True, and thus we never hit the BYTE path.

I think it might need to be something more like this:

any(x in parameter_type_ref_name for x in ["SINT", "INT"])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can fix it in this PR, but would need guidance. I kinda see what you mean here Greg, but I also haven't built many any statements with for loops.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait! I think I got it!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it might just be easier to change the order or lookup around. What is parameter_type_ref_name is it just a string, or is it a tuple/set/list?

A few options for you:

elif "INT" in parameter_type_ref_name:

or

elif parameter_type_ref_name in ["SINT", "INT"]:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made a new commit! It seemed to fix this issue.

Copy link
Collaborator

@bourque bourque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this quick fix! This will help me on #184

Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @maxinelasp about the BinaryParameterType change and removing the unused file.

I am guessing you may have run this with an older version of the file you had around somewhere, or haven't rebased this on upstream/dev so those changes aren't in your local file.

@GFMoraga GFMoraga requested a review from greglucas October 4, 2023 19:54
@GFMoraga
Copy link
Contributor Author

GFMoraga commented Oct 5, 2023

Ive never used codecov. How do I fix this check?

codecov/patch — 94.24% of diff hit (target 94.66%)
codecov/project — 94.33% (-0.33%) compared to 690038f

@GFMoraga GFMoraga dismissed greglucas’s stale review October 5, 2023 16:53

I figured out a solution and took comments seriously

Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for investigating the binary type issue

@GFMoraga GFMoraga merged commit b0d52a5 into IMAP-Science-Operations-Center:dev Oct 5, 2023
@GFMoraga GFMoraga deleted the fix-typo branch October 5, 2023 16:55
@GFMoraga
Copy link
Contributor Author

GFMoraga commented Oct 5, 2023

I tried to look into codecov..but I couldn't figure it out..so I merged because I was at a 94.24%. Maybe @greglucas you can help me understand this for future PRs?

bourque added a commit to bourque/imap_processing that referenced this pull request Oct 10, 2023
laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Nov 15, 2023
…s-Center#185)

fixed typo in xtce, fixed SHCOURSE, fixed BinaryParameter, and updated codice xtce files
laspsandoval pushed a commit to laspsandoval/imap_processing that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: L0 Level 0 processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants