Skip to content

Conversation

@nordic-segl
Copy link
Contributor

Add test that confirms correct operation of PWM
when target enters and leaves low power mode.

@github-actions 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 Sep 18, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Sep 18, 2024

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: bf1942906eef14f2a69d7244a9ece4dbd1c79892
zephyr: PR head: b83478a230e79d24d69592274f2a93c9c83958a9

more details

sdk-nrf:

PR head: bf1942906eef14f2a69d7244a9ece4dbd1c79892
merge base: 9de551ad932bf668ba15c18b2757a00745eb26ad
target head (main): 9de551ad932bf668ba15c18b2757a00745eb26ad
Diff

zephyr:

PR head: b83478a230e79d24d69592274f2a93c9c83958a9
merge base: ea7e265dadf63ab2def0813378f20f2da793877e
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 (31)
CODEOWNERS
tests
│  ├── benchmarks
│  │  ├── multicore
│  │  │  ├── idle
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay
│  │  │  │  │ testcase.yaml
│  │  │  ├── idle_gpio
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay
│  │  │  ├── idle_outside_of_main
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay
│  │  │  ├── idle_spim
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay
│  │  │  │  │ testcase.yaml
│  │  │  ├── idle_twim
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay
│  │  │  │  │ testcase.yaml
│  │  │  ├── idle_uarte
│  │  │  │  ├── boards
│  │  │  │  │  ├── nrf54h20dk_nrf54h20_cpuapp.overlay
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay
│  │  │  │  │ testcase.yaml
│  │  │  ├── idle_with_pwm
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── Kconfig
│  │  │  │  ├── Kconfig.sysbuild
│  │  │  │  ├── README.rst
│  │  │  │  ├── prj.conf
│  │  │  │  ├── prj_s2ram.conf
│  │  │  │  ├── remote
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── Kconfig
│  │  │  │  │  ├── boards
│  │  │  │  │  │  │ nrf54h20dk_nrf54h20_cpurad.overlay
│  │  │  │  │  ├── prj.conf
│  │  │  │  │  │ prj_s2ram.conf
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
│  │  │  │  ├── sysbuild.cmake
│  │  │  │  ├── sysbuild
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpurad.conf
│  │  │  │  │ testcase.yaml
│  │  ├── power_consumption
│  │  │  ├── gpio
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay
west.yml
zephyr
│  ├── dts
│  │  ├── common
│  │  │  ├── nordic
│  │  │  │  │ nrf54h20.dtsi
│  ├── tests
│  │  ├── drivers
│  │  │  ├── uart
│  │  │  │  ├── uart_pm
│  │  │  │  │  ├── boards
│  │  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.conf

Outputs:

Toolchain

Version: 6c44240e03
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:6c44240e03_81ed5a52d6

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

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ❌ Build twister
    • sdk-nrf test count: 83
    • sdk-zephyr test count: 6250
  • ❌ Integration tests
    • ❌ test-low-level
    • ❌ test-sdk-dfu
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-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_cloud
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • 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-proprietary_esb
    • 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-sdk-audio
    • 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

@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from cdcbe0e to 3ec29a8 Compare September 18, 2024 13:52
@nordic-segl
Copy link
Contributor Author

Tested manually without PPK / amperemeter.
Also, Port 0, pin 7 is not confirmed (it's in control of Radio core).

'benchmarks.multicore.idle_with_pwm.nrf54h20dk_cpuapp_cpurad.no_sleep' loops as expected - LED2 lights up, then OFF for ~1 sec, then lights up again, etc.

'benchmarks.multicore.idle_with_pwm.nrf54h20dk_cpuapp_cpurad.s2ram' doesn't work as expected. No PWM signal after entering low power mode - LED2 lights up, then it's OFF forever. It looks like some PWM/GPIO configuration is missing after system restores from low power.

@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from 3ec29a8 to de6dc40 Compare September 19, 2024 06:19
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch 2 times, most recently from 6605d4d to 01b02c6 Compare September 19, 2024 07:27
@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 Publishing GitHub Action.

@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from 01b02c6 to 76d64b9 Compare September 19, 2024 09:10
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch 2 times, most recently from 6b4b85b to 83c201d Compare September 19, 2024 09:17
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from 83c201d to 6bed1f1 Compare September 19, 2024 12:03
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch 2 times, most recently from dd59b7a to 4c06f99 Compare September 20, 2024 06:07
@nordic-segl nordic-segl requested a review from a team as a code owner September 20, 2024 06:07
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from c145cbd to 96c9c45 Compare October 3, 2024 10:36
@nordic-segl nordic-segl requested a review from a team as a code owner October 3, 2024 10:36
@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 Oct 3, 2024
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from 96c9c45 to 7c968c2 Compare October 7, 2024 05:51
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch 4 times, most recently from b7381a8 to 6f12840 Compare October 8, 2024 13:04
@NordicBuilder NordicBuilder removed the DNM label Oct 8, 2024
@rlubos
Copy link
Contributor

rlubos commented Oct 8, 2024

@gmarull Please revisit

@gmarull gmarull dismissed their stale review October 8, 2024 13:25

addressed

@rlubos
Copy link
Contributor

rlubos commented Oct 8, 2024

@nrfconnect/ncs-co-build-system Ping

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see what this has to do with a dts property, you can access dts properties in Kconfig, where is that happening 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.

min-residency-us is defined in DTS:
https://github.com/nrfconnect/sdk-zephyr/blob/main/dts/common/nordic/nrf54h20.dtsi#L127

Core has to sleep for more than 'min-residency-us' in order to enter that specific low power state.
It is used in main.c:
https://github.com/nordic-segl/sdk-nrf/blob/NRFX-6370_PWM_with_s2ram/tests/benchmarks/multicore/idle_with_pwm/src/main.c#L93

Default value of 1000ms is greater than min-residency for S2RAM state.

Here
https://github.com/nordic-segl/sdk-nrf/blob/NRFX-6370_PWM_with_s2ram/tests/benchmarks/multicore/idle_with_pwm/testcase.yaml#L28
KConfig value is set to 500 ms, which is too low to enter the deepest S2RAM while sufficient to enter IDLE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use a multiplier then and in the code take the dts property and multiply it so it can work with any board/future soc?

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 think whatever I do it will not scale easily. Power states and min-residency-us is very specific to a target (in fact, core).

"in the code take the dts property"

  1. I can't take dts property of &s2ram as it is valid only for nrf54h20dk/nrf54h20/cpuapp.
  2. I can't take dts property of &idle because it's going to be removed:
    https://github.com/nrfconnect/sdk-zephyr/pull/2032/files#diff-3455660ed5585b71497d4674c6a03a570690d462406141ea0170df7c10f28391

Copy link
Contributor

Choose a reason for hiding this comment

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

as above

Comment on lines +24 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

these values come from dts? E.g.

        pwmleds {
                compatible = "pwm-leds";
                red_pwm_led: red_pwm_led {
                        pwms = <&pwm0 0 PWM_MSEC(20) PWM_POLARITY_INVERTED>;
                };
                green_pwm_led: green_pwm_led {
                        pwms = <&pwm0 1 PWM_MSEC(20) PWM_POLARITY_INVERTED>;
                };
                blue_pwm_led: blue_pwm_led {
                        pwms = <&pwm0 2 PWM_MSEC(20) PWM_POLARITY_INVERTED>;
                };
        };

So why are they being hacked 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.

20 ms is too high for this test. I wanted to set it in one place for all platforms.

Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

FILE_SUFFIX ?

Copy link
Contributor Author

@nordic-segl nordic-segl Oct 9, 2024

Choose a reason for hiding this comment

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

No 😄
I believe FILE_SUFFIX will be applied to overlays as well. I have one overlay for all three test configurations (no_sleep, idle, s2ram).
I just need a bunch of KConfigs to enter s2ram and idle.

Copy link
Contributor

Choose a reason for hiding this comment

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

It applies if the file exists, if not it uses the normal one without the suffix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this be addressed later? This PR blocks everybody working on Lilium.

Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is non blocking

Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

FILE_SUFFIX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above

@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from 5b72af8 to 88b69f3 Compare October 9, 2024 07:35
@nordic-segl nordic-segl requested a review from nordicjm October 9, 2024 07:45
nordic-segl and others added 4 commits October 9, 2024 10:43
Add test that confirms correct operation of PWM
when target enters and leaves low power mode.

Signed-off-by: Sebastian Głąb <[email protected]>
Instead of defining power states per test, add power states
to the nrf54h20/cpuapp target definition.

Remove power states from tests overlays to prevent
build failures due to duplicated node definitions.

Signed-off-by: Sebastian Głąb <[email protected]>
Add missing entry integration_platforms in testcase.yaml in:
/tests/benchamrks/multicore/idle_spim,
/tests/benchamrks/multicore/idle_twim,
/tests/benchamrks/multicore/idle_uarte.

Set value to nrf54h20dk/nrf54h20/cpuapp in all three tests.

Signed-off-by: Sebastian Głąb <[email protected]>
Remove power states from tests overlays to prevent
build failures due to duplicated node definitions.

Signed-off-by: Robert Lubos <[email protected]>
@nordic-segl nordic-segl force-pushed the NRFX-6370_PWM_with_s2ram branch from 88b69f3 to bf19429 Compare October 9, 2024 08:43
return ret;
}

/* Sleep 1 second */
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is wrong

@rlubos rlubos merged commit a10710c into nrfconnect:main Oct 9, 2024
11 of 13 checks passed
@nordic-segl nordic-segl deleted the NRFX-6370_PWM_with_s2ram branch October 9, 2024 09:42
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. manifest manifest-zephyr

Projects

None yet

Development

Successfully merging this pull request may close these issues.