Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement all encodings of base data type #381

Merged
merged 9 commits into from
Jan 21, 2025
Merged

Implement all encodings of base data type #381

merged 9 commits into from
Jan 21, 2025

Conversation

andlaus
Copy link
Member

@andlaus andlaus commented Jan 17, 2025

For integers, this means packed and unpacked BCD, two-complement, one-complement and sign-magnitude. Strings can be represented using UTF-8, UTF-16 (big or little endian), ISO 8859-1 (latin1), ISO 8859-2 (latin2) and Windows codepage 1252. Finally, floating point values may be encoded using the 32 and 64 bit ISO 754 representation.

Be aware that I'm not sure if the bit mask is handled correctly for negative integers (or if there even is a "correct" handling of bit masks for negative integers). Also, I suspect that most ODX implementations encountered in the wild behave slightly differently for non byte-aligned negative integers...

Besides this, the PR includes a small fix to make the .odx-d file of the somersault ECU compliant with the ODX XSD: It turns out that for whatever reason only empty DESC tags can be specified for parameters (this either is an issue with the XSD or a bug in the XML validator which I like to use (xmllint)), so, as a work-around, the description of schroedinger_param is moved back to .long_name.

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

@andlaus
somersault: put the description of the schroedinger parameter into `.…
d6ae041 
…long_name`

this is not how it is supposed to be, but either the ODX XSD has an
issue, or the `xmllint` tool is buggy:
```
> xmllint --schema odx.xsd --noout somersault.odx-d
somersault.odx-d:826: element DESC: Schemas validity error : Element 'DESC': Character content other than whitespace is not allowed because the content type is 'element-only'.
somersault.odx-d fails to validate
```

This is despite the fact that the group for PARAM tags references
ELEMENT-ID and ELEMENT-ID specifies DESC. (note that an empty DESC tag
passes validation.)

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
Signed-off-by: Christian Hackenbeck <christian.hackenbeck@mercedes-benz.com>
@andlaus andlaus requested a review from kayoub5 January 17, 2025 10:04
@andlaus
Implement all encodings of base data type
06f5e9a 
For integers, this means packed and unpacked BCD, two-complement,
one-complement and sign-magnitude, strings can be encoded using UTF-8,
UTF-16 big or little endian, ISO 8859-1 (latin1), ISO 8859-2 (latin2)
and using Windows codepage 1252. Finally floating point values can be
encoded using 32 and 64 bit ISO 754 representation.

Be aware that I'm not sure if the bit mask is handled correctly for
negative integers (or even if there is a "correct" handling of bit
masks for negative integers). Also, I suspect that most ODX
implementations encountered in the wild behave slightly differently
for non byte-aligned negative integers...

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
Signed-off-by: Christian Hackenbeck <christian.hackenbeck@mercedes-benz.com>
@andlaus andlaus force-pushed the implement_encodings branch from dfde780 to 06f5e9a Compare January 17, 2025 10:07
kayoub5
kayoub5 reviewed Jan 17, 2025
View reviewed changes
odxtools/decodestate.py Outdated Show resolved Hide resolved
kayoub5
kayoub5 reviewed Jan 17, 2025
View reviewed changes
odxtools/decodestate.py Outdated Show resolved Hide resolved
kayoub5
kayoub5 reviewed Jan 17, 2025
View reviewed changes
odxtools/decodestate.py Outdated Show resolved Hide resolved
kayoub5
kayoub5 reviewed Jan 17, 2025
View reviewed changes
odxtools/encodestate.py Outdated
elif base_type_encoding == Encoding.ISO_8859_2:
raw_value = internal_value.encode("iso-8859-2")
elif base_type_encoding == Encoding.WINDOWS_1252:
raw_value = internal_value.encode("cp1252")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste, extract logic into a function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only almost copy-and-paste: DecodeState needs to call .decode() instead of .encode()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create function that takes the coded type as input, and return the correct string encoding name that python uses

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I budge (also for the MinMaxLengthType issue below: 7de4a81 . Thanks for the persistent nagging :)

@andlaus
properly deal with default encodings for base data types
c84241b 
the ASAM specification outlines these in Table 8.

Thanks to [at]kayoub5 for the catch!

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
@andlaus
encodings: add test for NaN en- and decoding
6a6a37d 
I refrained to add a test for inf and float64 because this uses
exactly the same code path as float32.

thanks to [at]kayoub5 for the suggestion.

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
@andlaus
address further review comments
ae17e8a 
these were suggested by [at]kayoub5

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
kayoub5
kayoub5 reviewed Jan 17, 2025
View reviewed changes
odxtools/decodestate.py Outdated
# ... unsigned integers, ...
elif base_data_type == DataType.A_UINT32:
if not isinstance(raw_value, int) or raw_value < 0:
odxraise(f"Raw value must be a positive integer, not {raw_value}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this happen?

Copy link
Member Author

@andlaus andlaus Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it should not be possible because we always extract unsigned integers using bitstruct. This is mainly to prevent things from breaking if this gets changed in the future...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mainly to prevent things from breaking if this gets changed in the future

That's what unit tests are for, I would not mind a few checks, but the function now have more checks that encoding/decoding logic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I'm rather indifferent about this. changed: 5c698b5

kayoub5
kayoub5 reviewed Jan 17, 2025
View reviewed changes
odxtools/encodestate.py

# ... unsigned integers, ...
elif base_data_type == DataType.A_UINT32:
if not isinstance(internal_value, int) or internal_value < 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you put u in bitstruct logic, so that would not happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a user-specified value. for .decode() this is correct...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User specify physical value, not internal one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but e.g., if the compu method is IDENTICAL and the user specifies a negative value, it still arrives here...

@andlaus
centralize the determination of the string encoding
7de4a81 
this is surprisingly complicated. Centralizing this code into a
`get_string_encoding()` function allows to reduce the amount of
copy-and-pasted code and implement the `MinMaxLengthType` diag coded
type properly.

thanks to [at]kayoub5 for insisting on this.

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
@andlaus
extent encoding test for little-endian float32
e91c394 
thanks to [at]kayoub5 for the catch.

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
kayoub5
kayoub5 reviewed Jan 20, 2025
View reviewed changes
odxtools/decodestate.py Outdated Show resolved Hide resolved
kayoub5
kayoub5 reviewed Jan 20, 2025
View reviewed changes
odxtools/decodestate.py Outdated Show resolved Hide resolved
kayoub5
kayoub5 reviewed Jan 20, 2025
View reviewed changes
odxtools/encodestate.py Outdated Show resolved Hide resolved
@andlaus
{De,En}codeState: Use in operator were no separate check for `Non…
356702a 
…e` is necessary

thanks to [at]kayoub5 for the catch.

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
@andlaus
DecodeState: remove unnecessary safety checks
5c698b5 
[at]kayoub5 prefers to not have them and I don't care...

Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
@andlaus andlaus merged commit f5b7b9c into mercedes-benz:main Jan 21, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kayoub5 kayoub5 kayoub5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andlaus @kayoub5