Added warning if missing pm_static.yml when using single bootloader #10686

hellesvik-nordic · 2023-03-24T10:43:10Z

if you have a deployed product that consists of multiple images, where only a subset of the included images can be upgraded through a firmware update mechanism, the upgradable images must be statically configured.

This PR gives a build warning, so users who miss this documentation will also know about this limitation.

Fixes NCSIDB-942

CLAassistant · 2023-03-24T10:43:18Z

All committers have signed the CLA.

NordicBuilder · 2023-03-24T10:43:30Z

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

cmake/partition_manager.cmake

NordicBuilder · 2023-04-03T10:52:36Z

Test specification

CI/Jenkins/NRF

Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
desktop52_verification	X
test-ci-nrfconnect-boot-fw-update	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-chip	X
test-fw-nrfconnect-fem	X
test-fw-nrfconnect-nfc	X
test-fw-nrfconnect-nrf-iot_cloud	X
test-fw-nrfconnect-nrf-iot_lwm2m	X
test-fw-nrfconnect-nrf-iot_mosh	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-sdk-audio	X
test-sdk-find-my	X
test-sdk-homekit	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

github-actions · 2023-06-03T00:29:13Z

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.

hellesvik-nordic · 2023-06-05T06:16:07Z

It is just review missing on this PR right?
Let me know if there is anything more I should do

hakonfam · 2023-06-06T04:53:48Z

cmake/partition_manager.cmake

+        --- WARNING: Using a single bootloader without pm_static.yml.   ---
+        --- Static partitioning requires using a single bootloader when ---
+        --- releasing firmware. Only use dynamic during development.    ---


I don't get it. Isn't it just as problematic having two bootloaders without a static configuration? You need to statically define the size of the first non-upgradeable bootloader in both cases, to avoid run-time issues when programming the updateable bootloader and/or application.

Ah right.
Yes I agree, the non-upgradable parts should be static, also for two bootloaders.
Should I expand this warning to all build with bootloaders then?
(And rephrase it)

Yes I would suggest that we do that. The docs covers the other configurations (for example if you have a statically placed file system), and its not trivial to check that in build time through kconfig.

I changed the check and text now.
What do you think?
(also misclicked resolve)

hakonfam

Look good. I would, however, consider changing the comment we have now. It provides details relevant for the user, but its hidden in a source file which I guess people won't feel a need to inspect due to the verbosity of the warning message. Its just a nit so feel free to keep it, merge its content to the warning message or simply remove it (especially if you feel that the information it gives is present in the docs pointed to by the warning message).

hellesvik-nordic · 2023-06-09T08:21:16Z

I agree that the comment is redundant now that I refer to the docs in the warning.
Removed the comment.
Also rebased PR to main

nordicjm · 2023-07-20T13:46:09Z

cmake/partition_manager.cmake

+# From Documentation: By default, the Partition Manager dynamically
+# places the partitions in memory. However, if you have a deployed
+# product that consists of multiple images, where only a subset of
+# the included images can be upgraded through a firmware update
+# mechanism, the upgradable images must be statically configured.
+if (NOT static_configuration AND
+  (CONFIG_BOOTLOADER_MCUBOOT OR CONFIG_SECURE_BOOT))
+      message(WARNING "
+        ---------------------------------------------------------------------
+        --- WARNING: Using a bootloader without pm_static.yml.            ---
+        --- Static partitioning is required for non-upgradable partitions.---
+        --- See Partition Manager docs on Static Configuration for more   ---
+        --- info.                                                         ---
+        ---------------------------------------------------------------------
+        \n"
+      )


Comment and message are opposed, and message is actually wrong

Good point
However, changing it to just
Static partitioning is required for non-upgradable partitions. is neither correct, as it can be done with dual bootloaders.
How about making it more generic?
Static partitioning is required for certain bootloader configurations. Check Partition Manager docs on Static Configuration to see if this applies to you.

Or possibly Static partitioning is required when using a non-upgradable bootloader. ?

I like the last sentence

Static partitioning is required when using a non-upgradable bootloader.

Is confusing, it would be better to just link to or quote the documentation as it is on https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/scripts/partition_manager/partition_manager.html#static-configuration

Cant find a way to include the link without exceeding line-length.
So I try to copy the text from docs in.
Then the warning will look like this in a build:

What do you think about this?

That looks great!

hellesvik-nordic · 2023-07-21T07:44:33Z

Should we add a link to docs instead of just reffering them by name?

Add a warning message if static configuration is missing when building with a single bootloader. Ref.: NCSIDB-942 Signed-off-by: Sigurd Hellesvik <[email protected]>

hellesvik-nordic requested a review from tejlmand as a code owner March 24, 2023 10:43

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Mar 24, 2023

NordicBuilder added the external External contribution label Mar 24, 2023

tejlmand approved these changes Mar 24, 2023

View reviewed changes

tejlmand requested review from oyvindronningstad and hakonfam March 24, 2023 10:56

tejlmand approved these changes Mar 24, 2023

View reviewed changes

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from d9e345d to 3be14b7 Compare March 24, 2023 11:07

tejlmand removed the external External contribution label Mar 24, 2023

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from 3be14b7 to c573dcb Compare March 24, 2023 11:42

sigvartmh reviewed Mar 24, 2023

View reviewed changes

cmake/partition_manager.cmake Outdated Show resolved Hide resolved

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch 2 times, most recently from ec257a6 to 16177a0 Compare March 24, 2023 14:42

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from 16177a0 to ea57327 Compare April 3, 2023 10:43

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from ea57327 to 9e56268 Compare April 3, 2023 12:05

github-actions bot added the Stale label Jun 3, 2023

github-actions bot removed the Stale label Jun 6, 2023

hakonfam reviewed Jun 6, 2023

View reviewed changes

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from 9e56268 to c09b577 Compare June 8, 2023 14:01

hakonfam approved these changes Jun 8, 2023

View reviewed changes

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch 3 times, most recently from c6d742d to 9138b8e Compare June 9, 2023 08:20

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from 9138b8e to 6a6ab90 Compare July 4, 2023 06:39

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch 4 times, most recently from 97afa58 to 2a7f335 Compare July 20, 2023 11:51

nordicjm requested changes Jul 20, 2023

View reviewed changes

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from 2a7f335 to a5eab2a Compare July 25, 2023 12:01

sigvartmh approved these changes Jul 25, 2023

View reviewed changes

nordicjm approved these changes Jul 25, 2023

View reviewed changes

cmake: partition_manager: Added warning if using single bootloader

ba3f950

Add a warning message if static configuration is missing when building with a single bootloader. Ref.: NCSIDB-942 Signed-off-by: Sigurd Hellesvik <[email protected]>

hellesvik-nordic force-pushed the partition_manager_single_bootloader_warning branch from a5eab2a to ba3f950 Compare July 25, 2023 14:41

nordicjm requested a review from annakielar July 26, 2023 09:26

greg-fer approved these changes Jul 26, 2023

View reviewed changes

nordicjm merged commit 4059b61 into nrfconnect:main Jul 26, 2023
38 checks passed

hellesvik-nordic deleted the partition_manager_single_bootloader_warning branch October 7, 2024 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added warning if missing pm_static.yml when using single bootloader #10686

Added warning if missing pm_static.yml when using single bootloader #10686

hellesvik-nordic commented Mar 24, 2023

CLAassistant commented Mar 24, 2023 •

edited

Loading

NordicBuilder commented Mar 24, 2023

NordicBuilder commented Apr 3, 2023 •

edited

Loading

github-actions bot commented Jun 3, 2023

hellesvik-nordic commented Jun 5, 2023

hakonfam Jun 6, 2023

hellesvik-nordic Jun 6, 2023

hakonfam Jun 6, 2023

hellesvik-nordic Jun 8, 2023

hakonfam left a comment

hellesvik-nordic commented Jun 9, 2023

nordicjm Jul 20, 2023

hellesvik-nordic Jul 20, 2023

hellesvik-nordic Jul 21, 2023

sigvartmh Jul 25, 2023

nordicjm Jul 25, 2023

hellesvik-nordic Jul 25, 2023

nordicjm Jul 25, 2023

hellesvik-nordic commented Jul 21, 2023

Added warning if missing pm_static.yml when using single bootloader #10686

Added warning if missing pm_static.yml when using single bootloader #10686

Conversation

hellesvik-nordic commented Mar 24, 2023

CLAassistant commented Mar 24, 2023 • edited Loading

NordicBuilder commented Mar 24, 2023

NordicBuilder commented Apr 3, 2023 • edited Loading

Test specification

github-actions bot commented Jun 3, 2023

hellesvik-nordic commented Jun 5, 2023

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

hakonfam left a comment

Choose a reason for hiding this comment

hellesvik-nordic commented Jun 9, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellesvik-nordic commented Jul 21, 2023

CLAassistant commented Mar 24, 2023 •

edited

Loading

NordicBuilder commented Apr 3, 2023 •

edited

Loading