Add codec objects #364

andlaus · 2024-11-21T12:20:42Z

Codecs are any objects that can be en- and decoded into PDUs. The approach so far was to derive all "composite" codecs from BasicStructure and to use a subset of that API for "simple" codable objects like DOPs or parameters. In particular, the previous approach to composite objects was sub-optimal because BASIC-STRUCTURE features subtags that other codable objects like Request and Response do not exhibit (i.e., .byte_size).

Since codecs do not play very nicely with inheritance, the new approach is based on typing protocols (Java would call these "interfaces"): codec.py defines a few runtime checkable typing.Protocol classes which codecs must implement: Codec for the basic functionality, CompositeCodec defines the API for codecs that are composed of a list of parameters like structures, requests, responses or environment datas, and ToplevelCodec defines the "external" API for requests and responses that is supposed to be called by user scripts. Any codable class implementing these APIs can check if it is compliant using isinstance() in the __post_init__() method of the respective class.

Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information

odxtools/basicstructure.py

kayoub5 · 2024-11-21T16:06:52Z

tests/test_encoding.py

@@ -85,7 +85,6 @@ def test_encode_coded_const_infer_order(self) -> None:
            admin_data=None,
            sdgs=[],
            parameters=NamedItemList([param1, param2]),
-            byte_size=None,


Why was byte_size=None, removed?

because the spec says that requests and responses do not have such an attribute. (so far, this was inherited from BasicStructure)

possible to move this change to its own PR? this particular change is too noisy in this PR

that's probably possible, but as I see it, most of the point of this PR is not having to derive Request and Response from BasicStructure anymore...

odxtools/response.py

odxtools/request.py

odxtools/paramlengthinfotype.py

odxtools/parameters/parameter.py

examples/somersaultecu.py

odxtools/codec.py

odxtools/dataobjectproperty.py

kayoub5 · 2024-11-22T14:57:57Z

odxtools/request.py

+    def get_static_bit_length(self) -> Optional[int]:
+        return composite_codec_get_static_bit_length(self)
+
+    def print_free_parameters_info(self) -> None:


personally I think that no object should have print function, that limits the re-usability of the function

I guess I disagree slightly here: When exploring an ODX database using the REPL (which I frequently do), I find it quite a bit more convenient to be able to do something like

service.print<TAB><RETURN>

than

print(service.free<TAB>)<RETURN>

(that's basically the use case for this method...)

you don't need to use print if the function return a string, REPL will auto print the return value of the function

kind of: IMO the readability of strings with new line characters is quite bad if they are not explicitly printed...

Codecs are any objects that can be en- and decoded into PDUs. The approach so far was to derive all "composite" codecs from `BasicStructure` and to use a subset of that API for "simple" codable objects like DOPs or parameters. In particular, the previous approach to composite objects was sub-optimal because `BASIC-STRUCTURE` features subtags that other codable objects like `Request` and `Response` do not exhibit (i.e., `.byte_size`). Since codecs do not play very nicely with inheritance, the new approach is based on typing protocols (Java would call these "interfaces"): `codec.py` defines a few runtime checkable `typing.Protocol` classes which codecs must implement: `Codec` for the basic functionality, `CompositeCodec` defines the API for codecs that are composed of a list of parameters like structures, requests, responses or environment datas, and `ToplevelCodec` defines the "external" API for requests and responses that is supposed to be called by user scripts. Any codable class implementing these APIs can check if it is compliant using `isinstance()` in the `__post_init__()` method of the respective class. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]> Signed-off-by: Gunnar Andersson <[email protected]>

…odec APIs at runtime as [at]kayoub5 correctly notes, this is already done by `mypy` ahead of time. Signed-off-by: Andreas Lauser <[email protected]>

this property has been deprecated for more than a year and after this pull request, request and response objects are no longer structures. Signed-off-by: Andreas Lauser <[email protected]>

kayoub5 · 2024-11-25T12:38:54Z

@andlaus you have a merge conflict

the intention of this was to make the code more robust in non-strict mode, but come think about it, exceptions raised by `._resolve_snref()` will not be caught even in non-strict mode, so we can as well simply skip the exception handling entirely. (besides this, `context.response` is now properly set in `Response._resolve_snrefs()`.) thanks to [at]kayoub5 for noticing this... Signed-off-by: Andreas Lauser <[email protected]>

As [at]kayoub5 rightfully notes, this was not used anywere for real. (Also, the `Response` class did not even strictly implement it, because the `.decode()` and `.encode()` methods accept the additional `coded_request` argument.) Signed-off-by: Andreas Lauser <[email protected]>

this was an artifact stemming from the development history of this PR: originally, the `Codec` type protocol mandated this property, but it turned out that the better approach was to move handling of statically-sized structures from the generic function (`composite_codec_get_static_bit_length()`) to the `get_static_bit_length()` method of structures. Signed-off-by: Andreas Lauser <[email protected]>

andlaus · 2024-11-25T13:24:22Z

@andlaus you have a merge conflict

right, fixed (sorry)

kayoub5 · 2024-11-25T16:11:45Z

odxtools/cli/browse.py

-        print("The following parameters are required but missing!")
-        print(" - " + "\n - ".join(missing_parameter_names))
+        print("The following parameters are required but missing:")
+        print(" - " + "\n - ".join(sorted(missing_parameter_names)))


why add sorted here?

kayoub5 · 2024-11-25T16:12:56Z

odxtools/cli/browse.py

        if parameter is None:
            print(f"I don't know the parameter {parameter_sn}")
            continue

        if isinstance(parameter_value, dict):
            # parameter_value refers to a structure (represented as dict of params)
+            dop = getattr(parameter, "dop", None)
+            inner_params = getattr(dop, "parameters", None)


not using the interface you created?

kayoub5 · 2024-11-25T16:15:51Z

odxtools/codec.py

+    """
+
+    @property
+    def parameters(self) -> List["Parameter"]:


I am not 100% sure, but I think this is more correct

Suggested change

def parameters(self) -> List["Parameter"]:

def parameters(self) -> NamedItemList[Parameter]:

kayoub5 · 2024-11-25T16:17:30Z

odxtools/codec.py

+
+    # Round up to account for padding bits (all structures are
+    # byte aligned)
+    return byte_length * 8


where is the round up code?

kayoub5 · 2024-11-25T16:19:24Z

odxtools/codec.py

+                                           request_prefix: bytes = b'') -> bytes:
+    from .parameters.codedconstparameter import CodedConstParameter
+    from .parameters.matchingrequestparameter import MatchingRequestParameter
+    from .parameters.physicalconstantparameter import PhysicalConstantParameter


why is the import here?

kayoub5 · 2024-11-25T16:25:40Z

odxtools/codec.py

+        for param_value_name in physical_value:
+            if param_value_name not in param_names:
+                odxraise(f"Value for unknown parameter '{param_value_name}' specified "
+                         f"for structure {codec.short_name}")


word "structure" don't fit here anymore

kayoub5 · 2024-11-25T16:26:43Z

odxtools/codec.py

+    for param in codec.parameters:
+        if id(param) == id(codec.parameters[-1]):
+            # The last parameter of the structure is at the end of
+            # the PDU if the structure itcodec is at the end of the


grammatically the sentence makes no sense

kayoub5 · 2024-11-25T16:27:17Z

odxtools/codec.py

+        if id(param) == id(codec.parameters[-1]):
+            # The last parameter of the structure is at the end of
+            # the PDU if the structure itcodec is at the end of the
+            # PDU. TODO: This assumes that the last parameter


don't start a todo in the middle of a comment

kayoub5 · 2024-11-25T16:28:20Z

odxtools/diagcodedtype.py

@@ -55,6 +55,10 @@ def _resolve_snrefs(self, context: SnRefContext) -> None:  # noqa: B027
        """Recursively resolve any short-name references"""
        pass

+    @property
+    def byte_size(self) -> Optional[int]:
+        return None


really needed?

kayoub5 · 2024-11-25T16:30:04Z

odxtools/message.py

-
-    @property
-    @deprecated("use .coding_object")  # type: ignore[misc]
-    def structure(self) -> Union["Request", "Response"]:


breaking change, merit its own PR

kayoub5 · 2024-11-25T16:31:57Z

odxtools/request.py

+
+    def _resolve_odxlinks(self, odxlinks: OdxLinkDatabase) -> None:
+        for param in self.parameters:
+            param._resolve_odxlinks(odxlinks)


what about the other properties?

andlaus requested a review from kayoub5 November 21, 2024 12:20