From 9c21b654aafe79e672093e5f51d84d86ff0de387 Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 09:34:46 +0200 Subject: [PATCH 1/7] use `odxraise()` for non-lenient ODXLINK resolves this allows to continue in non-strict mode, albeit with undefined results. Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/odxlink.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/odxtools/odxlink.py b/odxtools/odxlink.py index b6ba55cf..78b7c827 100644 --- a/odxtools/odxlink.py +++ b/odxtools/odxlink.py @@ -211,8 +211,10 @@ def resolve(self, ref: OdxLinkRef, expected_type: Optional[Any] = None) -> Any: return obj - raise KeyError(f"ODXLINK reference {ref} could not be resolved for any " - f"of the document fragments {ref.ref_docs}") + odxraise( + f"ODXLINK reference {ref} could not be resolved for any " + f"of the document fragments {ref.ref_docs}", KeyError) + return None @overload def resolve_lenient(self, ref: OdxLinkRef, expected_type: None = None) -> Any: From 4e5664665923f9aee5426631644a7947dd90e9c5 Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 09:35:01 +0200 Subject: [PATCH 2/7] ParameterWithDop: resolve the DOP non-leniently we now hopefully implement all specified DOPs, so we can be more strict if a referenced DOP cannot be resolved. Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/parameters/parameterwithdop.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/odxtools/parameters/parameterwithdop.py b/odxtools/parameters/parameterwithdop.py index b5ab03aa..7a08091e 100644 --- a/odxtools/parameters/parameterwithdop.py +++ b/odxtools/parameters/parameterwithdop.py @@ -43,7 +43,7 @@ def from_et(et_element: ElementTree.Element, def __post_init__(self) -> None: odxassert(self.dop_snref is not None or self.dop_ref is not None, f"Param {self.short_name} without a DOP-(SN)REF should not exist!") - self._dop: Optional[DopBase] = None + self._dop: DopBase @override def _build_odxlinks(self) -> Dict[OdxLinkId, Any]: @@ -55,10 +55,7 @@ def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None: if self.dop_ref is not None: odxassert(self.dop_snref is None) - # TODO: do not do lenient resolves here. The problem is - # that currently not all kinds of DOPs are internalized - # (e.g., static and dynamic fields) - self._dop = odxlinks.resolve_lenient(self.dop_ref) + self._dop = odxlinks.resolve(self.dop_ref) @override def _parameter_resolve_snrefs(self, diag_layer: "DiagLayer", *, @@ -71,11 +68,9 @@ def _parameter_resolve_snrefs(self, diag_layer: "DiagLayer", *, @property def dop(self) -> DopBase: - """may be a DataObjectProperty, a Structure or None""" + """This is usually a DataObjectProperty or a Structure object""" - return odxrequire( - self._dop, "Specifying a data object property is mandatory but it " - "could not be resolved") + return self._dop @override def get_static_bit_length(self) -> Optional[int]: From af1ebfdbd938f242c755b46be4334676c0eaab6b Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 09:35:16 +0200 Subject: [PATCH 3/7] allow to specify the values for all list types via tuples and lists so far, `DynamicLengthField` and `EndOfPduField` only allowed lists. Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/dynamiclengthfield.py | 2 +- odxtools/endofpdufield.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/odxtools/dynamiclengthfield.py b/odxtools/dynamiclengthfield.py index 68c793c0..04a78afd 100644 --- a/odxtools/dynamiclengthfield.py +++ b/odxtools/dynamiclengthfield.py @@ -55,7 +55,7 @@ def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeSt odxassert(encode_state.cursor_bit_position == 0, "No bit position can be specified for dynamic length fields!") - if not isinstance(physical_value, list): + if not isinstance(physical_value, (tuple, list)): odxraise( f"Expected a list of values for dynamic length field {self.short_name}, " f"got {type(physical_value)}", EncodeError) diff --git a/odxtools/endofpdufield.py b/odxtools/endofpdufield.py index f46e1706..384a8b8c 100644 --- a/odxtools/endofpdufield.py +++ b/odxtools/endofpdufield.py @@ -47,7 +47,7 @@ def encode_into_pdu(self, physical_value: Optional[ParameterValue], odxassert(not encode_state.cursor_bit_position, "No bit position can be specified for end-of-pdu fields!") - if not isinstance(physical_value, list): + if not isinstance(physical_value, (tuple, list)): odxraise( f"Invalid type {type(physical_value).__name__} of physical " f"value for end-of-pdu field, expected a list", EncodeError) From 459c85c630ba976052fe33997436172d61c95078 Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 09:35:31 +0200 Subject: [PATCH 4/7] fields: fix end-of-PDU handling an element of a field is at the end of a PDU if the field is at the end of the PDU and the item is the last element of the field. Note that even though some pathetic cases where this makes a difference can be constructed, I have doubts if this matters in practice. Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/dynamiclengthfield.py | 8 +++++++- odxtools/endofpdufield.py | 12 +++++++++++- odxtools/staticfield.py | 9 ++++++++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/odxtools/dynamiclengthfield.py b/odxtools/dynamiclengthfield.py index 04a78afd..dc64f36b 100644 --- a/odxtools/dynamiclengthfield.py +++ b/odxtools/dynamiclengthfield.py @@ -77,8 +77,14 @@ def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeSt encode_state.cursor_byte_position = encode_state.origin_byte_position + self.offset encode_state.cursor_bit_position = 0 - for value in physical_value: + orig_is_end_of_pdu = encode_state.is_end_of_pdu + encode_state.is_end_of_pdu = False + for i, value in enumerate(physical_value): + if i == len(physical_value) - 1: + encode_state.is_end_of_pdu = orig_is_end_of_pdu + self.structure.encode_into_pdu(value, encode_state) + encode_state.is_end_of_pdu = orig_is_end_of_pdu # ensure the correct message size if the field is empty if len(physical_value) == 0: diff --git a/odxtools/endofpdufield.py b/odxtools/endofpdufield.py index 384a8b8c..330b6988 100644 --- a/odxtools/endofpdufield.py +++ b/odxtools/endofpdufield.py @@ -46,6 +46,8 @@ def encode_into_pdu(self, physical_value: Optional[ParameterValue], encode_state: EncodeState) -> None: odxassert(not encode_state.cursor_bit_position, "No bit position can be specified for end-of-pdu fields!") + odxassert(encode_state.is_end_of_pdu, + "End-of-pdu fields can only be located at the end of PDUs!") if not isinstance(physical_value, (tuple, list)): odxraise( @@ -53,9 +55,17 @@ def encode_into_pdu(self, physical_value: Optional[ParameterValue], f"value for end-of-pdu field, expected a list", EncodeError) return - for value in physical_value: + orig_is_end_of_pdu = encode_state.is_end_of_pdu + encode_state.is_end_of_pdu = False + + for i, value in enumerate(physical_value): + if i == len(physical_value) - 1: + encode_state.is_end_of_pdu = orig_is_end_of_pdu + self.structure.encode_into_pdu(value, encode_state) + encode_state.is_end_of_pdu = orig_is_end_of_pdu + @override def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: odxassert(not decode_state.cursor_bit_position, diff --git a/odxtools/staticfield.py b/odxtools/staticfield.py index a0c492bd..713b309e 100644 --- a/odxtools/staticfield.py +++ b/odxtools/staticfield.py @@ -55,11 +55,16 @@ def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeSt odxraise(f"Value for static field '{self.short_name}' " f"must be a list of size {self.fixed_number_of_items}") - for val in physical_value: + orig_is_end_of_pdu = encode_state.is_end_of_pdu + encode_state.is_end_of_pdu = False + for i, val in enumerate(physical_value): if not isinstance(val, dict): odxraise(f"The individual parameter values for static field '{self.short_name}' " f"must be dictionaries for structure '{self.structure.short_name}'") + if i == len(physical_value) - 1: + encode_state.is_end_of_pdu = orig_is_end_of_pdu + pos_before = encode_state.cursor_byte_position self.structure.encode_into_pdu(val, encode_state) pos_after = encode_state.cursor_byte_position @@ -75,6 +80,8 @@ def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeSt encode_state.emplace_bytes(b'\x00' * (self.item_byte_size - (pos_after - pos_before))) + encode_state.is_end_of_pdu = orig_is_end_of_pdu + @override def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: From 787a8074ff6c4fe2e4b5f8435a58fc0e95c8ac92 Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 09:35:46 +0200 Subject: [PATCH 5/7] EndOfPduField, StaticField: set the origin position during encoding Note that this does not make a difference because all items of fields are structures, which need to set the origin position to the beginning of the structure anyway, but IMO it is conceptually clearer to explicitly set the origin position in fields as well. Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/endofpdufield.py | 7 +++++++ odxtools/staticfield.py | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/odxtools/endofpdufield.py b/odxtools/endofpdufield.py index 330b6988..8515bd26 100644 --- a/odxtools/endofpdufield.py +++ b/odxtools/endofpdufield.py @@ -71,6 +71,10 @@ def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: odxassert(not decode_state.cursor_bit_position, "No bit position can be specified for end-of-pdu fields!") + orig_origin = decode_state.origin_byte_position + orig_cursor = decode_state.cursor_byte_position + decode_state.origin_byte_position = decode_state.cursor_byte_position + result: List[ParameterValue] = [] while decode_state.cursor_byte_position < len(decode_state.coded_message): # ATTENTION: the ODX specification is very misleading @@ -79,4 +83,7 @@ def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: # repeated are identical, not their values result.append(self.structure.decode_from_pdu(decode_state)) + decode_state.origin_byte_position = orig_origin + decode_state.cursor_byte_position = max(orig_cursor, decode_state.cursor_byte_position) + return result diff --git a/odxtools/staticfield.py b/odxtools/staticfield.py index 713b309e..0f75bb8a 100644 --- a/odxtools/staticfield.py +++ b/odxtools/staticfield.py @@ -88,6 +88,10 @@ def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: odxassert(decode_state.cursor_bit_position == 0, "No bit position can be specified for static length fields!") + orig_origin = decode_state.origin_byte_position + orig_cursor = decode_state.cursor_byte_position + decode_state.origin_byte_position = decode_state.cursor_byte_position + result: List[ParameterValue] = [] for _ in range(self.fixed_number_of_items): orig_cursor = decode_state.cursor_byte_position @@ -101,4 +105,7 @@ def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: decode_state.cursor_byte_position = orig_cursor + self.item_byte_size + decode_state.origin_byte_position = orig_origin + decode_state.cursor_byte_position = max(orig_cursor, decode_state.cursor_byte_position) + return result From f81b40b7d1e4b8e144575062b719b4b6bf2b4770 Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 09:36:16 +0200 Subject: [PATCH 6/7] improve the docstrings for `EnvironmentData` and `EnvironmentDataDescription` it turns out that these classes are yet another multiplexer mechanism. Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/environmentdata.py | 9 ++++++++- odxtools/environmentdatadescription.py | 10 ++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/odxtools/environmentdata.py b/odxtools/environmentdata.py index 89e4f012..6e3e1d22 100644 --- a/odxtools/environmentdata.py +++ b/odxtools/environmentdata.py @@ -11,7 +11,14 @@ @dataclass class EnvironmentData(BasicStructure): - """This class represents Environment Data that describes the circumstances in which the error occurred.""" + """This class represents Environment Data that describes the + circumstances in which the error occurred. + + This is one of the many multiplexer mechanisms specified by the + ODX standard, because an environment data parameter must only be + used if a DTC parameter has a certain set of values. (In this + sense, it is quite similar to NRC-CONST parameters.) + """ all_value: Optional[bool] dtc_values: List[int] diff --git a/odxtools/environmentdatadescription.py b/odxtools/environmentdatadescription.py index eb61925f..c3dd37d9 100644 --- a/odxtools/environmentdatadescription.py +++ b/odxtools/environmentdatadescription.py @@ -20,8 +20,14 @@ @dataclass class EnvironmentDataDescription(ComplexDop): - """This class represents Environment Data Description, which is a complex DOP - that is used to define the interpretation of environment data.""" + """This class represents environment data descriptions + + An environment data description provides a list of all environment + data objects that are potentially applicable to decode a given + response. (If a given environment data object is applicable + depends on the value of the DtcDOP that is associated with it.) + + """ # in ODX 2.0.0, ENV-DATAS seems to be a mandatory # sub-element of ENV-DATA-DESC, in ODX 2.2 it is not From 7c5a56c68d505a33554d8006afaaec370c3c95b6 Mon Sep 17 00:00:00 2001 From: Andreas Lauser Date: Tue, 7 May 2024 13:21:31 +0200 Subject: [PATCH 7/7] fields: allow to use any sequence to specify fields The reason we expect `Sequence` instead of `Iterable` is because we use the `len()` method which is not available for `Iterable`. thanks to [at]kayoub5 for the catch! Signed-off-by: Andreas Lauser Signed-off-by: Gerrit Ecke --- odxtools/dynamicendmarkerfield.py | 4 ++-- odxtools/dynamiclengthfield.py | 4 ++-- odxtools/endofpdufield.py | 4 ++-- odxtools/odxtypes.py | 5 +++-- odxtools/staticfield.py | 4 ++-- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/odxtools/dynamicendmarkerfield.py b/odxtools/dynamicendmarkerfield.py index a85bb328..5e31aada 100644 --- a/odxtools/dynamicendmarkerfield.py +++ b/odxtools/dynamicendmarkerfield.py @@ -1,6 +1,6 @@ # SPDX-License-Identifier: MIT from dataclasses import dataclass -from typing import TYPE_CHECKING, Any, Dict, List +from typing import TYPE_CHECKING, Any, Dict, List, Sequence from xml.etree import ElementTree from typing_extensions import override @@ -65,7 +65,7 @@ def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeSt odxassert(encode_state.cursor_bit_position == 0, "No bit position can be specified for dynamic endmarker fields!") - if not isinstance(physical_value, (tuple, list)): + if not isinstance(physical_value, Sequence): odxraise( f"Expected a sequence of values for dynamic endmarker field {self.short_name}, " f"got {type(physical_value).__name__}", EncodeError) diff --git a/odxtools/dynamiclengthfield.py b/odxtools/dynamiclengthfield.py index dc64f36b..bff0889a 100644 --- a/odxtools/dynamiclengthfield.py +++ b/odxtools/dynamiclengthfield.py @@ -1,6 +1,6 @@ # SPDX-License-Identifier: MIT from dataclasses import dataclass -from typing import TYPE_CHECKING, Any, Dict, List +from typing import TYPE_CHECKING, Any, Dict, List, Sequence from xml.etree import ElementTree from typing_extensions import override @@ -55,7 +55,7 @@ def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeSt odxassert(encode_state.cursor_bit_position == 0, "No bit position can be specified for dynamic length fields!") - if not isinstance(physical_value, (tuple, list)): + if not isinstance(physical_value, Sequence): odxraise( f"Expected a list of values for dynamic length field {self.short_name}, " f"got {type(physical_value)}", EncodeError) diff --git a/odxtools/endofpdufield.py b/odxtools/endofpdufield.py index 8515bd26..53d6a7ef 100644 --- a/odxtools/endofpdufield.py +++ b/odxtools/endofpdufield.py @@ -1,6 +1,6 @@ # SPDX-License-Identifier: MIT from dataclasses import dataclass -from typing import List, Optional +from typing import List, Optional, Sequence from xml.etree import ElementTree from typing_extensions import override @@ -49,7 +49,7 @@ def encode_into_pdu(self, physical_value: Optional[ParameterValue], odxassert(encode_state.is_end_of_pdu, "End-of-pdu fields can only be located at the end of PDUs!") - if not isinstance(physical_value, (tuple, list)): + if not isinstance(physical_value, Sequence): odxraise( f"Invalid type {type(physical_value).__name__} of physical " f"value for end-of-pdu field, expected a list", EncodeError) diff --git a/odxtools/odxtypes.py b/odxtools/odxtypes.py index a8baad6c..7158b90c 100644 --- a/odxtools/odxtypes.py +++ b/odxtools/odxtypes.py @@ -1,6 +1,7 @@ # SPDX-License-Identifier: MIT from enum import Enum -from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Tuple, Type, Union, overload +from typing import (TYPE_CHECKING, Any, Callable, Dict, Iterable, Optional, Tuple, Type, Union, + overload) from xml.etree import ElementTree from .exceptions import odxassert, odxraise, odxrequire @@ -28,7 +29,7 @@ def bytefield_to_bytearray(bytefield: str) -> bytearray: # multiple items, so this can be a list of objects. TableStructParameterValue = Tuple[str, "ParameterValue"] ParameterValue = Union[AtomicOdxType, "ParameterValueDict", TableStructParameterValue, - List["ParameterValue"], "DiagnosticTroubleCode"] + Iterable["ParameterValue"], "DiagnosticTroubleCode"] ParameterValueDict = Dict[str, ParameterValue] diff --git a/odxtools/staticfield.py b/odxtools/staticfield.py index 0f75bb8a..76069f65 100644 --- a/odxtools/staticfield.py +++ b/odxtools/staticfield.py @@ -1,6 +1,6 @@ # SPDX-License-Identifier: MIT from dataclasses import dataclass -from typing import TYPE_CHECKING, Any, Dict, List +from typing import TYPE_CHECKING, Any, Dict, List, Sequence from xml.etree import ElementTree from typing_extensions import override @@ -51,7 +51,7 @@ def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None: def encode_into_pdu(self, physical_value: ParameterValue, encode_state: EncodeState) -> None: if not isinstance(physical_value, - (tuple, list)) or len(physical_value) != self.fixed_number_of_items: + Sequence) or len(physical_value) != self.fixed_number_of_items: odxraise(f"Value for static field '{self.short_name}' " f"must be a list of size {self.fixed_number_of_items}")