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

samples: matter: Refactor nRF54H20 partition in samples #19735

Merged

Conversation

ArekBalysNordic
Copy link
Contributor

  • Refactored nRF54H20 partitions to support storing Nordic-related
    SoC binaries in external flash.
  • Enabled storing Nordic SoC binaries in the external flash if
    the SUIT_ENVELOPE_NORDIC_TOP_DIRECTORY kconfig value is provided.
  • Described SUIT DFU process in Matter samples which can be performed
    on nRF54H20 modules. Currently, only the Matter OTA protocol is fully supported.

@ArekBalysNordic ArekBalysNordic requested review from a team as code owners January 3, 2025 09:52
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Jan 3, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 3, 2025

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 8

Inputs:

Sources:

sdk-nrf: PR head: 73b79da21bf598c5249857cf64d252cdee1c506f

more details

sdk-nrf:

PR head: 73b79da21bf598c5249857cf64d252cdee1c506f
merge base: a14701baea486b7d65771ad3b9826719c60633dd
target head (main): 371ff9d35234992b9b43f459fa76d9e78b61bd55
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 (16)
applications
│  ├── matter_bridge
│  │  ├── Kconfig.sysbuild
│  │  ├── boards
│  │  │  ├── nrf54h20dk_nrf54h20_cpuapp.conf
│  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay
│  │  ├── doc
│  │  │  │ matter_bridge_description.rst
config
│  ├── suit
│  │  ├── templates
│  │  │  ├── nrf54h20
│  │  │  │  ├── matter
│  │  │  │  │  ├── v1
│  │  │  │  │  │  ├── app_envelope.yaml.jinja2
│  │  │  │  │  │  ├── rad_envelope.yaml.jinja2
│  │  │  │  │  │  │ root_with_binary_nordic_top.yaml.jinja2
doc
│  ├── nrf
│  │  ├── app_dev
│  │  │  ├── device_guides
│  │  │  │  ├── nrf54h
│  │  │  │  │  │ ug_nrf54h20_suit_soc_binaries.rst
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
samples
│  ├── matter
│  │  ├── common
│  │  │  ├── dts
│  │  │  │  ├── nrf54h20
│  │  │  │  │  ├── nrf54h20_cpuapp_memory_map.dtsi
│  │  │  │  │  │ nrf54h20_cpurad_memory_map.dtsi
│  │  ├── lock
│  │  │  ├── Kconfig.sysbuild
│  │  │  │ README.rst
│  │  ├── template
│  │  │  ├── Kconfig.sysbuild
│  │  │  │ README.rst
sysbuild
│  │ Kconfig.matter

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_912848a074

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

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 7
  • ✅ Integration tests
    • ✅ test-sdk-audio - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ desktop52_verification - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-boot - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-apps - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_mesh - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-ble_samples - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nfc - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_libmodem-nrf - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_samples - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ doc-internal - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_thingy91 - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf_crypto - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-rpc - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-rs - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-tfm - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-thread - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-zigbee - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_mosh - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_positioning - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-sidewalk - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-wifi - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-low-level - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_nrf_provisioning - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-pmic-samples - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-mcuboot - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-dfu - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-ps - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-secdom-samples-public - Skipped: Job was skipped as it succeeded in a previous run

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

samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
samples/matter/template/README.rst Outdated Show resolved Hide resolved
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 3, 2025

Memory footprint analysis revealed the following potential issues

sample.matter.template.debug[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 911930[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)
sample.matter.template.release[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 820762[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)

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

@ArekBalysNordic ArekBalysNordic force-pushed the nrf54h20_new_partitions branch from c68920b to 1831f2a Compare January 7, 2025 07:41
@ArekBalysNordic ArekBalysNordic force-pushed the nrf54h20_new_partitions branch 2 times, most recently from a147c26 to 426104d Compare January 8, 2025 08:02
@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 Publish GitHub Action.

@ArekBalysNordic
Copy link
Contributor Author

@nrfconnect/ncs-charon, @nrfconnect/ncs-charon-doc and @nrfconnect/ncs-co-build-system could you please review related changes?

Comment on lines 40 to 55
if SOC_SERIES_NRF54HX

config SUIT_ENVELOPE
default y

config SUIT_BUILD_FLASH_COMPANION
default y

config SUIT_MULTI_IMAGE_PACKAGE_BUILD
default y if MATTER_OTA

config SUIT_BASE_MANIFEST_VARIANT
default "matter/v1"

endif
config SUIT_ENVELOPE_NORDIC_TOP_EXTRACT_PAYLOADS_TO_CACHE
default y if SUIT_ENVELOPE_NORDIC_TOP_DIRECTORY != ""
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be abusing Kconfig files as thought they are defconfig files. If these are needed then the symbols should depend on them, or select them if the known issues are adhered to properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I would like to set the SUIT_ENVELOPE_NORDIC_TOP_EXTRACT_PAYLOADS_TO_CACHE if the user provides a path to the top directory using SUIT_ENVELOPE_NORDIC_TOP_DIRECTORY. Currently in Kconfig.suit SUIT_ENVELOPE_NORDIC_TOP_EXTRACT_PAYLOADS_TO_CACHE depends on SUIT_ENVELOPE_NORDIC_TOP_IN_ROOT and SUIT_ENVELOPE_NORDIC_TOP_IN_ROOT is default y if SUIT_ENVELOPE_NORDIC_TOP_DIRECTORY != "". I don't understand the abuse here, which kconfig of all mentioned should select or have dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nordicjm what do you recommend in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are setting defaults, these defaults will only apply at configuration, when someone does sysbuild_menuconfig and changes them they will be completely ignored. If you want to select options, select them when your symbols are enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I've modified them. Indeed if we use MATTER sysbuild config at least one DFU method must be present (according to spec). So we must select SUIT mechanisms for the nRF54H20 device anyway and forbid users to change them. There is only one exception - SUIT_BASE_MANIFEST_VARIANT which may be different in a user application. Could you please take a look at whether now they're set properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nordicjm Is it OK now after the recent change?

- Refactored nRF54H20 partitions to support storing Nordic-related
SoC binaries in external flash.
- Enabled storing Nordic SoC binaries in the external flash if
the SUIT_ENVELOPE_NORDIC_TOP_DIRECTORY kconfig value is provided.

Signed-off-by: Arkadiusz Balys <arkadiusz.balys@nordicsemi.no>
Described SUIT DFU process in Matter samples which can be performed
on nRF54H20 modules.
Currently only Matter OTA protocol is fully supported.

Signed-off-by: Arkadiusz Balys <arkadiusz.balys@nordicsemi.no>
@ArekBalysNordic ArekBalysNordic force-pushed the nrf54h20_new_partitions branch from 426104d to 73b79da Compare January 15, 2025 12:38
Comment on lines 48 to 49
config SUIT_BASE_MANIFEST_VARIANT
default "matter/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

this has the same issue I said before, if you configure an application without matter, then enable matter, this is not transferred and the original default will remain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder why users even may want to compile matter samples without matter. Honestly, it doesn't make sense to me. Moreover, I cannot set string values via imply or select, right? So it should be set as

config SUIT_BASE_MANIFEST_VARIANT
     default "matter/v1" if MATTER

In the original definition within the SUIT module.

Your comment is not related to the scope of this PR, and moreover, this default here wasn't written by us, but by the ncs-charon team some time ago and this change must be discussed with them as well. So could you please review the actual changes?

@ArekBalysNordic ArekBalysNordic merged commit 4389cc3 into nrfconnect:main Jan 17, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants