Skip to content

samples: subsys: settings: Allow on nrf54L20 PDK #83165

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

samples: subsys: settings: Allow on nrf54L20 PDK #83165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions samples/subsys/settings/boards/nrf54l20pdk_nrf54l20_cpuapp.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CONFIG_NVS=y
CONFIG_SETTINGS_NVS=y
1 change: 1 addition & 0 deletions samples/subsys/settings/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ tests:
- nrf54l15dk/nrf54l05/cpuapp
- nrf54l15dk/nrf54l10/cpuapp
- nrf54l15dk/nrf54l15/cpuapp
- nrf54l20pdk/nrf54l20/cpuapp
Copy link
Contributor

@kartben kartben Dec 18, 2024

Choose a reason for hiding this comment

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

Suggested change
- nrf54l20pdk/nrf54l20/cpuapp

There is nothing to test, it's a sample (will follow up with a PR to clean up the rest of the list if I don't forget)
See https://docs.zephyrproject.org/latest/samples/sample_definition_and_criteria.html#twister-should-be-able-to-build-every-sample

Copy link
Contributor Author

@nordic-segl nordic-segl Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On that page/chapter there is:
"Use platform_allow to limit test to the platforms the sample was actually verified on. Those platforms should be listed in the sample’s README file."

I believe this sample doesn't work out-of-the box on every platform. At least, nRF boards require some KConfig tweaks. However, I see in the 'boards' directory overlay files for boards that are not listed under platform_allow.

I will add commit that removes platform_allow from this sample to check how CI reacts.
TBC

Copy link
Contributor Author

@nordic-segl nordic-segl Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

platform_allow must stay. Otherwise integration CI is failing:

INFO    - 2 of 27 executed test configurations passed (7.41%), 0 built (not run), 25 failed, 0 errored, with no warnings in 526.21 seconds.
(...)
INFO    - The following issues were found (showing the top 10 items):
INFO    - 1) sample.subsys.settings on m2gl025_miv/miv failed (Timeout)
INFO    - 2) sample.subsys.settings on mps2/an385 failed (Timeout)
INFO    - 3) sample.subsys.settings on mps2/an521/cpu0 failed (Timeout)
INFO    - 4) sample.subsys.settings on qemu_arc/qemu_arc_em failed (Timeout)
INFO    - 5) sample.subsys.settings on qemu_arc/qemu_arc_hs failed (Timeout)
INFO    - 6) sample.subsys.settings on qemu_arc/qemu_arc_hs/xip failed (Timeout)
INFO    - 7) sample.subsys.settings on qemu_arc/qemu_arc_hs5x failed (Timeout)
INFO    - 8) sample.subsys.settings on qemu_arc/qemu_arc_hs6x failed (Timeout)
INFO    - 9) sample.subsys.settings on qemu_cortex_a53/qemu_cortex_a53 failed (Timeout)
INFO    - 10) sample.subsys.settings on qemu_cortex_a53/qemu_cortex_a53/smp failed (Timeout)

Copy link
Contributor

@kartben kartben Dec 19, 2024

Choose a reason for hiding this comment

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

I never asked you to remove platform_allow altogether, just the line you'd added :) see my request for change.

Copy link
Contributor Author

@nordic-segl nordic-segl Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I remove that line, Twister will skip this test/sample on nrf54l20pdk because it's not in platform_allow.

So, what do You mean by "will follow up with a PR to clean up the rest of the list if I don't forget"?

Copy link
Contributor

@kartben kartben Dec 19, 2024

Choose a reason for hiding this comment

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

If I remove that line, Twister will skip this test/sample on nrf54l20pdk because it's not in platform_allow.

Yes, that's the whole point! :) There is no need for Twister to go through each sample and test them against dozen of platforms, this just doesn't scale in terms of project infrastructure.

So, what do You mean by "will follow up with a PR to clean up the rest of the list if I don't forget"?

I would like you to not add the new line, and by follow-up I mean I will clean up the other lines so that people stop unecessarily adding to this list and to keep Twister "testing" to the strict minimum, i.e test that the sample builds fine and runs fine on e.g native sim.

Copy link
Contributor Author

@nordic-segl nordic-segl Dec 20, 2024

Choose a reason for hiding this comment

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

If I remove that line, Twister will skip this test/sample on nrf54l20pdk because it's not in platform_allow.

Yes, that's the whole point! :) There is no need for Twister to go through each sample and test them against dozen of platforms, this just doesn't scale in terms of project infrastructure.

I was referring to Nordic's internal CI that executes on-target Twister tests. Currently, that's the only place where nrf54L20 PDK can be tested.
Based on Twister results we calculate metrics that "give overview" how well each target is tested, SW maturity level, etc. (this statement is an oversimplification).

So, what do You mean by "will follow up with a PR to clean up the rest of the list if I don't forget"?

I would like you to not add the new line, and by follow-up I mean I will clean up the other lines so that people stop unecessarily adding to this list and to keep Twister "testing" to the strict minimum, i.e test that the sample builds fine and runs fine on e.g native sim.

Nordic Management would like to have similar test coverage on nrf54L20 as on other platforms. As a result, I was given task to enable Twister tests on that target (in Nordic's internal CI but this also enables testing in the "upstream CI").
I need to discuss this with my boss because our expectations are conflicting. Unfortunately, he's out of office until January 2025.

So, I guess it's not as simple as run "upstream Twister" with options:
-T TESTSUITE_ROOT, --testsuite-root TESTSUITE_ROOT to select which tests to execute and/or
-P EXCLUDE_PLATFORM, --exclude-platform EXCLUDE_PLATFORM to skip testing on given platform and/or
-p PLATFORM, --platform PLATFORM to run tests on f.e. native sim only.

I will clean up the other lines so that people stop unecessarily adding to this list and to keep Twister "testing" to the strict minimum

Are other manufacturers aware that it's planned to limit "upstream Twister" scope in a way that silently limits test scope in "private Twister runs"?

- nrf54h20dk/nrf54h20/cpuapp
- s32z2xxdc2/s32z270/rtu0
- s32z2xxdc2/s32z270/rtu1
Expand Down
Loading