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

cmake: add REQUIRED option ot dt_nodelabel #78406

Conversation

unsanded
Copy link
Contributor

dt_nodelabel ea currently fail quietly.
This can cause some very confusing errors later on. By adding REQUIRED to the function call one can easily generate a more clear message:

required nodelabel not found: ...

I wrote this because i spent an hour debugging a strange error, which was caused because my dts didn't have a slot0_partition nodelabel, which mcuboot expects.

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Fine extension for cases where a nodelabel, alias, or property is indeed required.

But I would not go as far as saylng that a missing nodelabel, alias, or property is a quiet failure today.
Ref:

  • dt_nodelabel
  • dt_alias
  • dt_prop

these currently fail quietly.

There are many valid use-cases where a label can be missing which should not cause a failure, and it is documented that in those cases, the return variable will be empty so the caller of the function does have the ability to check for that:

# <var> will be undefined if node does not exist.

# The node's path will be returned in the <var> parameter. The
# variable will be left undefined if the alias does not exist.

But having a REQUIRED option for cases where the label / property is required is a fine addition and can avoid boilerplate testing code for developers.

Just a minor observation to fix before a final +1.

@@ -3849,10 +3859,13 @@ endfunction()
# PROPERTY <prop>: Property for which a value should be returned, as it
# appears in the DTS source
# INDEX <idx> : Optional index when retrieving a value in an array property
# REQUIRED : Generate a fatal error if the property is not found

Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove extra empty line.

Suggested change

@tejlmand
Copy link
Collaborator

and remember to fix compliance.

namely:
 - dt_nodelabel
 - dt_alias
 - dt_prop

these currently fail quietly.
This can cause some very confusing errors later on.
By adding `REQUIRED` to the function call one can
easily generate a more clear message:

    required nodelabel not found: ...

Signed-off-by: David van Rijn <david@refractor.dev>
@unsanded unsanded force-pushed the feature/dt_nodelabel_required_option branch from 34727df to 044c788 Compare September 16, 2024 09:46
@unsanded
Copy link
Contributor Author

Thanks for the review, i fixed the compliance and the line :)
Btw, the reason (and an example usecase) for this was: here. I didn't have this nodelabel, on my board (arduino nano ble), but the error only came way down in dt_prop, since path was empty.

@tejlmand
Copy link
Collaborator

tejlmand commented Sep 16, 2024

Btw, the reason (and an example usecase) for this was: here.

Thanks, and exactly that use seems to be flawed in MCUboot here:
https://github.com/zephyrproject-rtos/mcuboot/blob/84b56b61118a1140e1afcb8802e2d94dd9fee44d/boot/zephyr/CMakeLists.txt#L371-L376

where ERROR is used instead of FATAL_ERROR.
If the correct FATAL_ERROR had been used there, my expectation is that you had found your missing node much earlier.

So it seems it is mcuboot which silently fail because it failed to properly handle missing nodes.

@tejlmand
Copy link
Collaborator

sent a patch to mcuboot here: mcu-tools/mcuboot#2065

@unsanded
Copy link
Contributor Author

I agree the flaw was in mcuboot. I was going to send them a patch, but then it seemed that having this option would be the least duplicated code way, hence this pull request.

@fabiobaltieri fabiobaltieri merged commit 96386ea into zephyrproject-rtos:main Sep 17, 2024
23 checks passed
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.

6 participants