refactor `DiagLayer` into a class hierarchy #326

andlaus · 2024-07-31T09:38:56Z

Instead of a single class for all types of diagnostic layers, ODX specifies a hierarchy of types: ECU shared datas, protocols, functional groups, base variants and ECU variants where all but ECU shared data layers derive from a "hierarchy element" to indicate that these may be a member of a inheritance hierarchy defined using PARENT-REFs. This patch implements this approach.

Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information

odxtools/matchingparameter.py

odxtools/multiplexercase.py

odxtools/parentref.py

examples/data/SAE_J2411_SWCAN_CPSS.odx-cs

kayoub5 · 2024-07-31T20:22:55Z

odxtools/diaglayers/ecuvariant.py

+        return self._diag_variables
+
+    @property
+    def variable_groups(self) -> NamedItemList[VariableGroup]:


those are exclusive to ecu variants?

nope, but the XSD defines them separately for ECU-SHARED-DATA, FUNCTIONAL-GROUP, BASE-VARIANT and ECU-VARIANT layers (note: not for PROTOCOL). To avoid confusion, I decided to simply match the spec even if I think that their approach is rather inelegant...

python allows multi inheritance, so move them to shared class

I think that this would be quite confusing for people who try to match the XSD with the python code. (if the specification did it this way, I would be all for it...)

odxtools/diaglayers/ecuvariant.py

kayoub5 · 2024-07-31T20:27:19Z

odxtools/diaglayers/ecuvariantraw.py

+
+
+@dataclass
+class EcuVariantRaw(HierarchyElementRaw):


the concept of raw diaglayer was introduced to reduce the complexity of the diag layer api

if diag layer was split, then that concept should have been no longer needed, instead it's getting duplicate, thus making the diaglayer api more complex, and negating its original purpose

not quite: the reason why we have the split between "raw" and "full" layers is that the "full" layers provide more objects than they define directly because of the various inheritance mechanisms. IOW, it is impossible to reconstruct the contents of the original XML file using just full layers. I agree this makes it quite complex and error prone, but so far I could not come up with a better solution...

the "raw" part could be shared as far as I have seen the code, you don't need a new class for each "raw" type

I'd prefer to keep it simple and stupid even if this comes with a few copy-and-pasted lines. (who knows, maybe ODX 2.3 will modify one of the layers but not the others..)

kayoub5 · 2024-07-31T20:30:24Z

odxtools/diaglayers/protocol.py

+        # as self.diag_layer_raw.
+        result.protocol_raw = deepcopy(self.protocol_raw, memo)
+
+        return result


I am seeing almost identical copy-pasted classes, and still don't see the benefit in term of making things simpler

the purpose of this PR is not to make this stuff simpler, it is to cling to the spec and allowing to eliminate a few systematic errors this way. E.g., if the spec says that a protocol ought to be referenced, we can now expect a Protocol class instead of a DiagLayer. Also, attributes which are not present in the specification do no longer exist in our classes (e.g., parent refs in ECU-SHARED-DATA)

ODX model is for serialization, for actual diagnostic encoding/decoding, you may want to take a look at the standard D-PDU API model.

I use this ODX data to talk to ECUs all the time. as far as I know, the PDX files are even the canonical source of diagnostics data upon which all tools except candela studio build in my company...

tests/test_decoding.py

kayoub5 · 2024-08-05T11:30:49Z

odxtools/cli/_print_utils.py

+            num_dops.append(len(ddds.data_object_props))
+        else:
+            num_dops.append(0)
+        num_comparam_refs.append(len(getattr(variant, "comparams_refs", [])))


What's going on here?

not all diag layers provide .comparam_refs anymore. (Also, the compare tool is in serious need of refactoring...)

kayoub5 · 2024-08-05T11:35:31Z

odxtools/diagcomm.py

                for prot_snref in self.protocol_snrefs
            ])
        else:
            diag_layer = odxrequire(context.diag_layer)
            self._protocols = NamedItemList([
-                resolve_snref(prot_snref, diag_layer.protocols)
+                resolve_snref(prot_snref, getattr(diag_layer, "protocols", []))


Avoid getattr, it allows for typing bugs that mypy can't catch, use isinstance instead

I just tried this: This is pretty difficult to do as part of a list comprehension and it would not come with a major benefit because resolve_snref(..., Protocol) returns a protocol object from the type checking perspective.

kayoub5 · 2024-08-05T11:36:50Z

odxtools/diaglayers/ecuvariant.py

+
+
+@dataclass
+class EcuVariant(HierarchyElement):


Anything that BaseVariant have, should also be available in EcuVariant

Suggested change

class EcuVariant(HierarchyElement):

class EcuVariant(BaseVariant):

I agree, but unfortunately this is not how the spec defines things:

(note that base variants contain "BASE-VARIANT-PATTERNS" instead of "ECU-VARIANT-PATTERNS".)

kayoub5 · 2024-08-05T11:37:09Z

odxtools/diaglayers/ecuvariant.py

+class EcuVariant(HierarchyElement):
+
+    @property
+    def ecu_variant_raw(self) -> EcuVariantRaw:


Why expose the raw data?

because some low-level code might need to know what data was defined by the XML and in order to make typecheckers (mypy) happy, we cannot use .diag_layer to access EcuVariant-specific attributes...

"might" or "do need" ? if "might" then delete, you are not gonna need it, see https://en.m.wikipedia.org/wiki/You_aren't_gonna_need_it

for code which is currently in the main branch odxtools it is not needed, but I have some private scripts which fumble with the internals of ODX databases, and they would become even hackier than they already are...

kayoub5 · 2024-08-05T11:37:45Z

odxtools/diaglayers/ecuvariant.py

+        return cast(EcuVariantRaw, self.diag_layer_raw)
+
+    @property
+    def diag_variables_raw(self) -> List[Union[DiagVariable, OdxLinkRef]]:


The raw part should be internal, why expose it as a property ?

basically just to make the API of EcuVariant a superset of EcuVariantRaw (i.e., substituting EcuVariantRaw objects by EcuVariant ones should just work without any changes.)

kayoub5 · 2024-08-05T11:41:11Z

odxtools/diaglayers/basevariant.py

+
+        return self._compute_available_objects(get_local_objects_fn, not_inherited_fn)
+
+    def _compute_available_variable_groups(self,


I see copy-paste of this function and similar "compute" functions, move them to a helper class

While I also don't like the copy and paste across diag layers, given that this is not too much code, the added complexity of the cure would IMO be worse than the disease...

kayoub5 · 2024-08-05T11:44:02Z

odxtools/diagvariable.py

@@ -17,6 +18,14 @@
 from .variablegroup import VariableGroup


+@runtime_checkable
+class HasDiagVariables(typing.Protocol):


What layers don't have diag variables?

AFAICS just Protocol.

Instead of a single class for all types of diagnostic layers, ODX specifies a hierarchy of types: ECU shared datas, protocols, functional groups, base variants and ECU variants whereas all but ECU shared data layers derive from a "hierarchy element" to indicate that these may be a member of a inheritance hierarchy defined using `PARENT-REF`s. This patch implements this approach. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Florian Jost <florian.jost@mbition.io>

instead, let's use type checking protocols. Note that this trick cannot be used for the `hasattr("protocol")` instances of `DiagComm` because this would lead to cyclic imports. thanks to [at]kayoub5 for suggesting this! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Florian Jost <florian.jost@mbition.io>

…g layers If the underlying raw layer does not provide a DDDS, and inheritance does not apply either, an empty object is created. This should help to avoid having to handle a few special cases in user code. Thanks to [at]kayoub for the catch! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Florian Jost <florian.jost@mbition.io>

this slightly increases the coverage of the tests. (With the exception of the variant pattern functionality, `BaseVariant`s are supposed to exhibit the same API ase `EcuVariant`s.) Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Florian Jost <florian.jost@mbition.io>

andlaus · 2024-08-06T09:37:23Z

@kayoub5: do you consider this ready to be merged? (after merging this, I'll do an v8.0 release.)

andlaus · 2024-08-12T14:45:09Z

@kayoub5: if no new issues crop up until then, I'll merge this PR on Thursday or Friday...

kayoub5 · 2024-08-12T14:52:33Z

@kayoub5: if no new issues crop up until then, I'll merge this PR on Thursday or Friday...

I am on the beach, with little access to the pc, I believe I already reviewed most of this PR.

If I find issues later, I will create hotfix PRs.

andlaus · 2024-08-12T15:40:16Z

great, thanks. Let's get this merged. (have a nice vacation :))

andlaus requested a review from kayoub5 July 31, 2024 09:38

andlaus force-pushed the diag_layer_hierarchy branch from 5e33bbb to 545e7d6 Compare July 31, 2024 10:22