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

sysbuild: Add b0 zip update output generation without MCUboot #15463

Merged
merged 2 commits into from
May 27, 2024

Conversation

nordicjm
Copy link
Contributor

Adds a dfu_application.zip output file which contains the update bin files when secure boot is used without MCUboot being enabled

Adds a dfu_application.zip output file which contains the update
bin files when secure boot is used without MCUboot being enabled

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm requested a review from tejlmand as a code owner May 23, 2024 11:28
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 23, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 23, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-sdk-find-my X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@nordicjm nordicjm removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 24, 2024
@@ -94,6 +94,10 @@ function(include_fw_zip)
include(${ZEPHYR_NRF_MODULE_DIR}/cmake/sysbuild/zip.cmake)
endfunction()

function(include_b0_packaging)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the creation of a function to include a CMake file is done because of scoping.

If so, then a comment would be nice, as there is really not a reason for other to look at this and think that sourcing of CMake files should be done like this in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No actually this was done because you submitted the original for include_provision_hex which did it this way (and that file actually has its own function that gets called inside of that file)

Copy link
Contributor

@tejlmand tejlmand May 27, 2024

Choose a reason for hiding this comment

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

and include_provision_hex() was exactly done like that because of scoping, because the provision_hex.cmake file imports the kconfig and thus defines several CONFIG_ settings which would be polluting the scope.

import_kconfig(CONFIG_ ${BINARY_DIR}/zephyr/.config)

But agreed, the include_provision_hex() should actually have a comment in this regard.

Generally there are no reason for such a pattern, but in the case of the provision_hex.cmake, then that file is shared between sysbuild and parent/child builds and must use common prefix for loading the kconfig into scope and thus there is a reason for including the CMake file through a function.

@kapi-no kapi-no added this to the 2.7.0 milestone May 27, 2024
Copy link
Contributor

@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.

approved for now, but comment #15463 (comment) should be addressed in follow up.

Change brings back backwards compatibility to ensure compatibility with
existing tools and applications.

Jira: NCSDK-27566

Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 27, 2024
@MarekPieta MarekPieta requested a review from tejlmand May 27, 2024 14:16
@MarekPieta
Copy link
Contributor

Pushed my backwards compatibility alignment as agreed with @carlescufi. @tejlmand, please rereview.

Copy link
Contributor

@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.

Approving and merging with the understanding that this will be reworked later:
https://nordicsemi.atlassian.net/browse/NCSDK-27610

@carlescufi carlescufi merged commit a86be6f into nrfconnect:main May 27, 2024
13 checks passed
@gchwier
Copy link
Contributor

gchwier commented May 28, 2024

Introduced regression with test: https://github.com/nrfconnect/sdk-nrf/blob/main/tests/subsys/bootloader/bl_storage/testcase.yaml
To reproduce: west build --sysbuild -p -b nrf5340dk/nrf5340/cpuapp tests/subsys/bootloader/bl_storage

Instead of Firmware version (10) is smaller than monotonic counter (11).
is received Firmware version (1) is smaller than monotonic counter (2)..

I see that it is because of setting CONFIG_FW_INFO_FIRMWARE_VERSION=1 in sysbuild/mcuboot_enable_secure_bootloader.overlay, but in project prj.conf is CONFIG_FW_INFO_FIRMWARE_VERSION=10, why it is overwritten?

@gchwier
Copy link
Contributor

gchwier commented May 28, 2024

and maybe better to change extension in mcuboot_enable_secure_bootloader.overlay to .conf, overlay is rather used to DTS files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants