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

Xtce generator #145

Merged

Conversation

maxinelasp
Copy link
Contributor

@maxinelasp maxinelasp commented Sep 11, 2023

Change Summary

Updates to the XTCE generator code to make it more generic, including some API changes.

Overview

Codice needs two events for scientific data from telemetry. Rather than hardcoding these two event mnemonics, I changed the code to accept a dictionary. This makes it easier to overwrite methods for instrument specific changes without needing to update the writer/generator parts.

Making this change also required updating all the codice python files to provide the events through a dictionary instead of assuming them.

Long term, codice could benefit from a specific CodiceTelemetryGenerator class which has codice-specific code or helper functions. I have an example of this in my GLOWS code, which is still incomplete.

I also split up some methods which people may want to override in the future.

Please double check my docstrings, I'm not sure if I accurately represented what the mnemonics are used for.

New Files

  • new file 1
    • description of new file 1's purpose

Updated Files

  • telemetry_generator.py
    • Moved science mnemonics to dictionary
  • codice files
    • updated to match changed api

Testing

I checked, and the output codice file matches the existing one in the repository. So I didn't change any functionality.

@maxinelasp maxinelasp requested review from a team, bourque, sdhoyt, greglucas, tech3371, bryan-harter, laspsandoval and GFMoraga and removed request for a team September 11, 2023 16:36
@maxinelasp maxinelasp self-assigned this Sep 11, 2023
@maxinelasp
Copy link
Contributor Author

Done as part of issue #104

Copy link
Contributor

@GFMoraga GFMoraga left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you for making these changes for more flexible use!

Copy link
Contributor

@tech3371 tech3371 left a comment

Choose a reason for hiding this comment

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

Thank you for making it more generic!

data_type_data,
data_type_event_data,
sci_byte,
data_types,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the doc string of this function as well?

tools/xtce_generation/telemetry_generator.py Outdated Show resolved Hide resolved
Comment on lines 118 to 107
if size == sci_byte:
parameter_type_ref = (
"BYTE" # Use BYTE type for both "Data" and "Event_Data"
)
elif size == self.sci_byte:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is difference between this function input parameter sci_byte and self.sci_byte?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I actually removed the sci_byte input to this function and moved it entirely to the class variable. In the future, hopefully we can move this variable, since it is codice specific (?). In glows, I override this function and don't use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was built for packets with BYTE as a dataType. So, it is optional I think I put it as.

Parameters
----------
output_xml_path: the path for the final xml file
mnemonic_names: a list or single name for mnemonics for extracting data types
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is where we give in names of science data that are specific to each instrument such as SCIENCE_DATA or HIST_DATA field in packet definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly!

parameter_type_ref = data_types[
"Data"
] # use dataType of "Data" in data_types dict
elif data_types["EventData"] and size == self.sci_byte:
Copy link
Contributor

Choose a reason for hiding this comment

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

is "EventData" a CoDICE specific thing? I'm getting an error because I don't have "EventData" defined as a key

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is codice specific. But you can change the mnemonic. I also think Maxine might have fixed this, so when pushed, it should work..... If this helps, I have packets with "Data" OR "Event_Data" so it was my way of calling the correct one depending on what was in the row.

  • data_type_event_data: The data type for the "Event_Data" mnemonic

Copy link
Contributor Author

@maxinelasp maxinelasp Sep 12, 2023

Choose a reason for hiding this comment

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

Ah yes, it is codice specific. In my GLOWs code, I override it:

def create_parameter_types(
        self,
        parameter_type_set,
        unique_lengths,
        data_types,
        sci_byte=None,
    ):
        """
        Create parameter types based on 'dataType' for the unique 'lengthInBits' values.
        This will loop through the unique lengths and create a ParameterType element
        for each length representing a UINT type.

        Parameters:
        - parameter_type_set: The ParameterTypeSet element where parameter types are.
        - unique_lengths: Unique values from the 'lengthInBits' column.
        - data_type_data: The data type for the "Data" mnemonic.
        - data_type_event_data: The data type for the "Event_Data" mnemonic.
        - sci_byte: The BYTE number of lengthInBits in both "Data" and "Event_Data"

        Returns:
        - parameter_type_set: The updated ParameterTypeSet element.
        """
        for size in unique_lengths:
            # All bins equal?
            # Are bins the only thing that matters? -> check glows code for this
            parameter_type_ref = data_types["BIN"]  # Use UINT for other sizes
            print(parameter_type_ref)
            parameter_type = Et.SubElement(parameter_type_set, "xtce:ParameterType")
            parameter_type.attrib["name"] = f"uint{size}"
            parameter_type.attrib["signed"] = "false"

            encoding = Et.SubElement(parameter_type, "xtce:IntegerDataEncoding")
            encoding.attrib["sizeInBits"] = str(size)
            encoding.attrib["encoding"] = "unsigned"

            # Create BinaryParameterType if parameter_type_ref is "BYTE"
            if parameter_type_ref == "BYTE":
                binary_parameter_type = Et.SubElement(
                    parameter_type_set, "xtce:BinaryParameterType"
                )
                binary_parameter_type.attrib["name"] = parameter_type_ref

                Et.SubElement(binary_parameter_type, "xtce:UnitSet")

                binary_data_encoding = Et.SubElement(
                    binary_parameter_type, "xtce:BinaryDataEncoding"
                )
                binary_data_encoding.attrib["bitOrder"] = "mostSignificantBitFirst"

                size_in_bits = Et.SubElement(binary_data_encoding, "xtce:SizeInBits")
                fixed_value = Et.SubElement(size_in_bits, "xtce:FixedValue")
                fixed_value.text = str(sci_byte)  # Set the size in bits to sci_byte

        return parameter_type_set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, in the future, this will be something that is probably overridden in most instruments, or the more generic code above can be modified so it can be used in all cases where there are very simple parameters.

Copy link
Contributor

@sdhoyt sdhoyt left a comment

Choose a reason for hiding this comment

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

Looks good!

Updating telemetry generator based on review comments

Update tools/xtce_generation/telemetry_generator.py

Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com>
@maxinelasp maxinelasp merged commit 096e654 into IMAP-Science-Operations-Center:dev Sep 12, 2023
10 checks passed
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants