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

Sanitycheck: Support filtering based on DTS configuration data #11800

Merged
merged 6 commits into from
Dec 11, 2018

Conversation

nashif
Copy link
Member

@nashif nashif commented Dec 3, 2018

Cleanup some hardcoded samples and depend on DTS data instead making the sample
more generic.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Dec 3, 2018

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.

@codecov-io
Copy link

codecov-io commented Dec 3, 2018

Codecov Report

Merging #11800 into master will increase coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11800      +/-   ##
==========================================
+ Coverage   48.05%    48.3%   +0.24%     
==========================================
  Files         281      280       -1     
  Lines       43413    43326      -87     
  Branches    10404    10373      -31     
==========================================
+ Hits        20864    20929      +65     
+ Misses      18400    18245     -155     
- Partials     4149     4152       +3
Impacted Files Coverage Δ
subsys/shell/shell_utils.h 50% <0%> (-30%) ⬇️
drivers/clock_control/nrf5_power_clock.c 40.17% <0%> (-11.93%) ⬇️
lib/cmsis_rtos_v1/cmsis_semaphore.c 60% <0%> (-10%) ⬇️
subsys/shell/shell_ops.c 19.11% <0%> (-3.96%) ⬇️
subsys/shell/shell_utils.c 40.41% <0%> (-3.59%) ⬇️
subsys/logging/log_core.c 71.24% <0%> (-3.43%) ⬇️
drivers/ethernet/eth_native_posix.c 23.62% <0%> (-3.17%) ⬇️
subsys/net/ip/ipv4.c 64.1% <0%> (-1.33%) ⬇️
subsys/net/ip/ipv6.c 54.23% <0%> (-0.69%) ⬇️
include/logging/log_backend.h 75% <0%> (-0.68%) ⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fbcf03...f3f741b. Read the comment docs.

Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

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

LGTM, would like @galak blessing

@galak
Copy link
Collaborator

galak commented Dec 4, 2018

I want to think about this a little before approving/merging, not sure what the implications are we remove dt fixup, move to codegen, etc.

@galak galak self-assigned this Dec 4, 2018
@nashif
Copy link
Member Author

nashif commented Dec 4, 2018

I want to think about this a little before approving/merging, not sure what the implications are we remove dt fixup, move to codegen, etc.

we can always change to whatever method later and when such changes come, right now we are just checking for variables that are being used in samples anyways and those would need to be changed in any case depending on how we move forward.
Right now the hardcoding in samples and coverage of tests is very limited because we have no other way of filtering.

@@ -13,8 +13,12 @@ GPIO outputs of the MCU/board.
Wiring
******

The code may need some work before running on another board: set PORT,
LED1 and LED2 according to the board's GPIO configuration.
This sample should work on board with multiple built-in LEDs without any
Copy link
Contributor

Choose a reason for hiding this comment

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

This is contradictory, so let's simplify and say:

This sample may need some code changes to run on specific
boards by setting the PORT, LED0, and LED1 according to
the board's GPIO configuration.  As examples, see the instructions
below for some of our supported boards.

In addition to Kconfig values from generated .config, also parse and
filter based on configs generated by device-tree.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif force-pushed the sample_test_cleanup branch from 85c9abe to ca45180 Compare December 11, 2018 13:27
galak
galak previously requested changes Dec 11, 2018
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Is there some way we can convey these filters are coming from DT data? Would be helpful when we replace this with eDTS info that we know that. Not sure if adding a 'dt_filter' in addition to 'filter'.

@nashif
Copy link
Member Author

nashif commented Dec 11, 2018

Is there some way we can convey these filters are coming from DT data? Would be helpful when we replace this with eDTS info that we know that. Not sure if adding a 'dt_filter' in addition to 'filter'.

no, there is no way I can think of, the filter also supports environment variables, some variables from cmake, Kconfig etc. So DT is not special here.

We should not block this because of EDTS, unless we have a clear idea and a plan about when EDTS is landing.

@nashif
Copy link
Member Author

nashif commented Dec 11, 2018

Is there some way we can convey these filters are coming from DT data? Would be helpful when we replace this with eDTS info that we know that. Not sure if adding a 'dt_filter' in addition to 'filter'.

also, Kconfig is always prefix with CONFIG_, DT stuff will hopefully all move to DT_, there will be some aliases that will have neither, those will be assumed to be either from DTS or env. The only problematic ones will be CONFIG_ variables that come from DT, but those are going away slowly.

@nashif nashif force-pushed the sample_test_cleanup branch from ca45180 to 5581bcc Compare December 11, 2018 15:51
Do not limit to whitelisted boards, instead make it build/run for any
board that defines required DTS macros.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Do not limit to whitelisted boards, instead make it build/run for any
board that defines required DTS macros.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Do not limit to whitelisted boards, instead make it build/run for any
board that defines required DTS macros.
Remove QMSI kconfigs and depend on DTS only.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Do not limit to whitelisted boards, instead make it build/run for any
board that defines required DTS macros.

Simplified documentation and made it more generic. Remove HW setup and
made sample work with built-in LEDs and buttons.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
This sample does not belong under basic/, it should have its own
category.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@galak galak merged commit 77bd2c2 into zephyrproject-rtos:master Dec 11, 2018
@nashif nashif deleted the sample_test_cleanup branch March 26, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants