Miscellaneous fixes #386

andlaus · 2025-02-12T14:44:05Z

This PR contains a large number of patches for smaller issues which IMO do not merit separate pull requests. Since all patches of this series are self-contained, please let me know if you prefer it to be split into two or three PRs...

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

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

…inition if the specification has not yet been resolved when accesing `.short_name` an exception will be raised (though luck) Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Gerrit Ecke <[email protected]>

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

examples/somersaultecu.py

kayoub5 · 2025-02-12T15:57:01Z

examples/somersaultecu.py

@@ -1094,7 +1096,8 @@ class SomersaultSID(IntEnum):
            oid=None,
            odx_id=OdxLinkId("somersault.struct.recall.forward_flips_grudgingly_done", doc_frags),
            parameters=somersault_positive_responses["forward_flips_grudgingly_done"].parameters,
-            byte_size=None),
+            byte_size=None,
+            is_visible_raw=None),


And this is why I hate when they are not used

I can add them to whole codebase if you want, but that's for another PR...

kayoub5 · 2025-02-12T15:57:30Z

odxtools/comparaminstance.py

-
-        # ODXLINK IDs allow dots and hyphens, but short names do not.
-        # (This should not happen anyway in a correct PDX...)
-        return self.spec_ref.ref_id.replace(".", "__").replace("-", "_")


because accessing the short name before reference resolution IMO ought not to be supported. In particular because the short name would change depending on references have been resolved or not, so this IMO constitutes a medium-sized footgun...

self.spec is not available during object creation, but short name is used during object creation to populate the named list

this is what I mean with "footgun": if this was done this way, the NamedList would feature incorrect names because after reference resolution, the short_name attributes change whilst the list stays the same. This is the reason why the NamedItemList of ComparamInstance objects is only created after ODXLINK resolution. My point is that is better to get an exception if the code is incorrect rather than incorrect results...

odxtools/diagcomm.py

odxtools/diagnostictroublecode.py

odxtools/diagvariable.py

odxtools/inputparam.py

odxtools/modification.py

odxtools/parameters/tablekeyparameter.py

odxtools/scaleconstr.py

tests/test_diag_data_dictionary_spec.py

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

this is probably quite difficult to implement Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Gerrit Ecke <[email protected]>

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

i.e., add deprecated properties using the old name for `.dop`. thanks to [at]kayoub5 for the catch. Signed-off-by: Andreas Lauser <[email protected]>

the nesting was wrong... Signed-off-by: Andreas Lauser <[email protected]>

andlaus · 2025-02-13T15:24:00Z

@kayoub5: do you see any issues which should prevent merging this?

kayoub5 · 2025-02-13T16:16:31Z

@nada-ben-ali could you cross this PR against the files we have?

I have a feeling one of those many small changes that are standard compliant may not align with real world files.

nada-ben-ali · 2025-02-18T11:17:13Z

@nada-ben-ali could you cross this PR against the files we have?

I have a feeling one of those many small changes that are standard compliant may not align with real world files.

@kayoub5 the tests failed for three DIDs. I'm checking the root cause...

andlaus · 2025-02-19T09:25:49Z

any news on this front @nada-ben-ali?

nada-ben-ali · 2025-02-19T11:09:17Z

any news on this front @nada-ben-ali?

I got the following error while encoding:

@andlaus I think it's:
raw_value = internal_value
instead of:
internal_value = raw_value

right?

kayoub5 · 2025-02-19T12:24:06Z

@nada-ben-ali what was the "base_type_encoding" used by the ODX file?

nada-ben-ali · 2025-02-19T12:51:48Z

@nada-ben-ali what was the "base_type_encoding" used by the ODX file?

one DID has:

and the other:

thanks to [at]nada-ben-ali for finding this! Signed-off-by: Andreas Lauser <[email protected]>

andlaus · 2025-02-19T14:10:18Z

these encoding issues are probably due to #381, not this PR!? Do you have any idea what actually goes wrong? IIRC, I added comprehensive unit tests for all base type encodings (cf https://github.com/mercedes-benz/odxtools/blob/main/tests/test_encoding.py#L93-L437), but possibly I made a few mistakes there...

andlaus · 2025-02-19T14:12:43Z

@andlaus I think it's:
raw_value = internal_value
instead of:
internal_value = raw_value

right?

right. fixed: 3b807eb

nada-ben-ali · 2025-02-19T14:26:07Z

@andlaus I think it's:
raw_value = internal_value
instead of:
internal_value = raw_value
right?

right. fixed: 3b807eb

I think we need to fix it here too

odxtools/odxtools/encodestate.py

Line 160 in 3b369b5

internal_value = raw_value

as the previous commit, this only affects the non-strict mode. (This time I also checked that there are no more instances of this and that the the reverse does not happen in the inverse function of `DecodeState`... again, thanks to [at]nada-ben-ali for finding this! Signed-off-by: Andreas Lauser <[email protected]>

andlaus · 2025-02-19T14:34:25Z

oops, right. Fixed: a8edd4b (I hope that I did not miss another instance of this...)

nada-ben-ali · 2025-02-19T14:35:27Z

these encoding issues are probably due to #381, not this PR!? Do you have any idea what actually goes wrong? IIRC, I added comprehensive unit tests for all base type encodings (cf https://github.com/mercedes-benz/odxtools/blob/main/tests/test_encoding.py#L93-L437), but possibly I made a few mistakes there...

yes it must be due to #381 , we didn't get this issue using release 9.3.0.

andlaus · 2025-02-19T14:36:08Z

yes it must be due to #381 , we didn't get this issue using release 9.3.0.

let's get it fixed ASAP...

nada-ben-ali · 2025-02-19T14:59:02Z

yes it must be due to #381 , we didn't get this issue using release 9.3.0.

let's get it fixed ASAP...

@andlaus but I think it's a Pdx file issue, for :

A_INT32, we don't have the encoding BCD-P
A_UINT32, we don't have the encoding 2C

Introducing all encodings of base data type highlighted the Pdx issue, but using the latest fix for raw_value = internal_value will prevent the UnboundLocalError issue and will use the internal value as a fallback in non-strict mode.

@kayoub5 what do you think?

andlaus · 2025-02-19T15:40:28Z

I agree.

Maybe we can allow the BCD encodings for INT32. The spec disallows this, probably because BCD cannot represent negative values, but I have a feeling that negative values are rarely transmitted over the wire anyway...

UINT32 using two-complement encoding obviously does not make any sense because UINT32 obviously only works for positive numbers. The question is what ought to happen in such a case if a value where the most significant bit is set is encountered.

kayoub5 · 2025-02-19T16:28:12Z

UINT32 using two-complement encoding obviously does not make any sense because UINT32 obviously only works for positive numbers. The question is what ought to happen in such a case if a value where the most significant bit is set is encountered.

My feeling on this is that someone is not likely to put 2C as encoding by accident (since it's optional), but they may put UINT32 instead of INT32 by accident.

andlaus · 2025-02-19T19:44:02Z

My feeling on this is that someone is not likely to put 2C as encoding by accident (since it's optional), but they may put UINT32 instead of INT32 by accident.

sounds reasonable. That said, what do the existing commercial tools do in that case and do all that claim to be compliant behave the same? Since this is undefined behaviour, I think odxtools should continue to raise a DecodeError in strict mode, while in non-strict mode we should probably always switch to "no encoding" if the base type is unsigned but the specified encoding is 1C or 2C. what do you think?

kayoub5 · 2025-02-20T06:30:53Z

My feeling on this is that someone is not likely to put 2C as encoding by accident (since it's optional), but they may put UINT32 instead of INT32 by accident.

sounds reasonable. That said, what do the existing commercial tools do in that case and do all that claim to be compliant behave the same? Since this is undefined behaviour, I think odxtools should continue to raise a DecodeError in strict mode, while in non-strict mode we should probably always switch to "no encoding" if the base type is unsigned but the specified encoding is 1C or 2C. what do you think?

that's ok as well

nada-ben-ali · 2025-02-20T10:38:03Z

@andlaus Could we please have a new release today? The latest changes are urgently required.

andlaus added 11 commits February 12, 2025 15:40

Structure: internalize IS-VISIBLE

4cf4df0

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

DiagComm: fix copy-and-pasto

70290d5

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

CompanyRevisionInfo: fix typo

69bf069

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

DiagnosticTroubleCode: the .text attribute is mandatory!

ec39e28

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

Modification: .change is mandatory

0e91da4

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

PhysicalConstantParameter: make it more robust in non-strict mode

371a9d8

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

SwVariable: internalize the "OID" attribute

b30612d

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

printParentRef: write all NOT-INHERITED subtags

4361ecc

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

DiagComm: use List instead of Iterable for *_refs attributes

c2a11c8

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

DiagnosticTroubleCode: simplify .from_et()

8ee502c

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

andlaus requested a review from kayoub5 February 12, 2025 14:44