Skip to content

hwmv2: Fix problems with common board functionality #70548

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

Closed

Conversation

nordicjm
Copy link
Collaborator

@nordicjm nordicjm commented Mar 21, 2024

This change reverts a change that was introduced with hwmv2 which
allowed for using common board names for overlays, given the board
target ``[email protected]/nrf9160/ns``.

In hwmv1 this would have used:
    ``nrf9160dk_nrf9160_ns.conf``.
    ``nrf9160dk_nrf9160_ns_0_7_0.conf``.

In hwmv2 this would have used:
    ``nrf9160dk_nrf9160_ns.conf``
    ``nrf9160dk_nrf9160_ns_0_7_0.conf``
    ``nrf9160dk_nrf9160.conf``
    ``nrf9160dk_nrf9160_0_7_0.conf``
    ``nrf9160dk.conf``
    ``nrf9160dk_0_7_0.conf``

With these changes, the following are used (which restores the hwmv1
behaviour):
    ``nrf9160dk_nrf9160_ns.conf``
    ``nrf9160dk_nrf9160_ns_0_7_0.conf``

For a board with a default SoC which is not a variant for example
``rpi_pico`` then ``rpi_pico.conf`` will also be used, this file
will not be used for variants e.g. ``rpi_pico/rp2040/w``

This applies to .dts, .conf and .overlay files in the boards
directory, and to .conf and .overlay files in application board
overlay directories.

This revert is needed to avoid issues whereby variants have
incompatible configuration to the parent board target, which has
been affecting samples and tests.


cmake: Deprecate board files that do not have SoC in the name

Deprecated .dts, .overlay and .conf files that have the name of the
board itself but without the name of the SoC in them, emits cmake
deprecation notices upon such files being found.

Future plans for missing functionality (provisional)

We have internally discussed the following which could be added in future to enhance the features removed/changed here:

  • Having twister support short board names as cmake/west build does, by having it use the build system to determine if a name is valid
  • Having common file support but marking the files explicitly as common files, the current idea is to use a suffix in the filename so nrf5340dk_nrf5340_cpuapp.conf will only apply to nrf5340dk/nrf5340/cpuapp but something like nrf5340dk_nrf5340_cpuapp.all.conf could apply to this board target and all variants: nrf5340dk/nrf5340/cpuapp and nrf5340dk/nrf5340/cpuapp/ns

Fixes #70445
Fixed #70283

@nordicjm nordicjm force-pushed the hwmv2-revert-common-board branch 2 times, most recently from 9c6d767 to 06808b3 Compare March 21, 2024 10:52
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.

Perhaps better to issue a warning, while still have temp support for legacy named overlays (and conf files)

Comment on lines 1602 to 1619
list(LENGTH str_segment_list str_segment_list_len)
if("${str_segment_list_len}" EQUAL "2")
# Include board name without SoC
list(APPEND ${outvar} "${BUILD_STR_BOARD}")
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

initially I'm not sure we should change the behavior of MERGE.
Instead we should probably avoid using MERGE in this case, and instead update code here:

if(NOT DEFINED DTC_OVERLAY_FILE)
zephyr_build_string(board_overlay_strings
BOARD ${BOARD}
BOARD_IDENTIFIER ${BOARD_IDENTIFIER}
MERGE
)
list(TRANSFORM board_overlay_strings APPEND ".overlay")
zephyr_file(CONF_FILES ${APPLICATION_CONFIG_DIR} DTS DTC_OVERLAY_FILE
NAMES "${APP_BOARD_DTS};${board_overlay_strings};app.overlay" SUFFIX ${FILE_SUFFIX})
endif()

That code can then do something like:

  zephyr_build_string(board_overlay_string
                      BOARD ${BOARD}
                      BOARD_IDENTIFIER ${BOARD_IDENTIFIER}
  )
  set(board_overlay_string "${board_overlay_string}.overlay")
  if(NOT EXISTS "${board_overlay_string}")
    string(REGEX REPLACE" "^[^/]*/" "" legacy_board_identifier "${BOARD_IDENTIFIER}"
​    zephyr_build_string(legacy_board_overlay_string
                        BOARD ${BOARD}
                        BOARD_IDENTIFIER ${legacy_board_identifier}
    )
    set(legacy_board_overlay_string "${legacy_board_overlay_string}.overlay")
    if(EXISTS "${legacy_board_overlay_string}.overlay")
        message(WARNING Overlay file using legacy board target file name structure (${legacy_board_overlay_string}) detected.
         Rename overlay to: ${board_overlay_string})
        set(board_overlay_string ${legacy_board_overlay_string})
    endif()
  endif()
  
  zephyr_file(CONF_FILES ${APPLICATION_CONFIG_DIR} DTS DTC_OVERLAY_FILE
              NAMES "${APP_BOARD_DTS};${board_overlay_strings};app.overlay" SUFFIX ${FILE_SUFFIX})

and do similar for conf files.

After two releases, then warning code can be removed.
(We can already place the warning code in deprecated.cmake now, but that's outside the point of the example given)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The changes here also apply to board files (i.e. in boards), I don't remember if it was raised an issue on github or discord or internally but an issue was raised about common files setting things that then conflicted with configuration for variants, one was for trustzone, another was for qemu, the changes here are intentionally applied to the base function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes here also apply to board files (i.e. in boards)

Yes, and that would need to be handled in the lines just above:

zephyr_file(CONF_FILES ${APPLICATION_CONFIG_DIR}/boards DTS APP_BOARD_DTS SUFFIX ${FILE_SUFFIX})

and zephyr_file() should no longer apply MERGE, but do identical to Zephyr v3.6.0, which was:

zephyr_build_string(filename
BOARD ${FILE_BOARD}
BOARD_REVISION ${FILE_BOARD_REVISION}
BUILD ${FILE_BUILD}
)

instead of current main:

MERGE REVERSE

Of course the change to use BOARD_QUALIFIERS after v3.6.0 should be kept.
Only MERGE to be removed.

Copy link
Collaborator Author

@nordicjm nordicjm Mar 21, 2024

Choose a reason for hiding this comment

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

I don't think this should be done, taking the rpi_pico board, that is the board name, there is no other soc for this device and the twister identifier for this in hwmv2 is as it was in hwmv1: rpi_pico, the w variant had to have the name updated to rpi_pico/rp2040/w because that is required for variants but this would be a drastic change which then would invalidate (deprecate) all board names for in-tree boards (which are also used throughout tests and samples) which do not have the soc in the name. I would like @nashif to give feedback on if this is acceptable. So lots of board yaml files would need updating, lots of sample.yaml and testcase.yaml files would need updating and lots of overlay/Kconfig fragements would need renaming

Copy link
Collaborator

Choose a reason for hiding this comment

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

but this would be a drastic change which then would invalidate (deprecate) all board names for in-tree boards (which are also used throughout tests and samples) which do not have the soc in the name.

We need to make it clear how board overlays are being picked up.
Either always merge, as we do now, which clearly have drawbacks.
Or only used the normalized board target name, where we don't merge at all, but allow legacy format for a given time (that is file names without the SoC qualifier).

A combination where you would just allow SoC to be omitted but require the CPU set and variant parts will create a mess and also have edge-cases to deal with in future. (should a file be name plank__for.overlay (equivalent to BOARD=plank//foo) or just plank_foo.overlay ?

The possibility to omit SoC on command line invocation is a user convenience, but there is no reason why a fixed file name should be allowed to do so, because naming a file is a one time operation.

We seem to have accepted that the ability to inherit overlay files was a feature with undesired side-effects, and therefore long term there is no reason to allow different ways to name the same file.

Of course we don't need to print the warning immediately, but can allow a longer grace period for transitioning the overlay file names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not part of the PR (yet) just this discussion thread, want to ensure people are OK with it being added before working on it

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a usual RFC would be more appropriate to get feedback.
This should sound obvious to you and you probably want to move fast but we're all working on other stuff and we're not up to speed with daily hwmv2 evolutions.

Copy link
Member

Choose a reason for hiding this comment

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

So, on my side, I'm finding it confusing that we could have targets and twister references that can differ. I expect lot of confusion here, so I'm not in favor of it. What are the options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

5th commit here shows the changes

Copy link
Member

Choose a reason for hiding this comment

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

I agreed with @erwango and I'm favor to get the things without any confusing. This means that I would prefer go verbose and have a clear picture about how the overlays etc are selected and how is the order of "merging" content. I don't want to try guess how that could possible work, it should be precise and clear since beginning.

Maybe it could be a little overkill for single core cpu but the complexity grows to much for Multi-core Asymmetric SoC's with TFM for instance. BTW, this new model may allow the development of SOM (Systems On Module) and than we can have multi CPUs etc.

So, if it is necessary to drop complete the hwmv1 compatibility I'm in favor of that too. I think this is a big change and since we are moving to a LTS I prefer not think about hwmv1 anymore, if that is necessary to have a very clear procedure.

@nordicjm nordicjm force-pushed the hwmv2-revert-common-board branch from 50d31d0 to 63a3f2d Compare March 21, 2024 12:35
@nordicjm nordicjm requested a review from tejlmand March 21, 2024 12:55
@nordicjm nordicjm force-pushed the hwmv2-revert-common-board branch from 63a3f2d to 7676b28 Compare March 22, 2024 08:34
@nordicjm nordicjm requested a review from nandojve as a code owner March 22, 2024 08:34
@nordicjm nordicjm force-pushed the hwmv2-revert-common-board branch 5 times, most recently from 5591df4 to 04e8d38 Compare March 25, 2024 08:42
nordicjm added 5 commits April 4, 2024 00:04
This change reverts a change that was introduced with hwmv2 which
allowed for using common board names for overlays, given the board
target ``[email protected]/nrf9160/ns``.

In hwmv1 this would have used:
    ``nrf9160dk_nrf9160_ns.conf``.
    ``nrf9160dk_nrf9160_ns_0_7_0.conf``.

In hwmv2 this would have used:
    ``nrf9160dk_nrf9160_ns.conf``
    ``nrf9160dk_nrf9160_ns_0_7_0.conf``
    ``nrf9160dk_nrf9160.conf``
    ``nrf9160dk_nrf9160_0_7_0.conf``
    ``nrf9160dk.conf``
    ``nrf9160dk_0_7_0.conf``

With these changes, the following are used (which restores the hwmv1
behaviour):
    ``nrf9160dk_nrf9160_ns.conf``
    ``nrf9160dk_nrf9160_ns_0_7_0.conf``

For a board with a default SoC which is not a variant for example
``rpi_pico`` then ``rpi_pico.conf`` will also be used, this file
will not be used for variants e.g. ``rpi_pico/rp2040/w``

This applies to .dts, .conf and .overlay files in the boards
directory, and to .conf and .overlay files in application board
overlay directories.

This revert is needed to avoid issues whereby variants have
incompatible configuration to the parent board target, which has
been affecting samples and tests.

Signed-off-by: Jamie McCrae <[email protected]>
This reverts commit 037a3b5.

Signed-off-by: Jamie McCrae <[email protected]>
Splits configuration up that was merged as part of hwmv2 due to
the merged configuration feature being reverted

Signed-off-by: Jamie McCrae <[email protected]>
@tejlmand tejlmand force-pushed the hwmv2-revert-common-board branch from 3068391 to c0d648a Compare April 3, 2024 22:07
nordicjm added 9 commits April 4, 2024 09:09
Adds the SoC name to files

Signed-off-by: Jamie McCrae <[email protected]>
Adds the SoC name to files

Signed-off-by: Jamie McCrae <[email protected]>
Adds the SoC name to files

Signed-off-by: Jamie McCrae <[email protected]>
Adds the SoC name to the unit testing board

Signed-off-by: Jamie McCrae <[email protected]>
Renames boards to use identifiers which have the SoC name in, also
fixes some test issues that were noticed

Signed-off-by: Jamie McCrae <[email protected]>
Updates rst files to include renamed files with SoC names in them

Signed-off-by: Jamie McCrae <[email protected]>
Updates MCUboot board names to account for hwmv2 changes

Signed-off-by: Jamie McCrae <[email protected]>
Deprecated .dts, .overlay and .conf files that have the name of the
board itself but without the name of the SoC in them, emits cmake
deprecation notices upon such files being found.

Signed-off-by: Jamie McCrae <[email protected]>
Signed-off-by: Torsten Rasmussen <[email protected]>
Removes information on common includes as this feature has been
removed

Signed-off-by: Jamie McCrae <[email protected]>
@tejlmand tejlmand force-pushed the hwmv2-revert-common-board branch from c0d648a to fdcf7d7 Compare April 4, 2024 07:11
Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev left a comment

Choose a reason for hiding this comment

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

Is there a chance to avoid such board renames?

-rename from boards/snps/hsdk/hsdk_defconfig
+rename to boards/snps/hsdk/hsdk_arc_hsdk_defconfig

-rename from boards/snps/hsdk4xd/hsdk4xd_defconfig
+rename to boards/snps/hsdk4xd/hsdk4xd_arc_hsdk4xd_defconfig

New names look really weird:
hsdk_arc_hsdk
hsdk4xd_arc_hsdk4xd

@carlescufi
Copy link
Member

carlescufi commented Apr 4, 2024

Is there a chance to avoid such board renames?

@evgeniy-paltsev there is, but we decided not to go with it for the time being to avoid adding more complexity to this already complex PR. See the last slide in my presentation here:
https://docs.google.com/presentation/d/1BCeKKhs3BV5EciA5cQrbEfMexOjWsHyj-dgcB0yJrbQ/edit#slide=id.g2c87032e9ae_0_39

This was discussed in the Architecture WG and in a separate Discord thread: https://discord.com/channels/720317445772017664/1224744062687641622/1224744064168235028

Your SoC is named arc_hsdk and you board hsdk, that's why it looks weird. But having <board>_<soc> should not be an issue in general.

See this issue for more details: #71122

@tejlmand
Copy link
Collaborator

tejlmand commented Apr 5, 2024

Is there a chance to avoid such board renames?

There is, and it's part of this PR.
But at the latest Arch WG meeting it was decided to go for a full rename in Zephyr tree, and deprecate the short form.

If you don't rename hsdk4xd_defconfig, then this is what you'll see during build:

$ cmake -GNinja -DBOARD=hsdk4xd/arc_hsdk4xd -Bbuild samples/hello_world/
Loading Zephyr default modules (Zephyr repository).
-- Application: /projects/github/ncs/zephyr/samples/hello_world
...
-- Board: hsdk4xd, qualifiers: arc_hsdk4xd
...
CMake Deprecation Warning at /projects/github/ncs/zephyr/cmake/modules/FindDeprecated.cmake:240 (message):
  defconfig files (hsdk4xd_defconfig) without the SoC name are deprecated
  after Zephyr 3.6, you should use <board>_<soc>_defconfig instead
Call Stack (most recent call first):
  /projects/github/ncs/zephyr/cmake/modules/extensions.cmake:2709 (find_package)
  /projects/github/ncs/zephyr/cmake/modules/kconfig.cmake:78 (zephyr_file)
  /projects/github/ncs/nrf/cmake/modules/kconfig.cmake:29 (include)
  /projects/github/ncs/zephyr/cmake/modules/zephyr_default.cmake:132 (include)
  /projects/github/ncs/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /projects/github/ncs/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:117 (include_boilerplate)
  CMakeLists.txt:5 (find_package)


Parsing /projects/github/ncs/zephyr/samples/hello_world/Kconfig
Loaded configuration '/projects/github/ncs/zephyr/boards/snps/hsdk4xd/hsdk4xd_defconfig'
Merged configuration '/projects/github/ncs/zephyr/samples/hello_world/prj.conf'
Configuration saved to '/projects/github/ncs/zephyr/build/zephyr/.config'
...

So we could simply remove the deprecation part (and the renames).
For the CMake part, I would move some code around, because the current deprecated code is now moved to the deprecated modules.

And if short form is not deprecated anymore, then we should probably look up both names and only print a warning / error if both the long and the short form is present at same time.

Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev left a comment

Choose a reason for hiding this comment

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

Hi,

I've checked the changes in detail - looks like I was confused and we are only changing board files names, but the board itself still would be accessible with the previous name.
In that case I don't have anything against the change.

Sorry for the noise.

@nordicjm nordicjm closed this Apr 25, 2024
@nordicjm nordicjm deleted the hwmv2-revert-common-board branch July 12, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done