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

snippets: Added snippet for Matter power consumption measurements #20072

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kkasperczyk-no
Copy link
Contributor

Matter requires some extra configuration for the power consumption measurements that are automatized. This change introduces snippet that simplifies it and adds dedicated variant in the sample.ymls.

@kkasperczyk-no kkasperczyk-no requested review from a team as code owners January 24, 2025 13:20
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 24, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 24, 2025

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: b1fd34539090a3b3aa65fe6bf0f20b1ff90a74e3

more details

sdk-nrf:

PR head: b1fd34539090a3b3aa65fe6bf0f20b1ff90a74e3
merge base: a14efda70dbef70e0a2a6dc27f66a5f63f7fc2a6
target head (main): 8a0d4647d6c16dcd88e0b5b489c2033bd1e97c99
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 (9)
CODEOWNERS
samples
│  ├── matter
│  │  ├── lock
│  │  │  │ sample.yaml
│  │  ├── smoke_co_alarm
│  │  │  │ sample.yaml
│  │  ├── window_covering
│  │  │  │ sample.yaml
scripts
│  │ quarantine_integration.yaml
snippets
│  ├── matter-power-consumption-tests
│  │  ├── boards
│  │  │  ├── nrf54l15dk_nrf54l10_cpuapp.conf
│  │  │  │ nrf54l15dk_nrf54l15_cpuapp.conf
│  │  ├── power_consumption_tests.conf
│  │  │ snippet.yml

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
    • sdk-nrf test count: 115
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-chip
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi
    • test-secdom-samples-public

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

Comment on lines +7 to +14
# Disable LEDs that lead to the current leakages
CONFIG_NCS_SAMPLE_MATTER_LEDS=n

# Disable watchdog that increases the sleep current
CONFIG_NCS_SAMPLE_MATTER_WATCHDOG=n

# Increase MPSL calibration period to prevent too often CPU wake-ups.
CONFIG_MPSL_CALIBRATION_PERIOD=60000
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this need a specific kconfig fragment for the board, why can't it go in the normal snippet .conf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, the nRF54L15 needs few additional tweaks to lower power consumption. On other platforms this is not required (this particular Kconfig option is even not used).

Copy link
Contributor

Choose a reason for hiding this comment

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

so disabling it isn't a problem then, that way there is a snippet that works for all boards including customer boards, whereas right now this works for no nrf54l* customer boards

Copy link
Contributor Author

@kkasperczyk-no kkasperczyk-no Jan 27, 2025

Choose a reason for hiding this comment

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

This is not targeted for the customers use, and we also do not mention how to use it in the documentation. This is added for the QA to avoid handling multiple configurations in their Test Automation System.

This works on all boards, but we don't want to use it with all boards, because this is not required. Only nRF54L15 DK (not SoC) has problems with LEDs current leakage, so I don't see a reason to disable it for other/customer boards as well. Similarly, we configure nrf dks in samples' boards directory and this does not apply to the customers' boards, so I think we are not doing anything different here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, as maybe I haven't put it explicitly. Disabling this modules is not something we would like to do, ideally we would like to keep it enabled. Unfortunately, the nRF54L15 DK requires this sort of "workaround", so we don't want to put workaround, where it is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link.

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

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.

LGTM

Copy link
Contributor

@ArekBalysNordic ArekBalysNordic left a comment

Choose a reason for hiding this comment

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

I guess you should add the new snippet to codeowners and assign the ncs-matter to it.

@kkasperczyk-no kkasperczyk-no requested a review from a team as a code owner January 24, 2025 14:04
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 24, 2025
@doublemis1
Copy link
Contributor

@kkasperczyk-no, forgot to mention, can you look at the integration_quarantine.yaml in the scripts dir?
Maybe we can add these configuration to the integration quarantine, instead of on sample (on all platforms) e.g. door lock

Matter requires some extra configuration for the power consumption
measurements that are automatized. This change introduces snippet
that simplifies it and adds dedicated variant in the sample.ymls.

Signed-off-by: Kamil Kasperczyk <kamil.kasperczyk@nordicsemi.no>
@kkasperczyk-no kkasperczyk-no requested a review from a team as a code owner January 27, 2025 10:23
@kkasperczyk-no
Copy link
Contributor Author

@kkasperczyk-no, forgot to mention, can you look at the integration_quarantine.yaml in the scripts dir? Maybe we can add these configuration to the integration quarantine, instead of on sample (on all platforms) e.g. door lock

@doublemis1 done, please re-check

Comment on lines +6 to +11
nrf54l15dk/nrf54l15/cpuapp:
append:
EXTRA_CONF_FILE: boards/nrf54l15dk_nrf54l15_cpuapp.conf
nrf54l15dk/nrf54l10/cpuapp:
append:
EXTRA_CONF_FILE: boards/nrf54l15dk_nrf54l10_cpuapp.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we can merge those two to all DKs dedicated for nRF54L15 and don't have the additional nrf54l15dk_nrf54l10_cpuapp.conf file to maintain:

Suggested change
nrf54l15dk/nrf54l15/cpuapp:
append:
EXTRA_CONF_FILE: boards/nrf54l15dk_nrf54l15_cpuapp.conf
nrf54l15dk/nrf54l10/cpuapp:
append:
EXTRA_CONF_FILE: boards/nrf54l15dk_nrf54l10_cpuapp.conf
boards:
/nrf54l15dk.*/:
append:
EXTRA_DTC_OVERLAY_FILE: boards/nrf54l15dk_nrf54l15_cpuapp.overlay

Copy link
Member

Choose a reason for hiding this comment

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

I think we should change the overlay file name then? to something generic for all nrf54l15dk builds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants