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

edtlib: add hash attribute to nodes #83748

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pillo79
Copy link
Collaborator

@pillo79 pillo79 commented Jan 9, 2025

Add a new hash attribute to all Devicetree EDT nodes. The hash is calculated on the full path of the node; this means that its value remains stable across rebuilds. The hash is checked for uniqueness among nodes in the same EDT.

This computed token is then added to devicetree_generated.h and made accessible to Zephyr code via a new DT_HASH(node_id) macro.

A proposed usage of this new attribute is in RFC #77799, to allow stable device lookups from extensions.

Add a new "hash" attribute to all Devicetree EDT nodes. The hash is
calculated on the full path of the node; this means that its value
remains stable across rebuilds.
The hash is checked for uniqueness among nodes in the same EDT.

This computed token is then added to `devicetree_generated.h` and made
accessible to Zephyr code via a new DT_HASH(node_id) macro.

Signed-off-by: Luca Burelli <[email protected]>
@pillo79 pillo79 marked this pull request as ready for review January 9, 2025 16:16
# Calculates the hash associated with the node's full path.
hasher = hashlib.sha256()
hasher.update(path.encode())
hash = base64.b64encode(hasher.digest()).decode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why encode it with base64? Maybe use hexdigest instead?

Copy link
Collaborator Author

@pillo79 pillo79 Jan 10, 2025

Choose a reason for hiding this comment

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

My first version was with hexdigest(), but that is a 64 char string and thus the final identifiers ended up well over 80. Using base64 as was suggested in this thread trims ~20 chars off and makes them easier to distinguish, at basically the same vanishingly small collision probability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, b64encode can be passed alternatives for the + and / chars already:

return base64.b64encode(hasher.digest(), altchars="__").decode().rstrip("=")

@rruuaanng
Copy link
Collaborator

Does this PR have an associated open issue? I would like to review the discussion. Could you provide any links to discussions related to this proposal?

@pillo79
Copy link
Collaborator Author

pillo79 commented Jan 10, 2025

Does this PR have an associated open issue? I would like to review the discussion. Could you provide any links to discussions related to this proposal?

@rruuaanng the rationale is described in the linked PR, but there is no issue, and by your comment it looks like there should have been. I'm afraid I'm still new to many of the Zephyr processes... 😰

* @param node_id node identifier
* @return hash value as a preprocessor token
*/
#define DT_HASH(node_id) DT_CAT(node_id, _HASH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#define DT_HASH(node_id) DT_CAT(node_id, _HASH)
#define DT_NODE_HASH(node_id) DT_CAT(node_id, _HASH)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is closely related to the node hierarchy, so HASH should not be used as a secondary naming convention. So, we should clearly label it as the function for obtaining the node's hash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test for macro DT_HASH(or DT_NODE_HASH) in tests/lib/devicetree/api/src/main.c? I think this would be better, as the ultimate audience is C, not edtlib.

@rruuaanng
Copy link
Collaborator

Does this PR have an associated open issue? I would like to review the discussion. Could you provide any links to discussions related to this proposal?

@rruuaanng the rationale is described in the linked PR, but there is no issue, and by your comment it looks like there should have been. I'm afraid I'm still new to many of the Zephyr processes... 😰

I just want to know more context, you're doing great. However, I'd like to check if this PR has opened an issue, as I don’t want to miss anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants