Repository: medley56/space_packet_parser

Issue:
Falsy Raw Values Hidden by Label Value for Enumerated Parameter Types
# Context

Enumerated parameter types are returning the label value as the raw value if the raw value is falsy. This means users cannot access the true raw value of an enum (usually an int). The problem is in the _Parameter mixin class for returned data items within a packet:

```python
    def __new__(cls, value: BuiltinDataTypes, raw_value: BuiltinDataTypes = None) -> BuiltinDataTypes:
        obj = super().__new__(cls, value)
        # Default to the same value as the parsed value if it isn't provided
        obj.raw_value = raw_value or value  <-- HERE
        return obj
```

# Implementation Plan

Immediate fix:
Change the check in _Parameter to be 
```python
obj.raw_value = raw_value if raw_value is not None else value
```

This should be safe because there is no case where a data encoding parser should return None and actually mean None. There is no way to encode a None value using the simple data types available in XTCE. The key is that we want a true None to be the only possible way to instantiate a _Parameter where raw_value is the same as the main value.

Discussion:

Solution patch:
diff --git a/CITATION.cff b/CITATION.cff
index 95062e7..d643971 100644
--- a/CITATION.cff
+++ b/CITATION.cff
@@ -1,7 +1,7 @@
 cff-version: 1.2.0
 title: 'space_packet_parser'
 type: software
-version: '5.0.0'
+version: '5.0.1'
 description: A CCSDS telemetry packet decoding library based on the XTCE packet format description standard.
 license: BSD-3-Clause
 abstract: The Space Packet Parser Python library is a generalized, configurable packet decoding library for CCSDS telemetry
diff --git a/docs/source/changelog.md b/docs/source/changelog.md
index 1f63def..0a84726 100644
--- a/docs/source/changelog.md
+++ b/docs/source/changelog.md
@@ -5,6 +5,9 @@ list and release milestones.
 ## Version Release Notes
 Release notes for the `space_packet_parser` library
 
+### v5.0.1 (released)
+- BUGFIX: Allow raw_value representation for enums with falsy raw values. Previously these defaulted to the enum label.
+
 ### v5.0.0 (released)
 - *BREAKING*: Main API changed. No need to create separate definition and parser objects any more. Create only a 
   definition from your XTCE document and instead of `my_parser.generator`, use `my_packet_definition.packet_generator`.
diff --git a/pyproject.toml b/pyproject.toml
index 77bdc85..d29342e 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "space_packet_parser"
-version = "5.0.0"
+version = "5.0.1"
 description = "A CCSDS telemetry packet decoding library based on the XTCE packet format description standard."
 license = "BSD-3-Clause"
 readme = "README.md"
diff --git a/space_packet_parser/packets.py b/space_packet_parser/packets.py
index 6640772..4c806ed 100644
--- a/space_packet_parser/packets.py
+++ b/space_packet_parser/packets.py
@@ -24,7 +24,7 @@ class _Parameter:
     def __new__(cls, value: BuiltinDataTypes, raw_value: BuiltinDataTypes = None) -> BuiltinDataTypes:
         obj = super().__new__(cls, value)
         # Default to the same value as the parsed value if it isn't provided
-        obj.raw_value = raw_value or value
+        obj.raw_value = raw_value if raw_value is not None else value
         return obj
 
 
diff --git a/space_packet_parser/parameters.py b/space_packet_parser/parameters.py
index 6e06e0c..5c0e619 100644
--- a/space_packet_parser/parameters.py
+++ b/space_packet_parser/parameters.py
@@ -1,12 +1,12 @@
-
 """ParameterType definitions"""
+# Standard
 from abc import ABCMeta
 from dataclasses import dataclass
 from typing import Optional, Union
 import warnings
-
+# Installed
 import lxml.etree as ElementTree
-
+# Local
 from space_packet_parser import calibrators, comparisons, encodings, packets
 
 
@@ -119,7 +119,7 @@ def get_data_encoding(parameter_type_element: ElementTree.Element, ns: dict) ->
         raise ValueError(f"No Data Encoding element found for Parameter Type "
                          f"{parameter_type_element.tag}: {parameter_type_element.attrib}")
 
-    def parse_value(self, packet: packets.CCSDSPacket, **kwargs):
+    def parse_value(self, packet: packets.CCSDSPacket, **kwargs) -> packets.ParameterDataTypes:
         """Using the parameter type definition and associated data encoding, parse a value from a bit stream starting
         at the current cursor position.
 
@@ -131,8 +131,8 @@ def parse_value(self, packet: packets.CCSDSPacket, **kwargs):
 
         Returns
         -------
-        parsed_value : any
-            Resulting parsed data value.
+        parsed_value : packets.ParameterDataTypes
+            Resulting parsed parameter value
         """
         return self.encoding.parse_value(packet, **kwargs)
 
@@ -240,7 +240,7 @@ def get_enumeration_list_contents(element: ElementTree.Element, ns: dict) -> dic
             for el in enumeration_list.iterfind('xtce:Enumeration', ns)
         }
 
-    def parse_value(self, packet: packets.CCSDSPacket, **kwargs):
+    def parse_value(self, packet: packets.CCSDSPacket, **kwargs) -> packets.StrParameter:
         """Using the parameter type definition and associated data encoding, parse a value from a bit stream starting
         at the current cursor position.
 
@@ -252,17 +252,21 @@ def parse_value(self, packet: packets.CCSDSPacket, **kwargs):
 
         Returns
         -------
-        derived_value : StrParameter
+        derived_value : packets.StrParameter
             Resulting enum label associated with the (usually integer-)encoded data value.
         """
-        value = super().parse_value(packet, **kwargs)
+        raw_enum_value = super().parse_value(packet, **kwargs).raw_value
         # Note: The enum lookup only operates on raw values. This is specified in 4.3.2.4.3.6 of the XTCE spec "
         # CCSDS 660.1-G-2
+        # Note, this doesn't prohibit a user from defining a calibrator on an encoding that is used for an enum lookup.
+        # It just means that the calibrated derived value doesn't get used for the lookup, nor will the calibrated
+        # value be represented in the returned as part of the returned enum (string) parameter
         try:
-            label = self.enumeration[value]
+            label = self.enumeration[raw_enum_value]
         except KeyError as exc:
-            raise ValueError(f"Failed to find the value {value} in enum lookup list {self.enumeration}.") from exc
-        return packets.StrParameter(label, value)
+            raise ValueError(f"Failed to find the value {raw_enum_value} in "
+                             f"enum lookup list {self.enumeration}.") from exc
+        return packets.StrParameter(label, raw_enum_value)
 
 
 class BinaryParameterType(ParameterType):
@@ -307,17 +311,20 @@ def parse_value(self, packet: packets.CCSDSPacket, **kwargs):
             Binary representation of the packet used to get the coming bits and any
             previously parsed data items to infer field lengths.
 
-
         Returns
         -------
         derived_value : BoolParameter
             Resulting boolean representation of the encoded raw value
         """
-        # NOTE: There is an intermediate value here that we are potentially using. The flow is:
-        #       raw data -> parsed/processed (calibrated) value -> boolean
-        parsed_value = super().parse_value(packet, **kwargs)
-        # NOTE: This behaves very strangely for String and Binary data encodings.
-        # Don't use those for Boolean parameters. The behavior isn't specified well in XTCE.
+        # NOTE: The XTCE spec states that Booleans are "a restricted form of
+        # enumeration." Enumerated parameters are only permitted to perform lookups based on raw encoded values
+        # (not calibrated ones). We force this by taking the bool of the raw form of the parsed parameter.
+        parsed_value = super().parse_value(packet, **kwargs).raw_value
+        # NOTE: Boolean parameters may behave unexpectedly when encoded as String and Binary values.
+        # This is because it's not obvious nor specified in XTCE which values of
+        # binary encoded or string encoded data should be truthy/falsy.
+        # This implementation defaults to Python's interpretation of True/False for the (raw) parsed value,
+        # so non-empty byte strings (the representation for binary and string encoded data) will always be True.
         return packets.BoolParameter(bool(parsed_value), parsed_value)
 
 


Test patch:
diff --git a/tests/unit/test_xtcedef.py b/tests/unit/test_xtcedef.py
index cf14a99..271f395 100644
--- a/tests/unit/test_xtcedef.py
+++ b/tests/unit/test_xtcedef.py
@@ -1741,25 +1741,47 @@ def test_enumerated_parameter_type(xml_string: str, expectation):
 
 
 @pytest.mark.parametrize(
-    ('parameter_type', 'raw_data', 'expected'),
+    ('parameter_type', 'raw_data', 'expected_raw', 'expected'),
     [
         (parameters.EnumeratedParameterType(
             'TEST_ENUM',
             encodings.IntegerDataEncoding(16, 'unsigned'), {32768: 'NOMINAL'}),
          0b1000000000000000.to_bytes(length=2, byteorder='big'),
+         32768,
          'NOMINAL'),
         (parameters.EnumeratedParameterType(
-            'TEST_FLOAT',
+            'TEST_FALSY_RAW_ENUM',
+            encodings.IntegerDataEncoding(16, 'unsigned'), {0: 'FALSY_LABEL'}),
+         0b0000000000000000.to_bytes(length=2, byteorder='big'),
+         0,
+         'FALSY_LABEL'),
+        # Test to prove that enums are never using calibrated values
+        (parameters.EnumeratedParameterType(
+            'TEST_CALIBRATED_ENCODING_ENUM',
+            encodings.IntegerDataEncoding(
+                16, 'unsigned',
+                default_calibrator=calibrators.PolynomialCalibrator([
+                    calibrators.PolynomialCoefficient(5, 0),  # 5
+                    calibrators.PolynomialCoefficient(2, 1)  # 2x
+                ])
+            ), {0: 'USES_UNCALIBRATED_VALUE'},),
+         0b0000000000000000.to_bytes(length=2, byteorder='big'),
+         0,
+         'USES_UNCALIBRATED_VALUE'),
+        (parameters.EnumeratedParameterType(
+            'TEST_FLOAT_ENUM',
             encodings.IntegerDataEncoding(16, 'signed'), {-42: 'VAL_LOW'}),
          0b1111111111010110.to_bytes(length=2, byteorder='big'),
+         -42,
          'VAL_LOW'),
     ]
 )
-def test_enumerated_parameter_parsing(parameter_type, raw_data, expected):
+def test_enumerated_parameter_parsing(parameter_type, raw_data, expected_raw, expected):
     """"Test parsing enumerated parameters"""
     packet = packets.CCSDSPacket(raw_data=raw_data)
     value = parameter_type.parse_value(packet)
     assert value == expected
+    assert value.raw_value == expected_raw
 
 
 @pytest.mark.parametrize(
@@ -1950,7 +1972,7 @@ def test_boolean_parameter_type(xml_string, expectation):
             encodings.StringDataEncoding(fixed_raw_length=120, encoding="UTF-8")),
          b'false_is_truthyextradata',
          0,
-         'false_is_truthy', True),
+         b'false_is_truthy', True),
         (parameters.BooleanParameterType(
             'TEST_BOOL',
             encodings.IntegerDataEncoding(size_in_bits=2, encoding="unsigned")),