Add a common function to read partition address from CMake #24792

tomchy · 2025-09-30T13:15:40Z

No description provided.

NordicBuilder · 2025-09-30T13:16:24Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
zephyr	nrfconnect/sdk-zephyr@`e2ea7ff`	nrfconnect/sdk-zephyr@`d47f109` (`main`)	nrfconnect/[email protected]

✅ All manifest checks OK

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

NordicBuilder · 2025-09-30T13:20:12Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 10

Inputs:

Sources:

sdk-nrf: PR head: 07dc5a391549067609605a97e537341f14fd3c23
zephyr: PR head: d47f109210e74e2e9df3ab5485a32b8a34254caf

more details

sdk-nrf:

PR head: 07dc5a391549067609605a97e537341f14fd3c23
merge base: 90a5e519245123035108fe9bbfd7f9ec83217d3b
target head (main): 90a5e519245123035108fe9bbfd7f9ec83217d3b
Diff

zephyr:

PR head: d47f109210e74e2e9df3ab5485a32b8a34254caf
merge base: e2ea7ff53ff6701c74be3da713c33d473c311aad
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (6)

cmake
│  ├── modules
│  │  │ kconfig.cmake
│  ├── sysbuild
│  │  ├── bootloader_dts_utils.cmake
│  │  ├── image_signing.cmake
│  │  │ sign_nrf54h20.cmake
west.yml
zephyr
│  ├── scripts
│  │  ├── dts
│  │  │  │ gen_dts_cmake.py

Outputs:

Toolchain

Version: a7529a11f4
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:a7529a11f4_bba2ea5f2e

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
❌ Build twister
❌ Integration tests
- ❌ test-sdk-audio
- ❌ desktop52_verification
- ❌ test-fw-nrfconnect-apps
- ❌ test-fw-nrfconnect-ble_mesh
- ❌ test-fw-nrfconnect-ble_samples
- ❌ test-fw-nrfconnect-chip
- ❌ test-fw-nrfconnect-nfc
- ❌ test-fw-nrfconnect-nrf-iot_cloud
- ❌ test-fw-nrfconnect-nrf-iot_libmodem-nrf
- ❌ test-fw-nrfconnect-nrf-iot_serial_lte_modem
- ❌ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ❌ test-fw-nrfconnect-nrf-iot_samples
- ❌ test-fw-nrfconnect-nrf-iot_lwm2m
- ❌ test-fw-nrfconnect-nrf-iot_thingy91
- ❌ test-fw-nrfconnect-nrf_crypto
- ❌ test-fw-nrfconnect-proprietary_esb
- ❌ test-fw-nrfconnect-rpc
- ❌ test-fw-nrfconnect-rs
- ❌ test-fw-nrfconnect-fem
- ❌ test-fw-nrfconnect-tfm
- ❌ test-fw-nrfconnect-thread-main
- ❌ test-sdk-find-my
- ❌ test-sdk-wifi
- ❌ test-low-level
- ❌ test-sdk-pmic-samples
- ❌ test-sdk-mcuboot
- ❌ test-sdk-dfu
- ❌ test-fw-nrfconnect-ps-main
- ❌ test-secdom-samples-public

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

github-actions · 2025-09-30T13:58:20Z

You can find the documentation preview for this PR here.

tomchy · 2025-09-30T14:45:29Z

Scope reduced: There is no point in adding multiple overrides if they are about to be removed once the support for subpartitions will be introduced..

NordicBuilder · 2025-09-30T16:24:56Z

Memory footprint analysis revealed the following potential issues

applications.hpf.gpio.icmsg[nrf54l15dk/nrf54l15/cpuflpr]: High RAM usage: 9102[B] - link (cc: @nrfconnect/ncs-ll-ursus)
applications.hpf.gpio.icmsg[nrf54l15dk/nrf54l15/cpuflpr]: High ROM usage: 5858[B] - link (cc: @nrfconnect/ncs-ll-ursus)
applications.hpf.gpio.icbmsg[nrf54l15dk/nrf54l15/cpuflpr]: High RAM usage: 12744[B] - link (cc: @nrfconnect/ncs-ll-ursus)
applications.hpf.gpio.icbmsg[nrf54l15dk/nrf54l15/cpuflpr]: High ROM usage: 9492[B] - link (cc: @nrfconnect/ncs-ll-ursus)

Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-24792/7)

tejlmand

First round revierw.
Please note that line length limit is 100 chars, so some lines is not required to wrap.
I've only pointed out a few.

cmake/sysbuild/sign_nrf54h20.cmake

cmake/sysbuild/bootloader_dts_utils.cmake

tejlmand · 2025-10-01T13:21:19Z

cmake/sysbuild/bootloader_dts_utils.cmake

+    if(NOT DEFINED DT_PARTITION_PATH)
+      if(DT_PARTITION_REQUIRED)
+        message(FATAL_ERROR
+          "dt_partition_addr: Unable to find node with label: ${DT_PARTITION_LABEL}")


can we provide some more info to the developer on how to fix this issue ?

Not really. This is a helper/utility function and it is hard to guess, in which context it is called. Basically, if it fails, it means that some sort of script expects a node in your DTS, but it is not there. I will add a target name to the message to help at least a bit.

cmake/sysbuild/bootloader_dts_utils.cmake

tejlmand · 2025-10-02T09:11:53Z

cmake/sysbuild/bootloader_dts_utils.cmake

+    _dt_get_parent(dt_partition_parent)
+    if("${dt_partition_parent}" STREQUAL "")


A common pattern in CMake is to use, <var>-NOTFOUND and then simply do the test like this:

if(NOT dt_partition_parent)

but in which cases can the dt_partition_parent be an empty string ?
Because is seems that either there is a parent, or a CMake error is raised if there is no parent.
Ref:

sdk-nrf/cmake/sysbuild/bootloader_dts_utils.cmake

Lines 11 to 13 in a913cc8

if(pos EQUAL -1)

message(FATAL_ERROR "Unable to get parent of node: ${${node}}")

endif()

I've redesigned this section, based on the recent fix: nrfconnect/sdk-zephyr#3353

tejlmand

Much better.

Mostly minor nits, and some observations.

Please checkup regarding line length and line wrapping.
Take a look at the CMake Style Guidelines

cmake/sysbuild/bootloader_dts_utils.cmake

tejlmand · 2025-10-02T13:18:53Z

cmake/sysbuild/bootloader_dts_utils.cmake

+  cmake_parse_arguments(DT_PARTITION "ABSOLUTE;REQUIRED" "LABEL;PATH;TARGET" ""
+    ${ARGN})


Proposed change is still <100 chars.

Suggested change

cmake_parse_arguments(DT_PARTITION "ABSOLUTE;REQUIRED" "LABEL;PATH;TARGET" ""

${ARGN})

cmake_parse_arguments(arg_DT_PARTITION "ABSOLUTE;REQUIRED" "LABEL;PATH;TARGET" "" ${ARGN})

Also, please prefix with lower case arg_ or simply arg to avoid name clashing as recommended by CMake.

Choose a carefully to avoid clashing with existing variable names. When used inside a function, it is usually suitable to use the prefix arg. There is a very strong convention that all keywords are fully uppercase, so this prefix results in variables of the form arg_SOME_KEYWORD. This makes the code more readable, and it minimizes the chance of clashing with cache variables, which also have a strong convention of being all uppercase.

https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html

See also: https://gitlab.kitware.com/cmake/cmake/-/issues/25773

Prepended with the arg_ prefix.

cmake/sysbuild/bootloader_dts_utils.cmake

ahasztag

Minor suggestion regarding a variable name

cmake/sysbuild/bootloader_dts_utils.cmake

Add a common bootloader_dts_util to read both the relative and absolute address of a fixed sub/partition from DTS. Signed-off-by: Tomasz Chyrowicz <[email protected]>

tomchy requested review from a team as code owners September 30, 2025 13:15

NordicBuilder added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 30, 2025

tomchy changed the title ~~Add common function to read partition address from CMake~~ Add a common function to read partition address from CMake Sep 30, 2025

NordicBuilder added manifest-mcuboot DNM labels Sep 30, 2025

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch from 859c70b to 0474ea0 Compare September 30, 2025 14:14

tomchy requested review from a team as code owners September 30, 2025 14:14

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch from 0474ea0 to a913cc8 Compare September 30, 2025 14:44

NordicBuilder removed the manifest label Sep 30, 2025

NordicBuilder removed manifest-mcuboot DNM labels Sep 30, 2025

tejlmand requested changes Oct 2, 2025

View reviewed changes

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch from a913cc8 to 98e4353 Compare October 2, 2025 12:50

NordicBuilder added the manifest label Oct 2, 2025

tomchy requested a review from tejlmand October 2, 2025 12:50

NordicBuilder added manifest-zephyr DNM labels Oct 2, 2025

tejlmand requested changes Oct 2, 2025

View reviewed changes

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch 2 times, most recently from c804096 to 5419a9b Compare October 2, 2025 13:41

tomchy requested a review from tejlmand October 2, 2025 13:41

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch from 5419a9b to 4fd1e90 Compare October 3, 2025 08:53

ahasztag reviewed Oct 3, 2025

View reviewed changes

cmake/sysbuild/bootloader_dts_utils.cmake Outdated Show resolved Hide resolved

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch from 4fd1e90 to f7d7e2a Compare October 3, 2025 14:25

tomchy requested a review from ahasztag October 3, 2025 14:25

ahasztag approved these changes Oct 3, 2025

View reviewed changes

cmake: Add function to read partition address

07dc5a3

Add a common bootloader_dts_util to read both the relative and absolute address of a fixed sub/partition from DTS. Signed-off-by: Tomasz Chyrowicz <[email protected]>

tomchy force-pushed the feature/cmake/NCSDK-NONE_Unify_partition_addresses branch from f7d7e2a to 07dc5a3 Compare October 6, 2025 09:19

NordicBuilder removed the DNM label Oct 6, 2025

		_dt_get_parent(dt_partition_parent)
		if("${dt_partition_parent}" STREQUAL "")

	if(pos EQUAL -1)
	message(FATAL_ERROR "Unable to get parent of node: ${${node}}")
	endif()

		cmake_parse_arguments(DT_PARTITION "ABSOLUTE;REQUIRED" "LABEL;PATH;TARGET" ""
		${ARGN})

	cmake_parse_arguments(DT_PARTITION "ABSOLUTE;REQUIRED" "LABEL;PATH;TARGET" ""
	${ARGN})
	cmake_parse_arguments(arg_DT_PARTITION "ABSOLUTE;REQUIRED" "LABEL;PATH;TARGET" "" ${ARGN})

Add a common function to read partition address from CMake #24792

Are you sure you want to change the base?

Add a common function to read partition address from CMake #24792

Conversation

tomchy commented Sep 30, 2025

Uh oh!

NordicBuilder commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

Uh oh!

github-actions bot commented Sep 30, 2025

Uh oh!

tomchy commented Sep 30, 2025

Uh oh!

NordicBuilder commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Memory footprint analysis revealed the following potential issues

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tejlmand Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

tomchy Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tejlmand Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

tomchy Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tejlmand Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

tomchy Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahasztag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NordicBuilder commented Sep 30, 2025 •

edited

Loading

NordicBuilder commented Sep 30, 2025 •

edited

Loading

NordicBuilder commented Sep 30, 2025 •

edited

Loading