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.

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

Jump to bottom

Counter test pca10090 #15623

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Counter test pca10090 #15623

merged 2 commits into from
Apr 24, 2020

Conversation

nordic-krch
Copy link
Contributor

tests: drivers: counter: Add nrf9160_pca10090 board

Add board with nrf9160 to counter tests.

tests: drivers: counter: Add clock stabilization in test setup for nRF

Xtal LF clock source starts hundreds of milliseconds. When it is
not start, test may fail due to wrong timing. Added pending
on LF clock being start in test setup.

@nordic-krch nordic-krch requested a review from anangl April 23, 2019 14:16
@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 23, 2019 •
edited
Loading

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

galak
galak reviewed Apr 23, 2019
View reviewed changes
tests/drivers/counter/counter_basic_api/boards/nrf9160_pca10090.conf Outdated
@@ -0,0 +1,7 @@
CONFIG_COUNTER_TIMER0=y
CONFIG_COUNTER_TIMER0_PRESCALER=4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have the SoC code enable the various timers instead of having to add a board file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what do you mean. The configuration is different for boards as they have different number of RTCs and TIMERs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How is the number of RTCs & TIMERs board specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not board specific though every board here has different soc. Still i don't know what do you mean by Soc code in:

Can we have the SoC code enable the various timers

I don't want to enable all timers by default in those soc. Only in that test.

Copy link
Member

Choose a reason for hiding this comment

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

The issue I see, here, is that if someone wishes to have this test running for their nrf- board, they still need to add a board.conf. And we have ~20-25 nRF boards in the tree....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand we don't want to enable all counter instances by default. On the other hand, what is the value in testing same driver on different boards with same soc?
However, I see you point and ideally it would be good to have option to add .conf file for soc that would be applied to any board with given soc (like nrf9160.conf). @galak, @ioannisg Do we have option like that?

Copy link
Member

Choose a reason for hiding this comment

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

We don't have something like that, no. At least not that I am aware of @nordic-krch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@galak @ioannisg can we conclude here?
We have two options:

  • enable all instances in soc (i'm against since it's a waste of ram and rom, some you cannot enable like TIMER0 and RTC0 which are used by Bluetooth)
  • accept that each board that targets the test requires own board configuration

there is a third option to extend cmake to take conf files from <app>\soc folder. Then we would have specific option per soc which has value but i looked into cmake framework and it's not straightforward to do because contrary to BOARD, SoC name is taken from kconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@galak @ioannisg ping.

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 the second option is decent enough to be accepted

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from 6670153 to 1f2643a Compare April 24, 2019 05:29
@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch 2 times, most recently from 2710272 to 1184ef8 Compare April 25, 2019 06:30
@nordic-krch nordic-krch mentioned this pull request Apr 26, 2019
Merged
@galak galak added area: Counter area: Tests Issues related to a particular existing or missing test labels Apr 26, 2019
@nordic-krch nordic-krch added the platform: nRF Nordic nRFx label Apr 30, 2019
@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from 1184ef8 to a4c572f Compare May 15, 2019 09:47
@nordic-krch nordic-krch requested review from lemrey and nashif as code owners May 15, 2019 09:47
@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from a4c572f to 770ba4f Compare May 15, 2019 09:56
@ioannisg
Copy link
Member

ioannisg commented Aug 1, 2019

Do we want to have this in 2.0 release?

@nordic-krch
Copy link
Contributor Author

rebased

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch 2 times, most recently from 94c1fd0 to 645b257 Compare October 10, 2019 03:58
@nordic-krch
Copy link
Contributor Author

@ioannisg can you review? it seems you approved approach taken (#15623 (comment))

@ioannisg
Copy link
Member

Will take a look today

ioannisg
ioannisg reviewed Oct 11, 2019
View reviewed changes
boards/arm/nrf9160_pca10090/nrf9160_pca10090.yaml Outdated
@@ -12,3 +12,4 @@ supported:
- i2c
- pwm
- watchdog
- counter
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this could be added also to the _NS board

Copy link
Member

Choose a reason for hiding this comment

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

Just to be correct - the two board definitions (S , NS) ideally should be able to execute all counter tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should but enabling ns board in vanilla zephyr does not may much sense since there is no means to run it without spm.

ioannisg
ioannisg reviewed Oct 11, 2019
View reviewed changes
tests/drivers/counter/counter_basic_api/boards/nrf9160_pca10090.conf Outdated
@@ -0,0 +1,4 @@
CONFIG_COUNTER_TIMER0=y
Copy link
Member

Choose a reason for hiding this comment

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

You need to add license and copyright 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.

I usually don't see licenses in .conf files.

ioannisg
ioannisg reviewed Oct 11, 2019
View reviewed changes
tests/drivers/counter/counter_basic_api/src/test_counter.c Outdated
@@ -7,6 +7,10 @@
#include <drivers/counter.h>
#include <ztest.h>
#include <kernel.h>
#ifdef CONFIG_SOC_FAMILY_NRF
Copy link
Member

Choose a reason for hiding this comment

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

Why is the reason for this inclusion?
Ideally, generic samples/tests wouldn't need platform-specific inclusions, no?

Copy link
Member

Choose a reason for hiding this comment

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

OH, I see, now, that clock_control_on() is done for nRF SoCS only.

Copy link
Member

Choose a reason for hiding this comment

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

So, please, add some inline explanation for this, for the ease of reading ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

ioannisg
ioannisg reviewed Oct 11, 2019
View reviewed changes
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Looks good. I added some minor comments.
also, I would like to see license header and copyright in all affected files, if possible

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from 645b257 to f2f0bde Compare December 19, 2019 06:16
@jhedberg
Copy link
Member

jhedberg commented Jan 7, 2020

Any update on this PR? @ioannisg could you take another look?

@ioannisg ioannisg requested a review from pabigot January 16, 2020 12:15
@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from f2f0bde to 2098824 Compare January 29, 2020 13:28
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 29, 2020 •
edited
Loading

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from 2098824 to 10ce558 Compare January 29, 2020 13:53
@nordic-krch
Copy link
Contributor Author

PR rebased, @ioannisg can you take another look?

@jhedberg
Copy link
Member

This seems to have been neglected for awhile. I just triggered a Shippable re-run. @ioannisg could you take another look?

@nordic-krch
Copy link
Contributor Author

@ioannisg ping

@carlescufi
Copy link
Member

@nordic-krch could you please rebase?

@nordic-krch
CODEOWNERS: Adding codeowner for tests/drivers/counter
cfad1d1 
Added @nordic-krch as the codeowner.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from 10ce558 to ee4015f Compare April 23, 2020 07:51
@nordic-krch
Copy link
Contributor Author

rebased to celebrate first anniversary 🎂

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from ee4015f to d59cb22 Compare April 23, 2020 08:59
ioannisg
ioannisg requested changes Apr 23, 2020
View reviewed changes
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

@nordic-krch you need to rename the overlay files according to the new board names

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from d59cb22 to db7bd82 Compare April 23, 2020 09:15
@nordic-krch
Copy link
Contributor Author

@ioannisg done

@nordic-krch
Copy link
Contributor Author

Note that CI is failing on the same test as in #24374. Test is passing locally thus i have no idea how to fix it.

Created bug report for failing test: #24635

@stephanosio stephanosio mentioned this pull request Apr 23, 2020
Closed
@nordic-krch
Copy link
Contributor Author

@ioannisg can you take another look? CI fail is unrelated. Would be great to merge it on PR anniversary 😄

@ioannisg
Copy link
Member

@ioannisg can you take another look? CI fail is unrelated. Would be great to merge it on PR anniversary 😄

Or wait for the 2-y anniversary, instead?

@nordic-krch nordic-krch force-pushed the counter_test_pca10090 branch from db7bd82 to d6139e0 Compare April 24, 2020 05:04
@nordic-krch
tests: drivers: counter: Add nrf9160_pca10090 board
d6139e0 
Add board with nrf9160 to counter tests.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@ioannisg ioannisg merged commit 2c54ea4 into zephyrproject-rtos:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galak galak galak left review comments

@carlescufi carlescufi carlescufi left review comments

@anangl anangl anangl approved these changes

@ioannisg ioannisg ioannisg approved these changes

@lemrey lemrey Awaiting requested review from lemrey

@nashif nashif Awaiting requested review from nashif

@pabigot pabigot Awaiting requested review from pabigot

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

Assignees
No one assigned
Labels
area: Boards area: Counter area: Tests Issues related to a particular existing or missing test platform: nRF Nordic nRFx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nordic-krch @zephyrbot @ioannisg @jhedberg @carlescufi @galak @anangl