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.

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

Refactor encoding, part 2 (diag coded types) #292

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

andlaus
Copy link
Collaborator

@andlaus andlaus commented Apr 18, 2024

This converts the diag coded types to the new and shiny encoding infrastructure. Note that doing this as a stand-alone change requires some kludges like creating temporary EncodeState objects. These will be removed by later PRs in the series...

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

@andlaus andlaus requested a review from kayoub5 April 18, 2024 18:13
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
cursor_bit_position=0,
origin_byte_position=0)
encode_state.cursor_bit_position = encode_state.cursor_bit_position
self.diag_coded_type.encode_into_pdu(internal_trouble_code, encode_state=tmp_state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

make a copy/clone function in the encode state, instead of all this manual attributes copying

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note that this is not a copy of the original state: the cursor and origin byte positions are different and coded_message is empty. Under the new regime this is required to encode only the diag_coded_type, but it is one of the temporary kludges mentioned in the PR description (i.e., it will be gone after the next two PRs)

byte_val = dct.convert_internal_to_bytes("Hi", state, bit_position=0)
self.assertEqual(byte_val, bytes([0x48, 0x69, 0x0]))
state = EncodeState(
coded_message=bytearray([0x00]), parameter_values={}, is_end_of_pdu=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was the 0x12 replaced with zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that was done to avoid those pesky "overlapping parameter" warnings: convert_internal_to_bytes() simply did not care what already was in the PDU, encode_into_pdu() does...

state = EncodeState(
coded_message=bytearray([0xcc]), parameter_values={}, cursor_byte_position=2)
dct.encode_into_pdu(0x12345, state)
self.assertEqual(state.coded_message.hex(), "cc00012345")
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did the assertion changed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's been a while since I did this, so take this with a grain of salt: I suppose it is because this sets the cursor byte position to 2, and the initial value of the PDU to 0xcc. I don't remember why the old code produced two leading zero bytes...

dct = LeadingLengthInfoType(
base_data_type=DataType.A_UNICODE2STRING,
bit_length=8,
base_type_encoding=None,
is_highlow_byte_order_raw=False,
)
state = DecodeState(bytes([0x12, 0x4, 0x61, 0x00, 0x39, 0x00]), cursor_byte_position=1)
state = DecodeState(
bytes([0x12, 0x4, 0x61, 0x00, 0x39, 0x00, 0xff, 0x00]), cursor_byte_position=1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

different bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this tests for little-endian UTF-16 encoding, so the bytes representing the two letters are byte swapped compared to what's on line 106. (the first and the trailing two bytes are just garbage to check if the code properly works with arbitrary positions.)

… the corresponding length key

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
@andlaus andlaus merged commit 0f12e21 into mercedes-benz:main Apr 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants