-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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: dts: print binding dirs and board extension overlays #83533
cmake: dts: print binding dirs and board extension overlays #83533
Conversation
e5caa4a
to
8f84d7d
Compare
cmake/modules/dts.cmake
Outdated
@@ -181,6 +181,10 @@ endif() | |||
|
|||
zephyr_file(CONF_FILES ${BOARD_EXTENSION_DIRS} DTS board_extension_dts_files) | |||
|
|||
foreach(dts_file ${board_extension_dts_files}) | |||
message(STATUS "Found board extension devicetree overlay: ${dts_file}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same overlays are also printed down here (with no indication of origin):
zephyr/cmake/modules/dts.cmake
Lines 210 to 219 in b7b3449
set(i 0) | |
foreach(dts_file ${dts_files}) | |
if(i EQUAL 0) | |
message(STATUS "Found BOARD.dts: ${dts_file}") | |
else() | |
message(STATUS "Found devicetree overlay: ${dts_file}") | |
endif() | |
math(EXPR i "${i}+1") | |
endforeach() |
If you're interested in more verbosity, then this loop should be reworked to avoid logging overlays twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I changed last minute when I was preparing this PR. I'll change this back to printing board extension dirs only.
@@ -276,6 +276,18 @@ finds in the configuration phase, like this: | |||
|
|||
-- Found devicetree overlay: .../some/file.overlay | |||
|
|||
Additional directories containing devicetree overlay files can also be added | |||
by appending to the ``BOARD_EXTENSION_DIRS`` CMake variable in the application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BOARD_EXTENSION_DIRS
is an internal variable, so it shouldn't be mentioned here.
In case you haven't seen it, we have a documented way to add a second board directory:
https://docs.zephyrproject.org/4.0.0/hardware/porting/board_porting.html#board-extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw it when I was investigating the BOARD_EXTENSION_DIRS variable indeed. It seemed a bit overcomplicated to me, I don't want make any new variant with new qualifiers, I just want to extend a board with the same name. I also think it cannot do any harm by supporting BOARD_EXTENSION_DIRS more openly. It is still possible anyway, why not document it so people can use it correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want make any new variant with new qualifiers
I understand, but we require this because we don't want the original board description to be "quietly" replaced or overridden just by including an external module in the workspace. We also don't want potential conflicts from multiple modules trying to add their own overlays etc.
It is still possible anyway, why not document it so people can use it correctly?
In this case, it's because the variable is tied to the old board extension feature, which is deprecated and will be removed together with HWMv1.
But note that the use case you alluded to in this file - setting BOARD_EXTENSION_DIRS
before find_package(Zephyr)
- is also possible in ways that we openly support, e.g., by using another variable like EXTRA_DTC_OVERLAY_FILE
, or by adding a project-specific boards/<normalized_board_target>.overlay
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case that I am dealing with is that I have the following project structure where I want to reuse dts/bindings
and boards
directory only from a directory outside of the application directory (which is in fact located inside a tests
subdirectory of the app
directory itself):
app/
├─── src/
│ └─── ...
├─── tests/
│ └─── stripped_app/
│ ├─── sysbuild/...
│ ├─── CMakeLists.txt
│ ├─── Kconfig
│ ├─── Kconfig.sysbuild
│ ├─── prj.conf
│ └─── sysbuild.conf
├─── boards/
│ ├─── redacted.conf
│ ├─── redacted.overlay
│ ├─── ...
│ ├─── nrf5340dk_nrf5340_cpuapp.conf
│ └─── nrf5340dk_nrf5340_cpuapp.overlay
├─── dts/bindings
│ └─── binding.yml
├─── sysbuild/...
├─── CMakeLists.txt
├─── Kconfig
├─── Kconfig.sysbuild
├─── prj.conf
├─── sysbuild.conf
└─── west.yml
Essentially, I want to be able to specify where that project-specific boards/
directory is and not be restricted to CMAKE_CURRENT_SOURCE_DIR
. If I have to refactor to board extension format that I can tolerate but the former is where I mostly care about. Can I do that with BOARD_ROOT and board extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like you could use APPLICATION_CONFIG_DIR
here, and/or some combination of CONF_FILE
, DTC_OVERLAY_FILE
, DTS_ROOT
, KCONFIG_ROOT
. Extending the board should not be necessary.
To make debugging why entries end up in devicetree_generated.h but do not end up in .config, this commit adds a bit more verbosity to dts.cmake by printing BOARD_EXTENSION_DIRS and DTS_ROOT binding paths. Signed-off-by: Jelle De Vleeschouwer <[email protected]>
This adds a note about BOARD_EXTENSION_DIRS to the documentation on how to set devicetree overlays. Signed-off-by: Jelle De Vleeschouwer <[email protected]>
8f84d7d
to
b0899dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not convinced we should embark on a path where we start printing a bunch of different paths from where we might be looking up files.
This would easlily spread and soon the highly important information we print today can easily be overlooked.
There are other places where this info is better available, like build_info.yml.
Also for the BOARD_EXTENSION_DIRS, a better approach has been suggested here: #83533 (comment)
@@ -222,6 +226,7 @@ unset(DTS_ROOT_BINDINGS) | |||
foreach(dts_root ${DTS_ROOT}) | |||
set(bindings_path ${dts_root}/dts/bindings) | |||
if(EXISTS ${bindings_path}) | |||
message(STATUS "Found DTS bindings dir: ${bindings_path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always challenging to decide exactly what should be printed.
We try to limit the output CMake status output to just exact files or tools included in build as those are those are most important in order to quickly discover if a wrong file (conf, dts, etc.) or version of a given tool (compiler, python, etc) is picked up, as this quickly helps users to discover when something is wrong, like env variable, paths, and the like.
You may say paths can be in similar category, but there can be a lot of paths being searched and if we start to print all of those, including those being provided by Zephyr modules, then we quickly get a lot of lines that makes it harder to quickly spot the more important information mentioned above.
If you want to see which paths were actually considered for the build, then you should take a look at <build>/build_info.yml
which contains a lot more information regarding both files and paths used during the CMake and build stages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I see. build_info.yml
seems to be fairly recent (October 8, 2024) and doesn't seem to have made it into my downstream fork (nrfconnect/[email protected]) yet, so I haven't tried it. Currently, we're upstepping to nRF Connect 2.9.0, so then I can re-evaluate. For that reason, I will close this.
Most of my issues related to devicetree generation have been due to malformed file paths, forgetting the bindings
or dts
in dts/bindings
, for example. I am a big proponent of the "The Rule of Silence" posix philosophy, but discarding devicetree entries because the binding for a node could not be found (in all the paths where it goes looking for them), I consider particularly noteworthy. So in that philosophy, that would be the ideal moment to print the binding paths. But I am also pragmatic, so without dwelling too much into the devicetree generation code, this would be, I thought, the next best opportunity.
My apologies for any inconvenience this PR may have caused and thanks a lot for your valuable feedback and time 🙂
Regards,
Jelle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but discarding devicetree entries because the binding for a node could not be found
yes, this I agree too sounds like something which should probably be printed.
Discarding an entry without informing the user is in general never nice.
I'm unsure the reason for such silent discard, but I this PR (or an alternative) can be made which informs users when entries are discarded, then that sounds like a nice improvements.
Alternative, create an enhancement issue, as that can have other people providing feedback as to why this is silently ignored.
My apologies for any inconvenience this PR
The PR had no inconveniences, only the description of the PR wasn't exactly clear that entries was silently discarded.
You surely have found something worth improving 👍
To make debugging why entries end up in devicetree_generated.h
but do not end up in .config, this commit adds a bit more
verbosity to dts.cmake by printing overlays found in
BOARD_EXTENSION_DIRS and DTS_ROOT binding paths.
This PR also adds a note about BOARD_EXTENSION_DIRS to the documentation on how to set devicetree overlays: https://docs.zephyrproject.org/latest/build/dts/howtos.html#set-devicetree-overlays