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

Kconfig: add CONFIG_BOARD_REVISION #44654

Merged

Conversation

mbolivar-nordic
Copy link
Contributor

Propagate the board revision to Kconfig via the environment.

An alternative to: #44131

@mbolivar-nordic
Copy link
Contributor Author

mbolivar-nordic commented Apr 7, 2022

To try it out, do something like this:

$ west build --cmake-only -p -b nrf9160dk_nrf9160@0.7.0 samples/hello_world/
$ grep BOARD_REVISION build/zephyr/.config
CONFIG_BOARD_REVISION="0.7.0"

Edit: and similarly,

$ west build --cmake-only -p -b nrf52dk_nrf52832 samples/hello_world/
$ grep BOARD_REVISION build/zephyr/.config
CONFIG_BOARD_REVISION=""

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

If we are going to add this as a Kconfig symbol, this would indeed be the correct thing to do.

No strong opinion on whether this should be a Kconfig symbol or just a good old #define BOARD_REVISION; though, I am inclined to say Kconfig is better since you can potentially reuse this to set the defaults for other configs.

arch/Kconfig Outdated Show resolved Hide resolved
carlescufi
carlescufi previously approved these changes Apr 7, 2022
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

I like this better.

jfischer-no
jfischer-no previously approved these changes Apr 8, 2022
nordicjm
nordicjm previously approved these changes Apr 8, 2022
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Much simpler and better!

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

what @stephanosio says...

Moving this option to the subdirectory for boards might make it easier
to find, and will keep it next to some other board-related Kconfig
options set in the same file.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Propagate the board revision to Kconfig via the environment.

This is useful for application code to have access to for similar
reasons that CONFIG_BOARD is useful.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic
Copy link
Contributor Author

@nashif I have addressed @stephanosio's comments, including adding a prep work patch moving CONFIG_BOARD to boards/Kconfig to keep everything together.

@mbolivar-nordic mbolivar-nordic merged commit c11b785 into zephyrproject-rtos:main Apr 8, 2022
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.

a late arrival comment.
FYI @stephanosio @nashif @carlescufi @mbolivar-nordic

@@ -9,6 +9,14 @@ config BOARD
if not found we look for the linker file in
soc/<arch>/<family>/<series>

config BOARD_REVISION
def_string "$BOARD_REVISION"
Copy link
Collaborator

@tejlmand tejlmand Jun 29, 2022

Choose a reason for hiding this comment

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

@mbolivar-nordic a little late to the game.

I'm sure this is useful but did you notice the fact that the BOARD Kconfig setting is not passed through the environment, but set in the board's Kconfig.

The result of that is that the BOARD value in Kconfig != BOARD value passed to CMake, see for example:

config BOARD
default "nrf5340dk_nrf5340_cpuapp" if BOARD_NRF5340DK_NRF5340_CPUAPP || BOARD_NRF5340DK_NRF5340_CPUAPP_NS

This means that user can fetch the revision for the board given with -DBOARD=<board>@<rev>, but the board available in Kconfig may not be the same board, for example:
-DBOARD=nrf5340dk_nrf5340_cpuapp_ns@1.2.3 results in:

  • CONFIG_BOARD == nrf5340dk_nrf5340_cpuapp
  • CONFIG_BOARD_REVISION == 1.2.3

so not sure passing revision this way is wise if we don't cleanup and enforce that CONFIG_BOARD is identical to the board provided by the user with -DBOARD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so not sure passing revision this way is wise if we don't cleanup and enforce that CONFIG_BOARD is identical to the board provided by the user with -DBOARD.

are you aware of any counterexamples? That would indeed seem like a bug in the board defconfig if CONFIG_BOARD ended up different than BOARD (without the revision), wouldn't you agree?

Copy link
Collaborator

@tejlmand tejlmand Aug 10, 2022

Choose a reason for hiding this comment

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

are you aware of any counterexamples? That would indeed seem like a bug in the board defconfig if CONFIG_BOARD ended up different than BOARD (without the revision), wouldn't you agree?

@mbolivar-nordic did you look at the code I linked ?

That is exactly the counter example you are requesting.

A user specifying -DBOARD=nrf5340dk_nrf5340_cpuapp_ns@0.1.2 will have the BOARD = nrf5340dk_nrf5340_cpuapp_ns in CMake, but as BOARD = nrf5340dk_nrf5340_cpuapp in Kconfig, that is without _ns in Kconfig.

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.

None yet

7 participants