refactor the length key code

now, instead of using hacks with ODXLINK IDs, proper references to
`LengthKeyParameter` are used by `ParamLengthInfoType`. This
necessitates to include the diag coded types in the reference
resolution machinery because `ParamLengthInfoType` is a diag coded
type and it needs to resolve the reference to the length key
parameter.

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Gerrit Ecke <[email protected]>

odxtools/dataobjectproperty.py

@@ -166,19 +166,25 @@ def from_et(et_element, doc_frags: List[OdxDocFragment]) -> "DataObjectProperty"
         return dop
 
     def _build_odxlinks(self) -> Dict[OdxLinkId, Any]:
-        return super()._build_odxlinks()
+        result = super()._build_odxlinks()
+        result.update(self.diag_coded_type._build_odxlinks())
+        return result
 
     def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase):
         """Resolves the reference to the unit"""
         super()._resolve_odxlinks(odxlinks)
 
+        self.diag_coded_type._resolve_odxlinks(odxlinks)
+
         self._unit: Optional[Unit] = None
         if self.unit_ref:
             self._unit = odxlinks.resolve(self.unit_ref, Unit)
 
     def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:
         super()._resolve_snrefs(diag_layer)
 
+        self.diag_coded_type._resolve_snrefs(diag_layer)
+
     @property
     def unit(self) -> Optional[Unit]:
         return self._unit

odxtools/diagcodedtypes.py

@@ -2,17 +2,21 @@
 # Copyright (c) 2022 MBition GmbH
 import abc
 import math
-from typing import Any, List, Optional, Union
+from typing import TYPE_CHECKING, Any, List, Optional, Union
 
 import bitstruct
 
 from .decodestate import DecodeState
 from .encodestate import EncodeState
 from .exceptions import DecodeError, EncodeError
 from .globals import logger, xsi
-from .odxlink import OdxDocFragment, OdxLinkId
+from .odxlink import OdxDocFragment, OdxLinkDatabase, OdxLinkId, OdxLinkRef
 from .odxtypes import DataType, odxstr_to_bool
 
+if TYPE_CHECKING:
+    from .diaglayer import DiagLayer
+    from .parameters.lengthkeyparameter import LengthKeyParameter
+
 ODX_TYPE_TO_FORMAT_LETTER = {
     DataType.A_INT32: "s",
     DataType.A_UINT32: "u",
@@ -40,6 +44,17 @@ def __init__(
         self.base_type_encoding = base_type_encoding
         self.is_highlow_byte_order_raw = is_highlow_byte_order_raw
 
+    def _build_odxlinks(self):
+        return {}
+
+    def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None:
+        """Recursively resolve any odxlinks references"""
+        pass
+
+    def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:
+        """Recursively resolve any short-name references"""
+        pass
+
     @property
     def is_highlow_byte_order(self) -> bool:
         return self.is_highlow_byte_order_raw in [None, True]
@@ -468,7 +483,7 @@ def __init__(
         self,
         *,
         base_data_type: Union[str, DataType],
-        length_key_id: OdxLinkId,
+        length_key_ref: OdxLinkRef,
         base_type_encoding: Optional[str],
         is_highlow_byte_order_raw: Optional[bool],
     ):
@@ -478,11 +493,28 @@ def __init__(
             base_type_encoding=base_type_encoding,
             is_highlow_byte_order_raw=is_highlow_byte_order_raw,
         )
-        self.length_key_id = length_key_id
+        self.length_key_ref = length_key_ref
+
+    def _build_odxlinks(self):
+        return super()._build_odxlinks()
+
+    def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None:
+        """Recursively resolve any odxlinks references"""
+        super()._resolve_odxlinks(odxlinks)
+
+        self._length_key = odxlinks.resolve(self.length_key_ref)
+
+    def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:
+        """Recursively resolve any short-name references"""
+        super()._resolve_snrefs(diag_layer)
+
+    @property
+    def length_key(self) -> "LengthKeyParameter":
+        return self._length_key
 
     def convert_internal_to_bytes(self, internal_value, encode_state: EncodeState,
                                   bit_position: int) -> bytes:
-        bit_length = encode_state.length_keys.get(self.length_key_id, None)
+        bit_length = encode_state.length_keys.get(self.length_key.odx_id, None)
 
         if bit_length is None:
             if self.base_data_type in [
@@ -502,7 +534,7 @@ def convert_internal_to_bytes(self, internal_value, encode_state: EncodeState,
                 bit_length = math.ceil(bit_length / 8.0) * 8
 
         assert bit_length is not None
-        encode_state.length_keys[self.length_key_id] = bit_length
+        encode_state.length_keys[self.length_key.odx_id] = bit_length
 
         return self._to_bytes(
             internal_value,
@@ -519,14 +551,14 @@ def convert_bytes_to_internal(self, decode_state: DecodeState, bit_position: int
             # if isinstance(param_value.parameter, LengthKeyParameter) would be prettier,
             # but leads to cyclic import...
             if (parameter.parameter_type == "LENGTH-KEY" and
-                    parameter.odx_id == self.length_key_id  # type: ignore
+                    parameter.odx_id == self.length_key.odx_id  # type: ignore
                ):
                 # The bit length of the parameter to be extracted is given by the length key.
                 assert isinstance(value, int)
                 bit_length = value
                 break
 
-        assert bit_length is not None, f"Did not find any length key with ID {self.length_key_id}"
+        assert bit_length is not None, f"Did not find any length key with ID {self.length_key.odx_id}"
 
         # Extract the internal value and return.
         return self._extract_internal(
@@ -539,7 +571,7 @@ def convert_bytes_to_internal(self, decode_state: DecodeState, bit_position: int
         )
 
     def __repr__(self) -> str:
-        repr_str = f"ParamLengthInfoType(base_data_type='{self.base_data_type}', length_key_id={self.length_key_id}"
+        repr_str = f"ParamLengthInfoType(base_data_type='{self.base_data_type}', length_key={self.length_key.short_name}"
         if self.base_type_encoding is not None:
             repr_str += f", base_type_encoding={self.base_type_encoding}"
         if not self.is_highlow_byte_order:
@@ -654,19 +686,11 @@ def create_any_diag_coded_type_from_et(et_element, doc_frags: List[OdxDocFragmen
             is_highlow_byte_order_raw=is_highlow_byte_order_raw,
         )
     elif dct_type == "PARAM-LENGTH-INFO-TYPE":
-        # TODO: This is a bit hacky: we make an ID where the data
-        # specifies a reference. The reason is that we need to store
-        # the result in an associative array which is not possible
-        # with references. Note that we currently ignore the DOCREF
-        # attribute of the reference, so if there are any ID conflicts
-        # between document fragments, the encoding process will go
-        # wrong...
-        length_key_elem = et_element.find("LENGTH-KEY-REF")
-        length_key_id = OdxLinkId(length_key_elem.attrib["ID-REF"], doc_frags)
+        length_key_ref = OdxLinkRef.from_et(et_element.find("LENGTH-KEY-REF"), doc_frags)
 
         return ParamLengthInfoType(
             base_data_type=base_data_type,
-            length_key_id=length_key_id,
+            length_key_ref=length_key_ref,
             base_type_encoding=base_type_encoding,
             is_highlow_byte_order_raw=is_highlow_byte_order_raw,
         )

odxtools/parameters/codedconstparameter.py

@@ -1,29 +1,42 @@
 # SPDX-License-Identifier: MIT
 # Copyright (c) 2022 MBition GmbH
 import warnings
-from typing import ByteString, Union
+from typing import TYPE_CHECKING, Any, ByteString, Dict, List, Union
 
 from ..decodestate import DecodeState
 from ..diagcodedtypes import DiagCodedType
 from ..encodestate import EncodeState
 from ..exceptions import DecodeError
+from ..odxlink import OdxLinkDatabase, OdxLinkId
 from ..odxtypes import DataType
 from .parameterbase import Parameter
 
+if TYPE_CHECKING:
+    from ..diaglayer import DiagLayer
+
 
 class CodedConstParameter(Parameter):
 
     def __init__(self, *, diag_coded_type: DiagCodedType, coded_value: Union[int, ByteString],
                  **kwargs):
         super().__init__(parameter_type="CODED-CONST", **kwargs)
 
-        self._diag_coded_type = diag_coded_type
+        self.diag_coded_type = diag_coded_type
         assert isinstance(coded_value, (int, bytes, bytearray))
         self.coded_value = coded_value
 
-    @property
-    def diag_coded_type(self):
-        return self._diag_coded_type
+    def _build_odxlinks(self) -> Dict[OdxLinkId, Any]:
+        result = super()._build_odxlinks()
+
+        result.update(self.diag_coded_type._build_odxlinks())
+
+        return result
+
+    def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None:
+        super()._resolve_odxlinks(odxlinks)
+
+    def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:
+        super()._resolve_snrefs(diag_layer)
 
     @property
     def bit_length(self):

odxtools/parameters/lengthkeyparameter.py

@@ -1,8 +1,14 @@
 # SPDX-License-Identifier: MIT
 # Copyright (c) 2022 MBition GmbH
+from typing import TYPE_CHECKING, Any, Dict, Tuple
+
 from ..encodestate import EncodeState
+from ..odxlink import OdxLinkDatabase, OdxLinkId, OdxLinkRef
 from .parameterwithdop import ParameterWithDOP
 
+if TYPE_CHECKING:
+    from diaglayer import DiagLayer
+
 
 class LengthKeyParameter(ParameterWithDOP):
     """Length Keys specify the bit (!) length of another parameter.
@@ -18,6 +24,19 @@ def __init__(self, *, odx_id, **kwargs):
         super().__init__(parameter_type="LENGTH-KEY", **kwargs)
         self.odx_id = odx_id
 
+    def _build_odxlinks(self) -> Dict[OdxLinkId, Any]:
+        result = super()._build_odxlinks()
+
+        result[self.odx_id] = self
+
+        return result
+
+    def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None:
+        super()._resolve_odxlinks(odxlinks)
+
+    def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:
+        super()._resolve_snrefs(diag_layer)
+
     def is_required(self):
         return False
 

odxtools/parameters/nrcconstparameter.py

@@ -1,15 +1,19 @@
 # SPDX-License-Identifier: MIT
 # Copyright (c) 2022 MBition GmbH
 import warnings
-from typing import List
+from typing import TYPE_CHECKING, Any, Dict, List
 
 from ..decodestate import DecodeState
 from ..diagcodedtypes import DiagCodedType
 from ..encodestate import EncodeState
 from ..exceptions import DecodeError, EncodeError
+from ..odxlink import OdxLinkDatabase, OdxLinkId
 from ..odxtypes import DataType
 from .parameterbase import Parameter
 
+if TYPE_CHECKING:
+    from ..diaglayer import DiagLayer
+
 
 class NrcConstParameter(Parameter):
     """A param of type NRC-CONST defines a set of values to be matched.
@@ -24,14 +28,23 @@ class NrcConstParameter(Parameter):
     def __init__(self, *, diag_coded_type: DiagCodedType, coded_values: List[int], **kwargs):
         super().__init__(parameter_type="NRC-CONST", **kwargs)
 
-        self._diag_coded_type = diag_coded_type
+        self.diag_coded_type = diag_coded_type
         # TODO: Does it have to be an integer or is that just common practice?
         assert all(isinstance(coded_value, int) for coded_value in coded_values)
         self.coded_values = coded_values
 
-    @property
-    def diag_coded_type(self):
-        return self._diag_coded_type
+    def _build_odxlinks(self) -> Dict[OdxLinkId, Any]:
+        result = super()._build_odxlinks()
+
+        result.update(self.diag_coded_type._build_odxlinks())
+
+        return result
+
+    def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None:
+        super()._resolve_odxlinks(odxlinks)
+
+    def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:
+        super()._resolve_snrefs(diag_layer)
 
     @property
     def bit_length(self):

tests/test_diag_coded_types.py

@@ -333,31 +333,31 @@ class TestParamLengthInfoType(unittest.TestCase):
 
     def test_decode_param_info_length_type_uint(self):
         length_key_id = OdxLinkId("param.length_key", doc_frags)
+        length_key_ref = OdxLinkRef.from_id(length_key_id)
+        length_key = LengthKeyParameter(
+            odx_id=length_key_id,
+            short_name="length_key",
+            long_name=None,
+            description=None,
+            semantic=None,
+            sdgs=[],
+            dop_ref=OdxLinkRef("DOP.uint8", doc_frags),
+            dop_snref=None,
+            byte_position=1,
+            bit_position=None,
+        )
         dct = ParamLengthInfoType(
             base_data_type="A_UINT32",
             base_type_encoding=None,
-            length_key_id=length_key_id,
+            length_key_ref=length_key_ref,
             is_highlow_byte_order_raw=None,
         )
+        odxlinks = OdxLinkDatabase()
+        odxlinks.update({length_key_id: length_key})
+        dct._resolve_odxlinks(odxlinks)
         state = DecodeState(
             bytes([0x10, 0x12, 0x34, 0x56]),
-            [
-                ParameterValuePair(
-                    parameter=LengthKeyParameter(
-                        short_name="length_key",
-                        long_name=None,
-                        description=None,
-                        semantic=None,
-                        odx_id=length_key_id,
-                        dop_ref=OdxLinkRef("some_dop", doc_frags),
-                        dop_snref=None,
-                        byte_position=None,
-                        bit_position=None,
-                        sdgs=[],
-                    ),
-                    value=16,
-                )
-            ],
+            [ParameterValuePair(parameter=length_key, value=16)],
             next_byte_position=1,
         )
         internal, next_byte = dct.convert_bytes_to_internal(state, bit_position=0)
@@ -366,12 +366,28 @@ def test_decode_param_info_length_type_uint(self):
 
     def test_encode_param_info_length_type_uint(self):
         length_key_id = OdxLinkId("param.length_key", doc_frags)
+        length_key = LengthKeyParameter(
+            odx_id=length_key_id,
+            short_name="length_key",
+            long_name=None,
+            description=None,
+            semantic=None,
+            sdgs=[],
+            dop_ref=OdxLinkRef("DOP.uint8", doc_frags),
+            dop_snref=None,
+            byte_position=1,
+            bit_position=None,
+        )
+        length_key_ref = OdxLinkRef.from_id(length_key_id)
         dct = ParamLengthInfoType(
             base_data_type="A_UINT32",
             base_type_encoding=None,
-            length_key_id=length_key_id,
+            length_key_ref=OdxLinkRef.from_id(length_key_id),
             is_highlow_byte_order_raw=None,
         )
+        odxlinks = OdxLinkDatabase()
+        odxlinks.update({length_key_id: length_key})
+        dct._resolve_odxlinks(odxlinks)
         state = EncodeState(bytes([0x10]), {}, length_keys={length_key_id: 40})
         byte_val = dct.convert_internal_to_bytes(0x12345, state, bit_position=0)
         self.assertEqual(byte_val, bytes([0x0, 0x0, 0x1, 0x23, 0x45]))
@@ -392,8 +408,7 @@ def test_end_to_end(self):
                 ParamLengthInfoType(
                     base_data_type="A_UINT32",
                     base_type_encoding=None,
-                    length_key_id=OdxLinkId(
-                        "BV.dummy_DL.RQ.sendCertificate.lengthOfCertificateClient", doc_frags),
+                    length_key_ref=OdxLinkRef("param.dummy_length_key", doc_frags),
                     is_highlow_byte_order_raw=None,
                 ),
         }
@@ -470,8 +485,7 @@ def test_end_to_end(self):
                     description="Length parameter for certificateClient.",
                     semantic=None,
                     # LengthKeyParams have an ID to be referenced by a ParamLengthInfoType (which is a diag coded type)
-                    odx_id=diagcodedtypes["length_key_id_to_lengthOfCertificateClient"]
-                    .length_key_id,
+                    odx_id=OdxLinkId("param.dummy_length_key", doc_frags),
                     byte_position=1,
                     bit_position=None,
                     # The DOP multiplies the coded value by 8, since the length key ref expects the number of bits.