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

[provisioning] the DIN fields should be parsed as BCD values #25493

Merged
merged 1 commit into from
Dec 3, 2024
Merged

[provisioning] the DIN fields should be parsed as BCD values #25493

merged 1 commit into from
Dec 3, 2024

Conversation

timothytrippel
Copy link
Contributor

The DIN fields should each be parsed as decimal values, even though they are reported as a hex string; there are just unused bits of each subfield.

milesdai
milesdai approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@milesdai milesdai left a comment

Choose a reason for hiding this comment

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

Added a comment suggestion.

sw/host/provisioning/orchestrator/src/device_id.py Outdated
@@ -61,14 +61,18 @@ def to_int(self) -> int:
din |= self.year
return din

@staticmethod
def _read_int_as_decimal_str(x: int) -> int:
Copy link
Contributor

@milesdai milesdai Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
def _read_int_as_decimal_str(x: int) -> int:
def _read_int_as_decimal_str(x: int) -> int:
"""Interpret a number by reading its hexadecimal representation as a decimal number.
For example, `70` in hex is `0x46` and should be read as the decimal value 46.
This encoding is a manufacturing equipment constraint.
"""

nmoroze
nmoroze approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@nmoroze nmoroze left a comment

Choose a reason for hiding this comment

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

Good code 👍

@timothytrippel
[provisioning] the DIN fields should be parsed as decimal values
2f1e578 
The DIN fields should each be parsed as decimal values, even though they
are reported as a hex string; there are just unused bits of each
subfield.

Signed-off-by: Tim Trippel <[email protected]>
@timothytrippel timothytrippel merged commit 2d55ba1 into lowRISC:earlgrey_1.0.0 Dec 3, 2024
28 of 30 checks passed
@timothytrippel timothytrippel deleted the fix-device-id-parsing branch December 3, 2024 10:27
@timothytrippel timothytrippel changed the title [provisioning] the DIN fields should be parsed as decimal values [provisioning] the DIN fields should be parsed as BCD values Dec 4, 2024
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Dec 4, 2024
@timothytrippel
[orchestrator] fix device ID test
294015b 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in lowRISC#25493. However, the test was not updated accordingly.
Moreover, lowRISC#25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Dec 4, 2024
@timothytrippel
[orchestrator] fix device ID test
39853e4 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in lowRISC#25493. However, the test was not updated accordingly.
Moreover, lowRISC#25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Dec 4, 2024
@timothytrippel
[orchestrator] fix device ID test
6ebe90e 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in lowRISC#25493. However, the test was not updated accordingly.
Moreover, lowRISC#25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Dec 4, 2024
@timothytrippel
[orchestrator] fix device ID test
94375f7 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in lowRISC#25493. However, the test was not updated accordingly.
Moreover, lowRISC#25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Dec 4, 2024
@timothytrippel
[orchestrator] fix device ID test
1d960d5 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in lowRISC#25493. However, the test was not updated accordingly.
Moreover, lowRISC#25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Dec 4, 2024
@timothytrippel
[orchestrator] fix device ID test
09f8da7 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in lowRISC#25493. However, the test was not updated accordingly.
Moreover, lowRISC#25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
timothytrippel added a commit that referenced this pull request Dec 5, 2024
@timothytrippel
[orchestrator] fix device ID test
bed096d 
The DIN portion of the device ID contains fields that are in BCD format,
as was updated in #25493. However, the test was not updated accordingly.
Moreover, #25493 only added parsing BCD formated DINs, but not
generating them from the internal DIN object representation. This has
also been fixed.

Signed-off-by: Tim Trippel <[email protected]>
@jesultra jesultra mentioned this pull request Dec 13, 2024
Open
@jesultra
Copy link
Contributor

@timothytrippel , it seems to me that this PR breaks //sw/host/provisioning/orchestrator/tests:device_id_test, at least that is what I see when running locally.

INFO: Analyzed target //sw/host/provisioning/orchestrator/tests:device_id_test (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
.......E........
======================================================================
ERROR: test_from_hexstr (__main__.TestDeviceId)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/jbk/.cache/bazel/_bazel_jbk/2b6368bd18d7fcda9b5c492dd6e5c4e8/sandbox/linux-sandbox/19/execroot/lowrisc_opentitan/bazel-out/k8-fastbuild/bin/sw/host/provisioning/orchestrator/tests/device_id_test.runfiles/lowrisc_opentitan/sw/host/provisioning/orchestrator/tests/device_id_test.py", line 160, in test_from_hexstr
    cp_device_id = DeviceId.from_hexstr(hexstr)
  File "/usr/local/google/home/jbk/.cache/bazel/_bazel_jbk/2b6368bd18d7fcda9b5c492dd6e5c4e8/sandbox/linux-sandbox/19/execroot/lowrisc_opentitan/bazel-out/k8-fastbuild/bin/sw/host/provisioning/orchestrator/tests/device_id_test.runfiles/lowrisc_opentitan/sw/host/provisioning/orchestrator/src/device_id.py", line 160, in from_hexstr
    return DeviceId.from_int(cp_device_id)
  File "/usr/local/google/home/jbk/.cache/bazel/_bazel_jbk/2b6368bd18d7fcda9b5c492dd6e5c4e8/sandbox/linux-sandbox/19/execroot/lowrisc_opentitan/bazel-out/k8-fastbuild/bin/sw/host/provisioning/orchestrator/tests/device_id_test.runfiles/lowrisc_opentitan/sw/host/provisioning/orchestrator/src/device_id.py", line 189, in from_int
    din = DeviceIdentificationNumber.from_int((base_uid >> 32) & mask_din)
  File "/usr/local/google/home/jbk/.cache/bazel/_bazel_jbk/2b6368bd18d7fcda9b5c492dd6e5c4e8/sandbox/linux-sandbox/19/execroot/lowrisc_opentitan/bazel-out/k8-fastbuild/bin/sw/host/provisioning/orchestrator/tests/device_id_test.runfiles/lowrisc_opentitan/sw/host/provisioning/orchestrator/src/device_id.py", line 67, in from_int
    week = util.read_int_as_decimal_str((din >> 4) & 0xFF)
  File "/usr/local/google/home/jbk/.cache/bazel/_bazel_jbk/2b6368bd18d7fcda9b5c492dd6e5c4e8/sandbox/linux-sandbox/19/execroot/lowrisc_opentitan/bazel-out/k8-fastbuild/bin/sw/host/provisioning/orchestrator/tests/device_id_test.runfiles/lowrisc_opentitan/sw/host/provisioning/orchestrator/src/util.py", line 34, in read_int_as_decimal_str
    return int(hex(x)[2:])
ValueError: invalid literal for int() with base 10: 'c'

----------------------------------------------------------------------
Ran 16 tests in 0.019s

@timothytrippel
Copy link
Contributor Author

Sorry about that, it should be fixed in the latest version of earlgrey_1.0.0 branch. Try pulling the latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milesdai milesdai milesdai approved these changes

@nmoroze nmoroze nmoroze approved these changes

@cfrantz cfrantz Awaiting requested review from cfrantz

@moidx moidx Awaiting requested review from moidx

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

Successfully merging this pull request may close these issues.
4 participants
@timothytrippel @jesultra @nmoroze @milesdai