-
Notifications
You must be signed in to change notification settings - Fork 1.4k
sysbuild: mcuboot: direct_xip: enable custom overlay for secondary slot #24561
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
base: main
Are you sure you want to change the base?
sysbuild: mcuboot: direct_xip: enable custom overlay for secondary slot #24561
Conversation
Added a dedicated Kconfig option to sysbuild that allows the user to set a custom overlay file for their secondary slot build configuration. If used, the custom file replaces the default DTS overlay. Signed-off-by: Kamil Piszczek <[email protected]>
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 978548904de2fced3065960569e57defa6b9c9bb more detailssdk-nrf:
Github labels
List of changed files detected by CI (2)
Outputs:ToolchainVersion: 2b2cd9579a Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
Memory footprint analysis revealed the following potential issuesapplications.hpf.gpio.icbmsg[nrf54l15dk/nrf54l15/cpuflpr]: High RAM usage: 12430[B] - link (cc: @nrfconnect/ncs-ll-ursus) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-24561/1) |
config MCUBOOT_BUILD_DIRECT_XIP_VARIANT_CUSTOM_OVERLAY | ||
string "Use a custom DTS overlay for the secondary slot image" | ||
depends on MCUBOOT_BUILD_DIRECT_XIP_VARIANT || !PARTITION_MANAGER | ||
help | ||
When this value is not populated, the default DTS overlay is used for the secondary slot. | ||
Use this Kconfig option when you want to set a custom overlay file for the secondary slot | ||
image. |
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.
this is not acceptable. If you want to append a new file then doing that should work via command line
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.
We don't want to use a CLI parameter to override the DTS overlay file because the build command becomes complex (actually, you need to replace overlays for two secondary variants of the following images: the default application image and IPC radio).
west build -p -b nrf54h20dk/nrf54h20/cpuapp -- -Dmcuboot_secondary_app_DTC_OVERLAY_FILE=secondary.overlay -Dipc_radio_secondary_app_DTC_OVERLAY_FILE=secondary.overlay
There is also a guideline/requirement from PMT to make the default build command work in a given project. So the following command should work without any extra params:
west build -p -b nrf54h20dk/nrf54h20/cpuapp
Since there is already a mechanism that injects the overlay file from an arbitrary location ( nrf/subsys/mcuboot/mcuboot_secondary_app.overlay
), can't we simply introduce the possibility to change the path and pass your own DTS overlay?
Also, right now, you can't remove the nrf/subsys/mcuboot/mcuboot_secondary_app.overlay
overlay, which is added to your secondary images automatically.
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.
@kapi-no but the sample already builds out-of-the-box, correct ?
afaict the mcuboot_secondary_app.overlay
is always applied, so what you're tryin (correct me if i'm wrong) is for users to choose their own overlay.
For this, as @nordicjm points out, sysbuild already supports the use of -D<image>_DTC_OVERLAY_FILE=<user-custom-file>.overlay
, so why not change the code into dropping add_overlay_dts(${image} "${secondary_overlay}")
if custom overlay for image is set ?
Instead of inventing Kconfig setting for this.
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.
@kapi-no but the sample already builds out-of-the-box, correct ?
In the simple MCUboot direct-xip use case scenario, where we have a default application image (built for slot0) and its secondary counterpart (built for slot1) -> yes.
However, when each slot holds two images (merged binary configuration for the nRF54H20): the default application image and the IPC radio image -> the default overlay configuration from the nrf/subsys/mcuboot/mcuboot_secondary_app.overlay
is broken.
See the following comment for more context: #24494 (comment)
afaict the
mcuboot_secondary_app.overlay
is always applied, so what you're tryin (correct me if i'm wrong) is for users to choose their own overlay.
Yes
For this, as @nordicjm points out, sysbuild already supports the use of
-D<image>_DTC_OVERLAY_FILE=<user-custom-file>.overlay
, so why not change the code into droppingadd_overlay_dts(${image} "${secondary_overlay}")
if custom overlay for image is set ?Instead of inventing Kconfig setting for this.
We always strive to have the default build command working for each supported target. Requiring extra parameters breaks this requirement.
There is also a guideline/requirement from PMT to make the default build command work in a given project. So the following command should work without any extra params:
west build -p -b nrf54h20dk/nrf54h20/cpuapp
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.
afaict the mcuboot_secondary_app.overlay is always applied, so what you're tryin (correct me if i'm wrong) is for users to choose their own overlay.
Yes
For this, as @nordicjm points out, sysbuild already supports the use of -D
_DTC_OVERLAY_FILE=.overlay, so why not change the code into dropping add_overlay_dts(${image} "${secondary_overlay}") if custom overlay for image is set ?
Instead of inventing Kconfig setting for this.
We always strive to have the default build command working for each supported target. Requiring extra parameters breaks this requirement.
but ensuring that a default build command for a given target always work is not necessarily the same as supporting custom user provided files.
Is the nrf54h20dk/nrf54h20/cpuapp
not working with the currently used / default applied mcuboot_secondary_app.overlay
file ?
In short, does west build -p -b nrf54h20dk/nrf54h20/cpuapp --sysbuild
fail today ?
There is no Jira task reference provided, so I'm a bit unsure exactly what you're trying to solve here.
- Working sample out-of-the-box for
west build -p -b nrf54h20dk/nrf54h20/cpuapp --sysbuild
?
If this is not working, then we should have an issue describe what and why this is not working. - Ability for our downstream users / customers to provide their own overlays.
Of course a user wanting to provide their own overlay must be expected to do some work of their own.
Even with the use if a Kconfig proposol in this PR they would need to set the value somehow, for example:
-DMCUBOOT_BUILD_DIRECT_XIP_VARIANT_CUSTOM_OVERLAY=<file>.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.
Generally, the ROM footprint statistics for the nRF54H20 images in the merged binary approach are incorrect. More details can be found in the following ticket:
NCSDK-35489
The code partition for the application image is defined in such a way that it also contains the radio partition.
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.
If I understand NCSDK-35489 then the issue exists for all samples building for nRF54h20dk, so isn't it simply the original mcuboot_secondary_app.overlay
intended for the nrf54h20dk which is wrong and should be fixed ?
ef85447
Secondly, if a sample wants to provide custom overlays for images and have those applied per default, then that is also possible. Can even be board specific: https://docs.zephyrproject.org/latest/build/sysbuild/index.html#zephyr-application-configuration
If none of the existing solutions are suitable, then I believe it will be more efficient if you set up a meeting and invite relevant parties.
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.
If I understand NCSDK-35489 then the issue exists for all samples building for nRF54h20dk, so isn't it simply the original mcuboot_secondary_app.overlay intended for the nrf54h20dk which is wrong and should be fixed ?
Yes, at least with the merged binary approach. I haven't tested the DFU solution with isolated binaries, as we want to use one version number for the image set (application + radio image) to minimize complexity in our project.
Secondly, if a sample wants to provide custom overlays for images and have those applied per default, then that is also possible. Can even be board specific: https://docs.zephyrproject.org/latest/build/sysbuild/index.html#zephyr-application-configuration
If none of the existing solutions are suitable, then I believe it will be more efficient if you set up a meeting and invite relevant parties.
Well, I am aware of the application configuration directory solution. We use it in the nRF Desktop app where I initially want to use the suggested change in this PR. I know I can create the configuration from scratch for both the mcuboot_secondary_app
and the ipc_radio_secondary_app
images. However, this requires copying over 1000 lines of configuration files (at least for the mcuboot_secondary_app
) to change one DTS node. And even then, the default file - nrf/subsys/mcuboot/mcuboot_secondary_app.overlay
- will be applied.
Added a dedicated Kconfig option to sysbuild that allows the user to set a custom overlay file for their secondary slot build configuration. If used, the custom file replaces the default DTS overlay.