Comparam subsets: add write support #96

andlaus · 2022-11-18T12:09:49Z

This PR adds write support for COMPARAM-SUBSET. The main implication of this is that the communication parameters of a database written with odxtools should now always come with the correct definitions of their semantics.

Besides this, this PR fixes some minor issues which prevented me from writing back my internal diagnostics files using the pdxcopy example, and it renames all files containing Jinja2 templates to .xml.jinja2 to make IDEs like VSCode happy.

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

ENV_DATA is either ALL-VALUES or specifies a list of DTC-VALUES. So far, the former was implicitly assumed. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

according to the XSD, this is a mandatory attribute for the COMPARAM-SUBSET XML tag... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

kayoub5 · 2022-11-18T12:27:59Z

odxtools/templates/macros/printComparam.xml.jinja2

+ {{- printGenericComparam(sub_cp) | indent(1, first=True) }}
+ {%- endfor %}
+ {%- if cp.physical_default_value is not none %}
+ <COMPLEX-PHYSICAL-DEFAULT-VALUE>{{cp.physical_default_value}}</COMPLEX-PHYSICAL-DEFAULT-VALUE>


hm, I see: this is one of these overcomplicated constructs again. what got me confused is that ComparamSubset physical_default_value() folds this into physical_default_value instead of complex_physical_default_value. I think these two should be separated because the first is for simple parameters while the second is for complex ones?

Using complex_physical_default_value seemed like an overkill, since physical_default_value can support multiple types (based on the dop physical type) and it would force users to do extra type checking for a common property

If I got this right, the typing of this looks something like this:

SimpleValue = str ComplexValue = List[Union[SimpleValue, ComplexValue]]

IMO, conflating these into the same attribute would make things quite confusing (read: I was quite confused), and also, the COMPLEX-COMPARAM and COMPARAM tags use different subtags for this (COMPLEX-PHYSICAL-DEFAULT-VALUE vs PHYSICAL-DEFAULT-VALUE). I've adapted the parameter extraction code in the latest commit, and I think the new version is easier to read. (and it also takes the default values into account...)

odxtools/write_pdx_file.py

andlaus · 2022-11-18T13:48:12Z

ok, I hopefully fixed the issues you've found. During that exercise I noticed that we should consider raising the minimum python version to 3.10 to be able to use kw_only=True for dataclasses. Unfortunately, it is probably a bit too early for that. Opinions, @kayoub5?

kayoub5 · 2022-11-18T13:52:41Z

ok, I hopefully fixed the issues you've found. During that exercise I noticed that we should consider raising the minimum python version to 3.10 to be able to use kw_only=True for dataclasses. Unfortunately, it is probably a bit too early for that. Opinions, @kayoub5?

Too early for min version of 3.10

andlaus · 2022-11-18T13:53:46Z

ok, let's defer this then...

- rename the jinja2 template files to *.jinja2.xml to make some IDEs happy - move the comparam fragments shipped with the ODX standard from the directory for templates to `examples/data` Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

thanks to [at]kayoub5! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

- the values of complex communication parameters are now always a list of strings or nested complex communication parameter values. (the helper function `_get_comparam_value` which produced a dict out of this is no more.) - the default values of communication parameters from their specification is now taken into account by the DiagLayer. - `protocol_snref` and `prot_stack_snref` are correctly internalized and written. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

andlaus · 2022-11-21T16:16:42Z

alright. I've just pushed c71e1d1, and I think this is now ready to go. can you check if it still works for your files?

andlaus · 2022-11-22T09:01:36Z

(If there are no vetos, I'll merge this on Friday (Nov 25)...)

odxtools/diaglayer.py

kayoub5 · 2022-11-22T10:59:55Z

odxtools/diaglayer.py

+
+        return result
+
+    def _get_comparam_subvalue(self,


What happens when a complex param also contains a complex?

tough luck ;)

(I think for the standardized communication parameters this is never the case?)

odxtools/diaglayer.py

more thanks to [at]kayoub5! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

andlaus · 2022-11-22T20:25:24Z

ok, I pushed an update: 80bc8bf

we need it for the type checker, but we don't want to specify it as a hard dependency... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

kayoub5 · 2022-11-24T07:52:49Z

odxtools/communicationparameter.py

-        value = comparam.physical_default_value
-
-    if isinstance(comparam, Comparam):
-        return comparam.dop.physical_type.base_data_type.from_string(value)


@andlaus original code here did take into consideration the dop physical data type

yes, but that was a mightily confusing layer on top of CommunicationParameterRef.value. I suppose that in the future, this should be moved to CommunicationParameterRef. (which is a bit more work than it first seems, because of the writing stuff...)

kayoub5 · 2022-11-24T07:53:10Z

odxtools/communicationparameter.py

-        value = value + [None] * (len(comparam.comparams) - len(value))
-        result = dict()
-        for i, cp in enumerate(comparam.comparams):
-            result[cp.short_name] = _get_comparam_value(cp, value[i])


@andlaus original code here did take into account nested complex

yeah, let's burn that bridge when we come to it (again)

andlaus · 2022-11-25T14:31:07Z

ok. I guess it is time to get this in...

andlaus added 3 commits November 18, 2022 13:05

envdata: deal with the DTC-VALUES

7a24ef1

ENV_DATA is either ALL-VALUES or specifies a list of DTC-VALUES. So far, the former was implicitly assumed. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

comparam subset: add category attribute

9c2087b

according to the XSD, this is a mandatory attribute for the COMPARAM-SUBSET XML tag... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

comparam subsets: add write support

3c0357b

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

andlaus requested a review from kayoub5 November 18, 2022 12:09

kayoub5 reviewed Nov 18, 2022

View reviewed changes

odxtools/write_pdx_file.py Outdated Show resolved Hide resolved

andlaus force-pushed the comparam_subsets_write_support branch from 0fbea5a to bfb449a Compare November 18, 2022 13:52

andlaus added 3 commits November 21, 2022 16:30

consider review comments

6023fec

thanks to [at]kayoub5! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

andlaus force-pushed the comparam_subsets_write_support branch from bfb449a to c71e1d1 Compare November 21, 2022 16:06

kayoub5 reviewed Nov 22, 2022

View reviewed changes

odxtools/diaglayer.py Outdated Show resolved Hide resolved

kayoub5 reviewed Nov 22, 2022

View reviewed changes

odxtools/diaglayer.py Outdated Show resolved Hide resolved

kayoub5 reviewed Nov 22, 2022

View reviewed changes

odxtools/diaglayer.py Outdated Show resolved Hide resolved

consider more review comments

80bc8bf

more thanks to [at]kayoub5! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

workflow: install the types-Deprecated package manually

5e4a596

we need it for the type checker, but we don't want to specify it as a hard dependency... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>

kayoub5 reviewed Nov 24, 2022

View reviewed changes

andlaus merged commit a1592fb into mercedes-benz:main Nov 25, 2022

andlaus deleted the comparam_subsets_write_support branch December 7, 2023 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comparam subsets: add write support #96

Comparam subsets: add write support #96

andlaus commented Nov 18, 2022

kayoub5 Nov 18, 2022

andlaus Nov 18, 2022

kayoub5 Nov 18, 2022

andlaus Nov 21, 2022 •

edited

Loading

andlaus commented Nov 18, 2022

kayoub5 commented Nov 18, 2022

andlaus commented Nov 18, 2022

andlaus commented Nov 21, 2022

andlaus commented Nov 22, 2022

kayoub5 Nov 22, 2022

andlaus Nov 22, 2022 •

edited

Loading

andlaus commented Nov 22, 2022

kayoub5 Nov 24, 2022

andlaus Nov 25, 2022 •

edited

Loading

kayoub5 Nov 24, 2022

andlaus Nov 25, 2022

andlaus commented Nov 25, 2022

Comparam subsets: add write support #96

Comparam subsets: add write support #96

Conversation

andlaus commented Nov 18, 2022

kayoub5 Nov 18, 2022

Choose a reason for hiding this comment

andlaus Nov 18, 2022

Choose a reason for hiding this comment

kayoub5 Nov 18, 2022

Choose a reason for hiding this comment

andlaus Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

andlaus commented Nov 18, 2022

kayoub5 commented Nov 18, 2022

andlaus commented Nov 18, 2022

andlaus commented Nov 21, 2022

andlaus commented Nov 22, 2022

kayoub5 Nov 22, 2022

Choose a reason for hiding this comment

andlaus Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

andlaus commented Nov 22, 2022

kayoub5 Nov 24, 2022

Choose a reason for hiding this comment

andlaus Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

kayoub5 Nov 24, 2022

Choose a reason for hiding this comment

andlaus Nov 25, 2022

Choose a reason for hiding this comment

andlaus commented Nov 25, 2022

andlaus Nov 21, 2022 •

edited

Loading

andlaus Nov 22, 2022 •

edited

Loading

andlaus Nov 25, 2022 •

edited

Loading