suit: recovery application build separately #15422

ahasztag · 2024-05-21T14:54:32Z

This PR adds the SUIT BLE recovery application.

The application is build separately and than flashed after the main application is flashed.

test-sdk-nrf: sdk-nrf-pr-15422

NordicBuilder · 2024-05-21T14:56:40Z

Test specification

CI/Jenkins/NRF

Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
test-fw-nrfconnect-chip	X
test-sdk-dfu	X
test-sdk-find-my	X

Detailed information of selected test modules

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

NordicBuilder · 2024-05-21T16:24:11Z

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

Name	Old Revision	New Revision	Diff
suit-generator	nrfconnect/suit-generator@`2b796f1`	nrfconnect/suit-generator@`93f1fbb` (ncs)	nrfconnect/[email protected]

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

cmake/sysbuild/suit.cmake

samples/suit/recovery/CMakeLists.txt

samples/suit/recovery/Kconfig

samples/suit/recovery/Kconfig.sysbuild

samples/suit/recovery/prj.conf

samples/suit/recovery/src/main.c

samples/suit/smp_transfer/prj.conf

samples/suit/smp_transfer/rad_envelope.yaml.jinja2.digest

samples/suit/recovery/Kconfig

samples/suit/smp_transfer/CMakeLists.txt

samples/suit/recovery/src/main.c

samples/suit/smp_transfer/prj.conf

e-rk · 2024-05-22T12:05:19Z

cmake/sysbuild/suit.cmake

+    list(APPEND CORE_ARGS
+    --core ${target},${SUIT_ROOT_DIRECTORY}${target}.bin,${BINARY_DIR}/zephyr/edt.pickle,${BINARY_DIR}/zephyr/.config
+    )


I have one concern here. in flash_companion I didn't set any value for CONFIG_SUIT_ENVELOPE_TARGET, which means that it has application by default. In that case both smp_transfer and flash_companion would have the same target name, which is a recipe for problems.

I am wondering how CONFIG_SUIT_ENVELOPE_TARGET should be approached. On one hand, I think that using sysbuild image name by default is convenient and would be easier to understand for the user.

On the second hand, we want the default manifest templates to be generic regardless of the application name.

Perhaps in the flash_companion the CONFIG_SUIT_ENVELOPE_TARGET should be set to "" (empty) and in this loop there should be a check if there are multiple images with the same CONFIG_SUIT_ENVELOPE_TARGET and a warning should be displayed in such case.

Perhaps config SUIT_ENVELOPE_TARGET should depend on SUIT_ENVELOPE? In this case I think the CONFIG_SUIT_ENVELOPE_TARGET symbol would be undefined for the flash_companion image, which we could detect. I don't think we should directly set CONFIG_SUIT_ENVELOPE_TARGET in this case.

I think it would be a good approach.

samples/suit/recovery/Kconfig.sysbuild

samples/suit/smp_transfer/root_with_binary_nordic_top.yaml.jinja2

sysbuild/Kconfig.suit

cmake/sysbuild/suit.cmake

adsz-nordic · 2024-05-23T12:55:29Z

samples/suit/recovery/sysbuild.conf

@@ -0,0 +1,13 @@
+# #
+# # Copyright (c) 2024 Nordic Semiconductor ASA


Nit: Double comments?

samples/suit/smp_transfer/sysbuild/hci_ipc.overlay

e-rk · 2024-05-29T08:01:42Z

cmake/sysbuild/suit.cmake

@@ -176,12 +176,20 @@ function(suit_create_package)
  foreach(image ${IMAGES})
    sysbuild_get(BINARY_DIR IMAGE ${image} VAR APPLICATION_BINARY_DIR CACHE)
    sysbuild_get(BINARY_FILE IMAGE ${image} VAR CONFIG_KERNEL_BIN_NAME KCONFIG)
+    sysbuild_get(target IMAGE ${image} VAR CONFIG_SUIT_ENVELOPE_TARGET KCONFIG)


It is necessary to add unset(target), otherwise the parameters to suit-generator are wrong.

NordicBuilder · 2024-05-29T10:11:29Z

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.

The recovery application can be built as a standalone application and flashed with nrfutil. Signed-off-by: Artur Hadasz <[email protected]>

This commit updates the suit-generator revision to bring in the newest changes needed for recovery application. Signed-off-by: Artur Hadasz <[email protected]>

richabp

Some initial comments:

If it is an Application, it needs to be moved to Application folder.
It need to follow the Application/Sample templates that can be found https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/dev_model_and_contributions/documentation/templates.html.
Since it is a new doc, it needs to be added in the Changelog.

e-rk · 2024-05-29T12:14:22Z

If it is an Application, it needs to be moved to Application folder.

The situation is currently not clear. It would better fit the core_fw category and the current recommendation is to put such applications in the sample directory. See NCSDK-10551

ahasztag · 2024-05-29T12:15:21Z

Some initial comments:

If it is an Application, it needs to be moved to Application folder.

It need to follow the Application/Sample templates that can be found https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/dev_model_and_contributions/documentation/templates.html.

Since it is a new doc, it needs to be added in the Changelog.

The documentation will be extended and Changelog will be added in a separate PR.

When it comes to the "application" part - this will be finally placed in a completely different category - named something like "core firmware" together with some other applications. It is just temporarily kept in a sample - this was consulted with @wbober.

richabp · 2024-05-29T12:31:48Z

Some initial comments:

If it is an Application, it needs to be moved to Application folder.

It need to follow the Application/Sample templates that can be found https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/dev_model_and_contributions/documentation/templates.html.

Since it is a new doc, it needs to be added in the Changelog.

The documentation will be extended and Changelog will be added in a separate PR.

When it comes to the "application" part - this will be finally placed in a completely different category - named something like "core firmware" together with some other applications. It is just temporarily kept in a sample - this was consulted with @wbober.

Ok. Approving, assuming all the doc changes will be taken care of in another PR.

richabp

Approving. All the required doc changes will be done in another PR.

greg-fer · 2024-06-04T11:46:22Z

samples/suit/recovery/README.rst

@@ -0,0 +1,50 @@
+:orphan:


This is orphaned, but still appearing in the docs because of the inclusion at the higher level:
https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/samples/suit.html

Yes, this is/will be addressed in a different PR that is currently open, but thanks for catching!

ahasztag requested review from tomchy, SylwesterKonczyk, e-rk, robertstypa and adsz-nordic May 21, 2024 14:54

ahasztag requested review from tejlmand and nordicjm as code owners May 21, 2024 14:54

github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels May 21, 2024

ahasztag force-pushed the recovery_app branch from c84f5a2 to de772a8 Compare May 21, 2024 15:47

ahasztag requested a review from carlescufi as a code owner May 21, 2024 16:23

github-actions bot added the manifest label May 21, 2024

NordicBuilder added manifest-suit-generator DNM labels May 21, 2024

e-rk requested changes May 22, 2024

View reviewed changes

ahasztag force-pushed the recovery_app branch 5 times, most recently from b3eaa74 to bbb8e68 Compare May 22, 2024 09:57

e-rk requested changes May 22, 2024

View reviewed changes

ahasztag force-pushed the recovery_app branch from bbb8e68 to eff8cae Compare May 22, 2024 14:20

tomchy reviewed May 23, 2024

View reviewed changes

samples/suit/recovery/Kconfig.sysbuild Outdated Show resolved Hide resolved

samples/suit/smp_transfer/root_with_binary_nordic_top.yaml.jinja2 Outdated Show resolved Hide resolved

sysbuild/Kconfig.suit Outdated Show resolved Hide resolved

cmake/sysbuild/suit.cmake Show resolved Hide resolved

ahasztag force-pushed the recovery_app branch from eff8cae to fc6e8ce Compare May 23, 2024 13:59

adsz-nordic approved these changes May 24, 2024

View reviewed changes

ahasztag force-pushed the recovery_app branch from fc6e8ce to f42ac6c Compare May 24, 2024 10:33

tomchy reviewed May 24, 2024

View reviewed changes

samples/suit/smp_transfer/sysbuild/hci_ipc.overlay Outdated Show resolved Hide resolved

ahasztag force-pushed the recovery_app branch from f42ac6c to b8deb53 Compare May 27, 2024 13:20

tomchy approved these changes May 27, 2024

View reviewed changes

ahasztag force-pushed the recovery_app branch 2 times, most recently from 6a11c9c to f02ad8e Compare May 28, 2024 15:11

e-rk reviewed May 29, 2024

View reviewed changes

ahasztag force-pushed the recovery_app branch 2 times, most recently from 85fe451 to baf532e Compare May 29, 2024 08:39

ahasztag requested a review from richabp May 29, 2024 09:32

ahasztag force-pushed the recovery_app branch from baf532e to 90a3155 Compare May 29, 2024 09:34

richabp requested a review from kittydepa May 29, 2024 09:46

ahasztag force-pushed the recovery_app branch from 90a3155 to e060fe2 Compare May 29, 2024 10:26

e-rk approved these changes May 29, 2024

View reviewed changes

ahasztag force-pushed the recovery_app branch from e060fe2 to 8edf60b Compare May 29, 2024 10:46

NordicBuilder removed the DNM label May 29, 2024

ahasztag force-pushed the recovery_app branch from 8edf60b to 69eba75 Compare May 29, 2024 11:11

ahasztag added 2 commits May 29, 2024 13:53

samples: suit: Added first version of the recovery application

4db5fad

The recovery application can be built as a standalone application and flashed with nrfutil. Signed-off-by: Artur Hadasz <[email protected]>

manifest: update suit-generator revision

24c2969

This commit updates the suit-generator revision to bring in the newest changes needed for recovery application. Signed-off-by: Artur Hadasz <[email protected]>

ahasztag force-pushed the recovery_app branch from 69eba75 to 24c2969 Compare May 29, 2024 11:59

richabp reviewed May 29, 2024

View reviewed changes

ahasztag removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 29, 2024

richabp approved these changes May 29, 2024

View reviewed changes

ahasztag mentioned this pull request May 29, 2024

Add the default DFU partition definition #15490

Merged

rlubos merged commit e895510 into nrfconnect:main May 29, 2024
17 checks passed

greg-fer reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

suit: recovery application build separately #15422

suit: recovery application build separately #15422

ahasztag commented May 21, 2024 •

edited by kszromek-nordic

Loading

NordicBuilder commented May 21, 2024 •

edited

Loading

NordicBuilder commented May 21, 2024 •

edited

Loading

e-rk May 22, 2024 •

edited

Loading

ahasztag May 22, 2024

e-rk May 28, 2024

adsz-nordic May 23, 2024

ahasztag May 24, 2024

e-rk May 29, 2024 •

edited

Loading

NordicBuilder commented May 29, 2024

richabp left a comment

e-rk commented May 29, 2024 •

edited by jira bot

Loading

ahasztag commented May 29, 2024

richabp commented May 29, 2024

richabp left a comment

greg-fer Jun 4, 2024

kittydepa Jun 4, 2024

		@@ -0,0 +1,13 @@
		# #
		# # Copyright (c) 2024 Nordic Semiconductor ASA

suit: recovery application build separately #15422

suit: recovery application build separately #15422

Conversation

ahasztag commented May 21, 2024 • edited by kszromek-nordic Loading

NordicBuilder commented May 21, 2024 • edited Loading

Test specification

NordicBuilder commented May 21, 2024 • edited Loading

e-rk May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-rk May 29, 2024 • edited Loading

Choose a reason for hiding this comment

NordicBuilder commented May 29, 2024

richabp left a comment

Choose a reason for hiding this comment

e-rk commented May 29, 2024 • edited by jira bot Loading

ahasztag commented May 29, 2024

richabp commented May 29, 2024

richabp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahasztag commented May 21, 2024 •

edited by kszromek-nordic

Loading

NordicBuilder commented May 21, 2024 •

edited

Loading

NordicBuilder commented May 21, 2024 •

edited

Loading

e-rk May 22, 2024 •

edited

Loading

e-rk May 29, 2024 •

edited

Loading

e-rk commented May 29, 2024 •

edited by jira bot

Loading