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

manifest: Update sdk-zephyr revision #15530

Merged
merged 7 commits into from
Jun 15, 2024

Conversation

tejlmand
Copy link
Contributor

Update sdk-zephyr to pull in APPLICATION_CONFIG_DIR support in sysbuild.

@tejlmand tejlmand requested a review from carlescufi as a code owner May 28, 2024 07:16
@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels May 28, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 28, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@d21c2b9 nrfconnect/sdk-zephyr@e4277b0 (main) nrfconnect/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 28, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
desktop52_verification X
test-fw-nrfconnect-apps X
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_mesh X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-boot X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-nfc X
test-fw-nrfconnect-nrf-iot_cloud X
test-fw-nrfconnect-nrf-iot_libmodem-nrf X
test-fw-nrfconnect-nrf-iot_lwm2m X
test-fw-nrfconnect-nrf-iot_mosh X
test-fw-nrfconnect-nrf-iot_nrf_provisioning X X
test-fw-nrfconnect-nrf-iot_positioning X
test-fw-nrfconnect-nrf-iot_samples X
test-fw-nrfconnect-nrf-iot_serial_lte_modem X
test-fw-nrfconnect-nrf-iot_thingy91 X
test-fw-nrfconnect-nrf-iot_zephyr_lwm2m X
test-fw-nrfconnect-nrf_crypto X
test-fw-nrfconnect-proprietary_esb X
test-fw-nrfconnect-rpc X
test-fw-nrfconnect-rs X
test-fw-nrfconnect-tfm X
test-fw-nrfconnect-thread X
test-fw-nrfconnect-zigbee X
test-low-level X
test-sdk-audio X
test-sdk-dfu X
test-sdk-find-my X
test-sdk-mcuboot X
test-sdk-pmic-samples X
test-sdk-sidewalk X
test-sdk-wifi X

All integration tests: null

Detailed information of selected test modules

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

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from a464e09 to 39b62dd Compare May 29, 2024 13:55
@kapi-no kapi-no added this to the 2.7.0 milestone Jun 4, 2024
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from 39b62dd to 60f41dc Compare June 6, 2024 10:10
@tejlmand tejlmand requested a review from nordicjm as a code owner June 6, 2024 10:10
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch 5 times, most recently from bfa6607 to c6be226 Compare June 7, 2024 11:52
@tejlmand tejlmand requested a review from ludvigsj as a code owner June 7, 2024 11:52
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from c6be226 to 19b3e7f Compare June 10, 2024 08:49
Copy link
Contributor

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

boot CI tests failing, FEM CI tests failing, crypto CI tests failing, rs CI tests failing, tfm CI tests failing, dfu CI tests failing, find-my CI tests failing, mcuboot CI tests failing. Needs to be fixed before merge

@kapi-no
Copy link
Contributor

kapi-no commented Jun 11, 2024

Find My tests CI failed due to the JUnit parser instability. This failure is irrelevant and could potentially be resolved by the Find My CI rerurn. @miha-nordic, please correct me if I am wrong

@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch 2 times, most recently from cd4ca42 to c599e9b Compare June 12, 2024 12:22
@nordicjm
Copy link
Contributor

./sysbuild/application/zephyr/net_provision.hex
./sysbuild/application/zephyr/signed_by_b0_hci_ipc.hex
./sysbuild/application/zephyr/signed_by_mcuboot_and_b0_hci_ipc.hex

In wrong locations. Also this has the sysbuild prefix so doing ninja sys and pressing tab, instead of going to sysbuild_ and showing menuconfig and guiconfig, shows over 180 files in the sysbuild folder which was discussed, the folder should have a prefix so it does not appear like that

@tejlmand
Copy link
Contributor Author

tejlmand commented Jun 12, 2024

In wrong locations.

@nordicjm If you think those are in wrong location, then please describe why you would argue they should be at top-level build folder.
The introduction of nrfconnect/sdk-zephyr#1746 means we can avoid populating the top-level dir with a bunch of files, instead we should place files on that level if there are good reasons.

Currently the dfu_application.zip makes sense to have at that level.
Same can be said for merged.hex, as that file might be flashed manually by test systems.

but afaik the files you mention are merged into other files and not intended to be used independently.

in the sysbuild folder which was discussed, the folder should have a prefix so it does not appear like that

I'm open to suggestions, but making just one more folder level like <build>/share/sysbuild/... just for this is not really beneficial.
Just makes it even harder to browse the build dir.

Options I can think of are:

  • _sysbuild (instead of sysbuild), but that also makes it look like it's private or similar, and not really what we mean.
  • sb, would require that we reserve sb in addition to sysbuild as restricted app names.

@nordicjm
Copy link
Contributor

but afaik the files you mention are merged into other files and not intended to be used independently.

Only the provision file is like that, the rest are fully used and expected to be available. And the net provision should either be in the top level or in the folder for the application it is used for. signed_by_b0_hci_ipc.hex/bin is something customers can use to update if they do not use MCUboot to load the image or if they load it through their application (as nrf_desktop does). signed_by_mcuboot_and_b0_hci_ipc.hex/bin is used for updating from the application using MCUmgr, or from MCUboot using serial recovery

_sysbuild (instead of sysbuild), but that also makes it look like it's private or similar, and not really what we mean.

Gets my vote and is simple.

@tejlmand
Copy link
Contributor Author

And the net provision should either be in the top level or in the folder for the application it is used for.

That's a good point. Will move to top-level, although image specific build folder might actually be more appropriate then I don't like sysbuild modules to start placing extra files in a build folder controlled by the image build.

the rest are fully used and expected to be available. ... is something customers can use to update if they do not use MCUboot to load the image or if they load it through their application

will move to top-level as well, but this comment made me wonder if we are actually documenting this.
Both in term of which build files can be expected to be available and where, as well as how to use those file when they are not using MCUboot.

Gets my vote and is simple.

👍

@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from c599e9b to d7ddce3 Compare June 12, 2024 19:58
@mkapala-nordic
Copy link
Contributor

Also it seems that the dfu_application.zip generated when B0 is enabled is in different directory than dfu_application.zip when MCUboot is enabled:

build_52_b0/zephyr/dfu_application.zip <- I think it should be up a folder
build_52_mcuboot/dfu_application.zip

It seems that nrf/cmake/sysbuild/b0_packaging.cmake is not yet aligned

@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from 877d454 to 9e57114 Compare June 13, 2024 12:21
@tejlmand
Copy link
Contributor Author

Still seeing them in wrong folder:

Should be fixed now.

Also it seems that the dfu_application.zip generated when B0 is enabled is in different directory than dfu_application.zip when MCUboot is enabled:

Fixed

@nordicjm
Copy link
Contributor

wrong folder:

./zephyr/dfu_mcuboot.zip   <-- should be up a folder

Matter is broken, samples/matter/light_bulb build for nrf5340dk/nrf5340/cpuapp:

ninja: error: 'zephyr/signed_by_mcuboot_and_b0_ipc_radio.bin', needed by 'dfu_application.zip', missing and no known rule to make it

@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from 9e57114 to 551dd68 Compare June 13, 2024 14:20
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from 551dd68 to 39bccc9 Compare June 13, 2024 18:50
@tejlmand
Copy link
Contributor Author

@nordicjm PTAL, all is fixed.
CI issues are caused by existing failures on main.

@nordicjm
Copy link
Contributor

wrong folder:

./zephyr/dfu_mcuboot.zip   <-- should be up a folder

@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from 39bccc9 to a49e6bd Compare June 14, 2024 08:09
@doublemis1
Copy link
Contributor

@nordicjm @tejlmand
Why is the milestoneset to NCS 2.7.0?
Is it some crucial fix?

Copy link
Contributor

@doublemis1 doublemis1 left a comment

Choose a reason for hiding this comment

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

Please remove NCS 2.7.0 milestone since it is not a bugfix.

@tejlmand
Copy link
Contributor Author

@nordicjm @tejlmand Why is the milestoneset to NCS 2.7.0? Is it some crucial fix?

Yes it is:
#15857 (comment)

If we want to maintain the production quality of the nRF Desktop app, we need this change that is blocked on the @tejlmand PR (dependency indicated in the PR description). Alternatively, we can lower the nRF Desktop quality from production to experimental following the VS code approach. However, going back to the experimental quality for NCS 2.7.0 needs wider discussion from relevant stakeholders.

On a separate note, we had a different arrangement with @carlescufi and @tejlmand, as this change was supposed to be part of NCS 2.7.0.

@doublemis1
Copy link
Contributor

doublemis1 commented Jun 14, 2024

@nordicjm @tejlmand Why is the milestoneset to NCS 2.7.0? Is it some crucial fix?

Yes it is: #15857 (comment)

If we want to maintain the production quality of the nRF Desktop app, we need this change that is blocked on the @tejlmand PR (dependency indicated in the PR description). Alternatively, we can lower the nRF Desktop quality from production to experimental following the VS code approach. However, going back to the experimental quality for NCS 2.7.0 needs wider discussion from relevant stakeholders.
On a separate note, we had a different arrangement with @carlescufi and @tejlmand, as this change was supposed to be part of NCS 2.7.0.

Please add bugfix label and report to @shanthanordic
Also good to have a impact on other projects since it touches build system in last days of bugfixing.

@doublemis1 doublemis1 self-requested a review June 14, 2024 09:57
@doublemis1 doublemis1 dismissed their stale review June 14, 2024 09:57

Discussed on DM

partition_manager.cmake generates the pm.config files under the
APPLICATION_BINARY_DIR folder.
Update fast_pair.cmake to use APPLICATION_BINARY_DIR instead of
CMAKE_BINARY_DIR so that location in fast_pair.cmake is aligned
to the location used in partition_manager.cmake.

Signed-off-by: Torsten Rasmussen <[email protected]>
Update zip.cmake, packaging.cmake and b0_packaging.cmake to reference
bin files using absolute paths. Also update the location of the
dfu_application.zip to be at the CMAKE_BINARY_DIR instead of the
PROJECT_BINARY_DIR for images' bin-files.

Using CMAKE_BINARY_DIR ensures that the zip file is always placed at the
toplevel in the build directory.

Use CMAKE_BINARY_DIR for signed_by bin-files to align the absolute
path and ensure b0_mcuboot_signing.cmake uses same path.

Signed-off-by: Torsten Rasmussen <[email protected]>
Update provision_hex.cmake and sign.cmake to use absolute path to the
application's .config file to ensure correct dependency handling.

Signed-off-by: Torsten Rasmussen <[email protected]>
kconfig.cmake generates `.config` as `${PROJECT_BINARY_DIR}/.config`.
Adjust `${CMAKE_BINARY_DIR}/zephyr/.config` to
`${PROJECT_BINARY_DIR}/.config`.

Signed-off-by: Torsten Rasmussen <[email protected]>
partition_manager.cmake creates `merged.hex` as
`${PROJECT_BINARY_DIR}/merged.hex`.

Adjust `${PROJECT_BINARY_DIR}/merged.hex` to
`${CMAKE_BINARY_DIR}/merged.hex` so that external tools / tests / etc.
which looks for `merged.hex` at top-level build folder can still find
it.

Signed-off-by: Torsten Rasmussen <[email protected]>
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from a49e6bd to d4d2f2c Compare June 14, 2024 17:44
@NordicBuilder NordicBuilder removed the DNM label Jun 14, 2024
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from d4d2f2c to 1e832d0 Compare June 14, 2024 21:10
Update sign.cmake and debug_keys.cmake to use CMAKE_BINARY_DIR instead
of PROJECT_BINARY_DIR.

Using CMAKE_BINARY_DIR ensures that the pem files are always placed at
the toplevel in the build directory.

Signed-off-by: Torsten Rasmussen <[email protected]>
Update sdk-zephyr to pull in APPLICATION_CONFIG_DIR support in sysbuild.

Signed-off-by: Torsten Rasmussen <[email protected]>
@tejlmand tejlmand force-pushed the sysbuild/application_config_dir branch from 1e832d0 to 21a187b Compare June 14, 2024 21:17
@anangl anangl merged commit 10ebc98 into nrfconnect:main Jun 15, 2024
44 of 47 checks passed
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. manifest manifest-zephyr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants