cmake: dts: move to specifying shield on the command line #12403

galak · 2019-01-09T19:39:04Z

Rather than specifying SHIELD via Kconfig, we move it to being specified
via the command line, similar to board.

codecov-io · 2019-01-09T20:15:46Z

Codecov Report

Merging #12403 into master will decrease coverage by 4.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12403      +/-   ##
==========================================
- Coverage   53.81%   49.73%   -4.09%     
==========================================
  Files         242      292      +50     
  Lines       27719    42433   +14714     
  Branches     6733     9969    +3236     
==========================================
+ Hits        14917    21102    +6185     
- Misses       9997    17180    +7183     
- Partials     2805     4151    +1346

Impacted Files	Coverage Δ
misc/printk.c	`84.93% <0%> (-2.09%)`	⬇️
kernel/init.c	`72.13% <0%> (-0.89%)`	⬇️
kernel/timeout.c	`86.11% <0%> (-0.26%)`	⬇️
subsys/net/ip/tcp.c	`56.93% <0%> (-0.13%)`	⬇️
subsys/shell/shell_cmds.c	`44.53% <0%> (ø)`	⬆️
include/sys_clock.h	`100% <0%> (ø)`	⬆️
include/kernel.h	`94.44% <0%> (ø)`	⬆️
include/misc/gcov.h
boards/posix/nrf52_bsim/main.c	`90.24% <0%> (ø)`
subsys/bluetooth/controller/hci/hci_driver.c	`67.32% <0%> (ø)`
... and 62 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 9707584...fef51e6. Read the comment docs.

erwango

This works fine, few comments plus following additions:

In samples/shields/x_nucleo_iks01a1/CMakeLists.txt:

# This sample is specific to x_nucleo_iks01a1 shield. Enforce -DSHIELD option
set(SHIELD x_nucleo_iks01a1)

Doc doc change required in doc/porting/shields.rst:

Shield activation
*****************

Activate support for a shield by adding the matching -DSHIELD arg to CMake
command

  .. zephyr-app-commands::
     :zephyr-app: your_app
     :gen-args: -DSHIELD=x_nucleo_iks01a1

samples/shields/x_nucleo_iks01a1/prj.conf

samples/shields/x_nucleo_iks01a1/sample.yaml

I'm now pushing this change

dbkinder

doc changes LGTM

galak · 2019-01-17T18:28:27Z

@SebastianBoe can you take a look, I'm also trying to figure out how to export ${shield_list} to cmake/app/boilerplate.cmake, but can't figure out scope rules.

SebastianBoe · 2019-01-18T09:00:21Z

I need more information to be able to help you.

What seems to be the problem?

galak · 2019-01-18T13:02:41Z

I need more information to be able to help you.

What seems to be the problem?

I wanted to print ${shield_list} in the usage message, when I try to access it in cmake/usage/usage.cmake its empty.

SebastianBoe · 2019-01-18T13:19:35Z

I see.

usage.cmake is executed by it's own CMake process, see https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/usage/CMakeLists.txt#L5.

So it only has access to the variables that has been explicitly exported to it, see https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/usage/CMakeLists.txt#L6 for an example of exporting a variable to usage.cmake.

Also, usage.cmake is executed twice, so any additional variables must be applied both here:

https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/usage/CMakeLists.txt#L6

and here:

https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/extensions.cmake#L1114

zephyrbot · 2019-01-18T13:44:28Z

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 · 2019-01-18T13:53:04Z

I see.

usage.cmake is executed by it's own CMake process, see /cmake/usage/CMakeLists.txt@master#L5.

So it only has access to the variables that has been explicitly exported to it, see /cmake/usage/CMakeLists.txt@master#L6 for an example of exporting a variable to usage.cmake.

Also, usage.cmake is executed twice, so any additional variables must be applied both here:

/cmake/usage/CMakeLists.txt@master#L6

and here:

/cmake/extensions.cmake@master#L1114

Ah thanks, that makes sense now.

galak · 2019-01-18T14:04:51Z

@SebastianBoe can you re-review the full PR. Should be complete now.

cmake/app/boilerplate.cmake

SebastianBoe · 2019-01-18T14:12:32Z

cmake/app/boilerplate.cmake

+
+    list(APPEND SHIELD_LIST ${shield})
+    if(DEFINED SHIELD)
+    if(${shield} STREQUAL ${SHIELD})


With Kconfig, it was possible to enable multiple shields.

To retain this functionality we need to check if ${shield} is in the list ${SHIELD}.

... and possibly rename from SHIELD to SHIELDS.

How would we specify multiple shields, as a 'space separated' list?

-DSHIELD="frdm_kw41z x_nucleo_iks01a1"

Yes, CONF_FILE should be able to demonstrate how to do it if you see any problems.

Yes, CONF_FILE should be able to demonstrate how to do it if you see any problems.

Ok, should now handle this case.

Rather than specifying SHIELD via Kconfig, we move it to being specified via the command line, similar to board. So we can do: -DSHIELD=x_nucleo_iks01a1 or, for multiple shields: -DSHIELD="x_nucleo_iks01a1 frdm_kw41z" Following cmake change, update x_nucleo_iks01a1 sample in order not to enable CONFIG option anymore but set SHIELD cmake option. Last, update documentation to reflect this change. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org> Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Following change of selection mechanism, clean boards/shields from Kconfig related code. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

MaureenHelm · 2019-01-18T19:01:25Z

samples/bluetooth/peripheral_hr/CMakeLists.txt

+if(BOARD STREQUAL mimxrt1020_evk OR
+   BOARD STREQUAL mimxrt1050_evk OR
+   BOARD STREQUAL mimxrt1060_evk OR
+   BOARD STREQUAL frdm_k64f)


Well that's unfortunate. :(

Eventually we are going to need something more sophisticated than CMake variables to give input to DT.

Like a separate Kconfig root/tree for DT for instance.

MaureenHelm · 2019-01-18T19:05:40Z

samples/bluetooth/peripheral_hr/sample.yaml

-    harness: bluetooth
-    extra_args: OVERLAY_CONFIG=overlay-shield_frdm_kw41z.conf
-    platform_whitelist: mimxrt1020_evk mimxrt1050_evk mimxrt1060_evk frdm_k64f
+    platform_whitelist: qemu_cortex_m3 qemu_x86 mimxrt1020_evk mimxrt1050_evk mimxrt1060_evk frdm_k64f


Instead of the modification to CMakeLists.txt, could you leave the two tests and set extra_args: SHIELD=frdm_kw41 in the second one?

I went ahead and merged this as I wanted to close on the base -DSHIELD support this week. I'll open a new PR for reworking the bluetooth test as you've asked in here.

I opened PR #12585 for the BT Periph Sample change.

After the rework in zephyrproject-rtos#12403, specifying a shield which has overlay out of the tree causes unnecessary inclusions of overlays. For every board root, overlays that have same index as expected overlay are being included. Fix this by removing already included overlays from SHIELD list. Signed-off-by: Marcin Szymczyk <Marcin.Szymczyk@nordicsemi.no>

After the rework in #12403, specifying a shield which has overlay out of the tree causes unnecessary inclusions of overlays. For every board root, overlays that have same index as expected overlay are being included. Fix this by removing already included overlays from SHIELD list. Signed-off-by: Marcin Szymczyk <Marcin.Szymczyk@nordicsemi.no>

After the rework in zephyrproject-rtos#12403, specifying a shield which has overlay out of the tree causes unnecessary inclusions of overlays. For every board root, overlays that have same index as expected overlay are being included. Fix this by removing already included overlays from SHIELD list. Signed-off-by: Marcin Szymczyk <Marcin.Szymczyk@nordicsemi.no>

After the rework in #12403, specifying a shield which has overlay out of the tree causes unnecessary inclusions of overlays. For every board root, overlays that have same index as expected overlay are being included. Fix this by removing already included overlays from SHIELD list. Signed-off-by: Marcin Szymczyk <Marcin.Szymczyk@nordicsemi.no>

galak added the DNM This PR should not be merged (Do Not Merge) label Jan 9, 2019

galak requested review from jhedberg, MaureenHelm, nashif, SebastianBoe, sjanc and Vudentz as code owners January 9, 2019 19:39

galak changed the title ~~cmake: dts: move to specifying shield on the command line~~ [RFC] cmake: dts: move to specifying shield on the command line Jan 9, 2019

galak added area: Devicetree area: Build System labels Jan 9, 2019

erwango previously requested changes Jan 10, 2019

View reviewed changes

samples/shields/x_nucleo_iks01a1/prj.conf Show resolved Hide resolved

samples/shields/x_nucleo_iks01a1/sample.yaml Outdated Show resolved Hide resolved

erwango force-pushed the shield branch from bb7abba to 4502c44 Compare January 14, 2019 15:51

galak requested a review from dbkinder as a code owner January 14, 2019 15:51

erwango removed the DNM This PR should not be merged (Do Not Merge) label Jan 14, 2019

dbkinder approved these changes Jan 14, 2019

View reviewed changes

erwango force-pushed the shield branch from 4502c44 to 6de701f Compare January 15, 2019 08:22

erwango mentioned this pull request Jan 15, 2019

Complete x-nucleo-iks01a2 shield support #12398

Merged

erwango force-pushed the shield branch from 6de701f to 4b5330c Compare January 16, 2019 09:35

galak force-pushed the shield branch 4 times, most recently from 5e45e93 to dfe3839 Compare January 17, 2019 00:04

galak changed the title ~~[RFC] cmake: dts: move to specifying shield on the command line~~ cmake: dts: move to specifying shield on the command line Jan 17, 2019

galak force-pushed the shield branch from dfe3839 to 46db672 Compare January 18, 2019 13:39

galak force-pushed the shield branch from 46db672 to 8f1df7d Compare January 18, 2019 14:03

SebastianBoe requested changes Jan 18, 2019

View reviewed changes

galak force-pushed the shield branch 2 times, most recently from 45b55ec to 737f41e Compare January 18, 2019 15:20

SebastianBoe approved these changes Jan 18, 2019

View reviewed changes

galak force-pushed the shield branch from 737f41e to c4e8318 Compare January 18, 2019 16:30

erwango added 2 commits January 18, 2019 12:09

boards/shields: Clean up Kconfig'ery

fef51e6

Following change of selection mechanism, clean boards/shields from Kconfig related code. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

galak force-pushed the shield branch from c4e8318 to fef51e6 Compare January 18, 2019 18:16

MaureenHelm reviewed Jan 18, 2019

View reviewed changes

galak merged commit 5a197bf into zephyrproject-rtos:master Jan 18, 2019

galak deleted the shield branch January 18, 2019 19:22

masz-nordic mentioned this pull request Apr 3, 2019

cmake: fix support for out of tree shields #15158

Merged

erwango mentioned this pull request Jul 3, 2019

boards: shields: use Kconfig.defconfig system for shields #17310

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: dts: move to specifying shield on the command line #12403

cmake: dts: move to specifying shield on the command line #12403

galak commented Jan 9, 2019 •

edited by erwango

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

erwango left a comment

dbkinder left a comment

galak commented Jan 17, 2019

SebastianBoe commented Jan 18, 2019

galak commented Jan 18, 2019

SebastianBoe commented Jan 18, 2019

zephyrbot commented Jan 18, 2019 •

edited

Loading

galak commented Jan 18, 2019

galak commented Jan 18, 2019

SebastianBoe Jan 18, 2019 •

edited

Loading

galak Jan 18, 2019

SebastianBoe Jan 18, 2019

galak Jan 18, 2019

MaureenHelm Jan 18, 2019

SebastianBoe Jan 21, 2019

MaureenHelm Jan 18, 2019

galak Jan 18, 2019

galak Jan 18, 2019

cmake: dts: move to specifying shield on the command line #12403

cmake: dts: move to specifying shield on the command line #12403

Conversation

galak commented Jan 9, 2019 • edited by erwango Loading

codecov-io commented Jan 9, 2019 • edited Loading

Codecov Report

erwango left a comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

galak commented Jan 17, 2019

SebastianBoe commented Jan 18, 2019

galak commented Jan 18, 2019

SebastianBoe commented Jan 18, 2019

zephyrbot commented Jan 18, 2019 • edited Loading

galak commented Jan 18, 2019

galak commented Jan 18, 2019

SebastianBoe Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galak commented Jan 9, 2019 •

edited by erwango

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

zephyrbot commented Jan 18, 2019 •

edited

Loading

SebastianBoe Jan 18, 2019 •

edited

Loading