Support SoC and board extensions

[tejlmand]

Fixes: https://github.com/zephyrproject-rtos/zephyr/issues/69548 Fixes: https://github.com/zephyrproject-rtos/zephyr/issues/72374

This PR introduces support for extending boards and SoC with new variants oot.

Scope reduction in this PR compared to #69548:

• Thus it must be possible to describe when a variant want to apply base variant overlay and conf files. This feature part will be addressed as follow-up PR.

[nashif]

nice. would be great if this feature is presented in the arch wg to get more feedback and input.

[henrikbrixandersen]

@PerMac Couldn't this be used for addressing https://github.com/zephyrproject-rtos/zephyr/issues/73421 as well?

[PerMac]

@PerMac Couldn't this be used for addressing #73421 as well?

I will sync with @tejlmand to get a better understanding.

[tejlmand]

Can also seemingly extend a board twice, which is invalid, by duplicating the extended board, the output gives this:

Fixed, an error will now be printed: https://github.com/zephyrproject-rtos/zephyr/blob/41561f164a8f7bc1e697fd1f7942700dd4b54854/scripts/list_boards.py#L294-L297

[tejlmand]

an extended board cannot change the west flash runner config it seems because it looks for that in scripts/west_commands/run_common.py:

Correct, this use-case is not described in https://github.com/zephyrproject-rtos/zephyr/issues/72374 or https://github.com/zephyrproject-rtos/zephyr/issues/69548 and therefore not part of this PR.

Which does not support using the extended board config but seems like something that should be used also (if the file exists in the extended board folder, if not it uses the original board file if it exists, otherwise the soc file)

This would be a too simplified solution, as it would expect that a board can only be extended once. If we want to allow a user to overwrite or extend the runners oot, then we should actually have a dedicated issue opened, so that it can be properly discussed how such extension is expected to work for runners section.

Just following the proposal in the comment will not work in case a board is extended twice. Instead it should be considered if an extended board should be able to extend the runners section with additional information, but that would require merging of runner information from multiple sources.

Or, if we decide to just load the runners information once, then we would need to discuss and decide who takes precendence when a board is extended multiple times.

Or should it be so that the extended runners section is only loaded if the extended board is used. Note, such solution would require that we keep track of the location where the extended board is defined.

Due to above, I think extension of runners deserves its own issue and PR, so that a proper solution can be made, and therefore is outside the scope of this PR.

[nordicjm]

It needs to be part of the base PR, a feature cannot be added that allows extending SoCs and boards if a base feature (flash runner configuration) is entirely unable to be extended by that feature, an issue can be opened specifically for it for discussion purposes but it must be completely implemented in the original feature PR.

[tejlmand]

It needs to be part of the base PR

I disagree. The flash runner feature itself was made after the HWMv2 itself, and similar can extension of flash runners be made after extension of boards. Flash runners are a west feature aimed at flashing, whereas board and soc extensions are aimed at building images.

an issue can be opened specifically for it for discussion purposes but it must be completely implemented in the original feature PR.

An issue should be opened for dedicated discussion. Discussing the flash runner extension in this PR just makes noise.

[57300]

breaks testing on v1 boards

@PerMac Thanks for catching this. I can push a fix here while @tejlmand is on vacation. I'm surprised to find multiple instances of the missing Board.dir -> Board.directories rename.

[PerMac]

@57300 maybe then it would be safer to "revert" change from dir to directories in this PR if there can be more misaligned places?

[57300]

@57300 maybe then it would be safer to "revert" change from dir to directories in this PR if there can be more misaligned places?

The rename is still useful IMO (it was my suggestion) as it matches the BOARD_DIRECTORIES CMake variable. But BOARD_DIR is still defined in CMake as well, so I'm thinking of adding this to the Board class:

@property
def dir(self):
    return self.directories[0]

Then the changes in other scripts can be reverted.

[teburd]

I don't see any docs in the PR explaining how this works or what I'd need to do to try it out. How would I go about trying this out?

[github-actions[bot]]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[nashif]

This has been open for months without activity at all and without further reviews, so it was not clear where it was going. We need more reviews here and testing before it can be merged and for 4.0, this is just too late as the PR basically changed status without any warning at the very last minute.

My preference here is to give time for this to be reviewed and merge it after 4.0 is released. I will not have time to review this before RC1, although I will be trying.

[tejlmand]

This has been open for months without activity at all and without further reviews, so it was not clear where it was going.

@nashif the only thing which has happened is a rebase, there has been no functional changes because it has been ready. The issues seen in CI was minor and due to some list_boards calls in docs which was added between last rebase and the newest rebase.

this is just too late as the PR basically changed status without any warning at the very last minute.

which status change are you referring to ?

[nordicjm]

I have checked this out this morning by adding an extension to nrf5340dk then checking that the extended board was used and that the flash runner configuration from the extended board is used

[nashif]

which status change are you referring to ?

status change from what seemed to be a completely abandoned PR with pending change requests (stale bot marked it stale just 5 days ago), to approved at the last minute before feature freeze. This is basic infrastructure change and needs further reviews/testing by others.

[kartben]

FWIW I agree with @nashif and was about to comment something similar. While documentation for this could be added later, the fact that @teburd's comment hasn't been addressed is IMO a sign that this might not be ready just yet.

To put it differently, for this to be reviewed effectively, it needs to be something that can be reviewed from a user perspective, not from a Python/Kconfig/Cmake code perspective. I, for one, would like to provide feedback on the overall user experience for this new (very much welcome, don't get me wrong) feature, and I simply wouldn't know where to start

[tejlmand]

While documentation for this could be added later, the fact that @teburd's comment hasn't been addressed is IMO a sign that this might not be ready just yet.

sorry, I had missed that comment. Docs are here: https://github.com/zephyrproject-rtos/zephyr/pull/73409

[nashif]

sorry, I had missed that comment.

I do not blame you, all I can see in this PR is:

[carlescufi]

sorry, I had missed that comment.

I do not blame you, all I can see in this PR is:

@nashif Yes, we are addressing this as we speak, you will no longer see those messages in upstream PRs. Sorry for the noise.

[tejlmand]

@nashif Yes, we are addressing this as we speak, you will no longer see those messages in upstream PRs. Sorry for the noise.

at least this long and horrible list of noise :see_no_evil: shows that the feature is cherry-picked into NCS because it's used downstream. Thus it's actively being tested, besides the twister tests included in the PR.

[tejlmand]

pending change requests

those change requests was actually addressed months ago, but true, I didn't followup with reviewers to get final approvals.

[nashif]

at least this long and horrible list of noise 🙈 shows that the feature is cherry-picked into NCS because it's used downstream. Thus it's actively being tested, besides the twister tests included in the PR.

i have no idea what that is and should not be relying on this data to make judgment about the state of a PR and its level of testing and maturity. I am sure it works for you, but this a project feature and not a nordic only feature and a core one that requires more reviews and testing.

[tejlmand]

but this a project feature and not a nordic only feature and a core one that requires more reviews and testing.

and it has been developed as a project feature, not a Nordic feature. But project features are developed because they are useful, and being useful sometimes also means useful to Nordic. And in this case cherry-picked into Nordics fork.

Nordic doesn't require this PR for 4.0.0, because we already have it cherry-picked, but the feature can be very useful to other users / companies, and for that I think it is beneficial if it is part of 4.0.0.

As side-note what testing are reviewers doing on build system PRs ? I rarely find people who actually tests build system features in depth before they are merged.

Even good reviews on the build system itself is hard to get :disappointed: .

If you mean more testing on main, before being part of a release, then that still requires someone downstream of Zephyr repo to use this feature, because Zephyr main itself will not use this feature. The feature allows Zephyr downstream users to extend upstream boards. That's one reason that I think it's worth mentioning that it is being used by Nordic, because it means the use is actually verified.

why is this in a different PR? This should come as part of the same PR and go in together.

Simply because I often experience that you get approvals on the implementation, just to get change request on docs afterwards, resulting in having to again ping reviewers to get re-approvals. Or vice-versa.

Having two PRs avoids approvals for the implementation to be cleared when having to push change requests on the doc set.

[tejlmand]

why is this in a different PR? This should come as part of the same PR and go in together.

doc commit cherry-picked into this PR.

[carlescufi]

@kartben regardless of when this gets merged, are you happy with the doc?