Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 6, 2025

Description

Implements XTCE 5.3.4 IncludeCondition support for conditional parameter parsing based on previously-parsed values.

Implementation

  • New ParameterRefEntry class wraps parameter references with optional IncludeCondition and RepeatEntry (placeholder)

    • Evaluates conditions against partially-parsed packet state using existing MatchCriteria infrastructure
    • Skips parameter parsing when condition evaluates false, leaving bit position unchanged
    • Raises NotImplementedError for RepeatEntry during parse
  • Updated SequenceContainer

    • Creates ParameterRefEntry objects only when IncludeCondition/RepeatEntry present (backward compatible)
    • Stores _parameter_lookup for runtime parameter resolution
    • Warns on unrecognized EntryList element types
    • Updated type hints: entry_list: list[Union[ParameterRefEntry, Parameter, SequenceContainer]]

Example

# XTCE with conditional parameter
<ParameterRefEntry parameterRef="CheckSum">
    <IncludeCondition>
        <Comparison parameterRef="CSFlag" value="1" comparisonOperator="=="/>
    </IncludeCondition>
</ParameterRefEntry>

# CheckSum parsed only when CSFlag == 1
packet = SpacePacket(binary_data=b"\x01\x04\xd2...")  # CSFlag=1
container.parse(packet)
assert "CheckSum" in packet

packet = SpacePacket(binary_data=b"\x00...")  # CSFlag=0
container.parse(packet)
assert "CheckSum" not in packet  # Skipped, no position advance

Limitations

ComparisonList with multiple comparisons evaluates only first element (warns user). Full AND logic requires architectural changes to support multiple MatchCriteria per condition.

Checklist

  • Changes are fully implemented without dangling issues or TODO items
  • Deprecated/superseded code is removed or marked with deprecation warning
  • Current dependencies have been properly specified and old dependencies removed
  • New code/functionality has accompanying tests and any old tests have been updated to match any new assumptions
  • The changelog.md has been updated

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • www.omg.org
    • Triggering command: /usr/bin/python3 /home/REDACTED/.local/bin/pytest --color=yes --cov --cov-report=term-missing (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

This section details on the original issue you should resolve

<issue_title>Support IncludeCondition in ParameterRefEntry elements</issue_title>
<issue_description>## Summary

Space Packet Parser supports much of the XTCE XML schema but the IncludeCondition element within ParameterRefEntry is not yet handled.

XTCE Reference: Section 5.3.4 (p. 235) of CCSDS 660.1-G-2

IncludeCondition elements allow conditional inclusion of a ParameterRefEntry in the EntryList of a SequenceContainer. The condition can only reference previously parsed parameters in the packet. During parsing, the partially-parsed packet object is available to look up these values.

XTCE Example

<xtce:SequenceContainer name="ConditionalExample">
  <xtce:EntryList>
    <xtce:ParameterRefEntry parameterRef="CSFlag"/>
    <xtce:ParameterRefEntry parameterRef="CheckSum">
      <xtce:IncludeCondition>
        <xtce:Comparison value="1" parameterRef="CSFlag"/>
      </xtce:IncludeCondition>
    </xtce:ParameterRefEntry>
  </xtce:EntryList>
</xtce:SequenceContainer>

In this example, CheckSum is only parsed if the previously-parsed CSFlag parameter equals 1.

Implementation

Current Architecture Limitation

Currently in containers.py lines 111-131, we parse SequenceContainer/EntryList elements into a simple entry_list of Parameter and SequenceContainer objects. This doesn't support:

  • Conditional inclusion (via IncludeCondition)
  • Repeated parameters (via RepeatEntry)
  • Other ParameterRefEntry attributes

Create New ParameterRefEntry Class

Create a new class in containers.py to represent the ParameterRefEntry XML element with its optional child elements.

Attributes:

  • parameter_ref: str - Name reference to the Parameter
  • repeat_entry: Optional[RepeatEntry] - Repeat information (raise NotImplementedError for now)
  • include_condition: Optional[MatchCriteria] - Condition for inclusion (use existing MatchCriteria types from comparisons.py)

Inheritance:

  • Must inherit from common.XmlObject and common.Parseable
  • Implement from_xml() to parse the XML structure
  • Implement to_xml() for serialization
  • Implement parse() to handle conditional parsing logic

Parse Logic:

  1. If include_condition exists, evaluate it using the current packet state
  2. If condition is False (or None and parameter should be skipped), return early without parsing
  3. If repeat_entry exists, raise NotImplementedError with message: "RepeatEntry is not currently supported in parsing"
  4. Otherwise, resolve the parameter reference and parse it normally

Scope Constraint (XTCE 5.8.4):
The IncludeCondition can only reference parameters that have already been parsed in the current container (previous entries in the EntryList). References to "future" parameters in the same container must be rejected or produce a warning.

Update SequenceContainer.from_xml()

Modify the loop at lines 111-131 to create ParameterRefEntry objects instead of directly referencing Parameter objects:

elif entry_tag_name == "ParameterRefEntry":
    parameter_name = entry.attrib["parameterRef"]
    
    # Parse optional IncludeCondition
    include_condition = None
    if (include_cond_elem := entry.find("IncludeCondition")) is not None:
        # Use existing MatchCriteria parsing logic from comparisons module
        include_condition = cls._parse_include_condition(include_cond_elem)
    
    # Parse optional RepeatEntry
    repeat_entry = None
    if entry.find("RepeatEntry") is not None:
        repeat_entry = ...  # Parse but will raise NotImplementedError during parse()
    
    # Create ParameterRefEntry object with parameter reference and conditions
    param_ref_entry = ParameterRefEntry(
        parameter_ref=parameter_name,
        include_condition=include_condition,
        repeat_entry=repeat_entry
    )
    entry_list.append(param_ref_entry)

Add Warning for Unrecognized EntryList Elements

At the end of the if-elif chain (after line 131 in current code), add:

else:
    warnings.warn(
        f"Unrecognized entry type '{entry_tag_name}' in EntryList for container "
        f"'{element.attrib['name']}'. Supported types: ParameterRefEntry, ContainerRefEntry. "
        f"Skipping this entry.",
        category=UserWarning
    )
    continue

Change Type of entry_list

The entry_list attribute in SequenceContainer should be updated to:

entry_list: list[Union[ParameterRefEntry, SequenceContainer]]

This replaces the current list[Union[parameters.Parameter, SequenceContainer]].

Backward Compatibility: The SequenceContainer.parse() method must be updated to handle ParameterRefEntry objects, which will need to resolve their parameter references and evaluate conditions during parsing.

Test Cases

  1. Simple condition: Parameter included only when flag=1, exclud...

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Support IncludeCondition in ParameterRefEntry elements Support IncludeCondition in ParameterRefEntry elements Nov 6, 2025
Copilot AI requested a review from medley56 November 6, 2025 02:40
@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 96.72619% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.11%. Comparing base (4dec594) to head (f015995).

Files with missing lines Patch % Lines
space_packet_parser/xtce/containers.py 82.81% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #216      +/-   ##
==========================================
+ Coverage   93.87%   94.11%   +0.23%     
==========================================
  Files          46       48       +2     
  Lines        3594     3924     +330     
==========================================
+ Hits         3374     3693     +319     
- Misses        220      231      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@medley56
Copy link
Member

medley56 commented Nov 6, 2025

pre-commit.ci autofix

@medley56 medley56 requested a review from greglucas November 6, 2025 02:55
@medley56 medley56 marked this pull request as ready for review November 6, 2025 02:55
Copilot AI review requested due to automatic review settings November 6, 2025 02:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements support for IncludeCondition in ParameterRefEntry elements within XTCE sequence containers. This allows parameters to be conditionally included in packet parsing based on the values of previously parsed parameters.

  • Adds a new ParameterRefEntry class to handle conditional parameter inclusion
  • Updates SequenceContainer to support ParameterRefEntry objects in addition to direct Parameter references
  • Implements comprehensive unit and integration tests covering various conditional parsing scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
space_packet_parser/xtce/containers.py Adds ParameterRefEntry class for conditional parameter parsing; updates SequenceContainer to handle ParameterRefEntry objects and store parameter lookup for runtime resolution
tests/unit/test_xtce/test_include_condition.py Comprehensive unit tests for ParameterRefEntry parsing, XML serialization/deserialization, and conditional logic with various operators
tests/integration/test_include_condition_integration.py Integration tests validating end-to-end parsing with XTCE definitions containing conditional parameters
tests/test_data/test_include_condition.xml Test XTCE document defining a packet structure with conditional parameters for integration testing

Comment on lines 213 to 214
# Store parameter lookup for use during parsing
_parameter_lookup: Optional[dict[str, parameters.Parameter]] = field(default=None, repr=False, compare=False)
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment 'Store parameter lookup for use during parsing' should be placed inside the docstring for the field or as part of the class docstring explaining the purpose of _parameter_lookup. Inline comments for dataclass fields are better placed in the class-level docstring or as a docstring for the field itself.

Copilot uses AI. Check for mistakes.
Comment on lines +116 to +121
# LIMITATION: MatchCriteria interface expects a single object, but ComparisonList
# requires AND logic across multiple comparisons. For now, we only support single
# comparisons in ComparisonList. Full AND logic would require a new MatchCriteria
# subclass or modifying include_condition to accept a list of MatchCriteria.
# This follows the same limitation pattern used in restriction_criteria where
# multiple MatchCriteria are stored in a list on the SequenceContainer itself.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment describes a limitation but doesn't clearly state what the expected behavior is according to the XTCE spec. The comment should explicitly state that according to the XTCE specification, ComparisonList should have AND semantics across all comparisons, and link to the relevant section if possible.

Copilot uses AI. Check for mistakes.
warnings.warn(
f"ComparisonList with {len(comparisons_list)} comparisons in IncludeCondition "
f"for parameter '{parameter_ref}'. Only the first comparison will be evaluated. "
f"Full AND logic for ComparisonList in IncludeCondition requires architectural "
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The warning message should indicate the potential consequences of this limitation more clearly. Consider adding that this may result in incorrect packet parsing if the XTCE definition relies on multiple comparisons in the ComparisonList.

Suggested change
f"Full AND logic for ComparisonList in IncludeCondition requires architectural "
f"This may result in incorrect packet parsing if the XTCE definition relies on multiple comparisons "
f"being evaluated with AND logic. Full AND logic for ComparisonList in IncludeCondition requires architectural "

Copilot uses AI. Check for mistakes.
# Since we don't fully parse it, we can't fully serialize it either
warnings.warn(
"RepeatEntry serialization is not fully implemented. "
"The RepeatEntry element will not be included in the XML output.",
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The warning is issued during serialization but doesn't indicate whether this could cause issues when the XML is read back. Consider adding information about whether round-tripping (parse XML -> serialize -> parse again) will preserve the RepeatEntry information or if it will be lost.

Suggested change
"The RepeatEntry element will not be included in the XML output.",
"The RepeatEntry element will not be included in the XML output. "
"If you parse XML containing RepeatEntry and then serialize it again, the RepeatEntry information will be lost.",

Copilot uses AI. Check for mistakes.
Copilot AI and others added 6 commits November 24, 2025 21:27
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
Co-authored-by: medley56 <7018964+medley56@users.noreply.github.com>
@medley56 medley56 force-pushed the copilot/support-include-condition branch from f015995 to 4cac7d7 Compare November 24, 2025 21:29
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.

Support IncludeCondition in ParameterRefEntry elements

2 participants