Skip to content

cmake: introduce snippets for common configuration #40669

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 4 commits into from
Closed

cmake: introduce snippets for common configuration #40669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
068de6d
cmake: introduce "snippet" concept
Nov 25, 2021
f022d60
west_commands: build: remove deprecated `-s` opt
Nov 25, 2021
2e9aac5
west_commands: build: add `--snippet` arg
Nov 25, 2021
7b43f08
boards: nrf52840dk_nrf52840: snippets example
Nov 25, 2021
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
3 changes: 3 additions & 0 deletions boards/arm/nrf52840dk_nrf52840/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,7 @@ config BOARD
config BT_CTLR
default BT

config USE_DT_CODE_PARTITION
default y

endif # BOARD_NRF52840DK_NRF52840
34 changes: 34 additions & 0 deletions boards/arm/nrf52840dk_nrf52840/dts/mb_partitions.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright (c) 2022, Commonwealth Scientific and Industrial Research
* Organisation (CSIRO) ABN 41 687 119 230.
*
* SPDX-License-Identifier: Apache-2.0
*/

/delete-node/ &standalone_partition;

&flash0 {

partitions {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;

boot_partition: partition@0 {
label = "mcuboot";
reg = <0x000000000 0x0000C000>;
};
slot0_partition: partition@c000 {
label = "image-0";
reg = <0x0000C000 0x00067000>;
};
slot1_partition: partition@73000 {
label = "image-1";
reg = <0x00073000 0x00067000>;
};
scratch_partition: partition@da000 {
label = "image-scratch";
reg = <0x000da000 0x0001e000>;
};
};
};
25 changes: 5 additions & 20 deletions boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
zephyr,bt-c2h-uart = &uart0;
zephyr,sram = &sram0;
zephyr,flash = &flash0;
zephyr,code-partition = &slot0_partition;
zephyr,code-partition = &standalone_partition;
};

leds {
Expand Down Expand Up @@ -251,29 +251,14 @@ arduino_spi: &spi3 {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;

boot_partition: partition@0 {
label = "mcuboot";
reg = <0x00000000 0x0000C000>;
};
slot0_partition: partition@c000 {
label = "image-0";
reg = <0x0000C000 0x00067000>;
};
slot1_partition: partition@73000 {
label = "image-1";
reg = <0x00073000 0x00067000>;
};
scratch_partition: partition@da000 {
label = "image-scratch";
reg = <0x000da000 0x0001e000>;
};

/*
* The flash starting at 0x000f8000 and ending at
* 0x000fffff is reserved for use by the application.
*/

standalone_partition: partition@0 {
label = "standalone";
reg = <0x000000000 0x000F8000>;
};
/*
* Storage partition will be used by FCB/LittleFS/NVS
* if enabled.
Expand Down
14 changes: 14 additions & 0 deletions boards/arm/nrf52840dk_nrf52840/snippets/mb_boot.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2022, Commonwealth Scientific and Industrial Research
* Organisation (CSIRO) ABN 41 687 119 230.
*
* SPDX-License-Identifier: Apache-2.0
*/

#include "mb_partitions.dtsi"

/ {
chosen {
zephyr,code-partition = &boot_partition;
};
};
14 changes: 14 additions & 0 deletions boards/arm/nrf52840dk_nrf52840/snippets/mb_slot0.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2022, Commonwealth Scientific and Industrial Research
* Organisation (CSIRO) ABN 41 687 119 230.
*
* SPDX-License-Identifier: Apache-2.0
*/

#include "mb_partitions.dtsi"

/ {
chosen {
zephyr,code-partition = &slot0_partition;
};
};
1 change: 1 addition & 0 deletions boards/arm/nrf52840dk_nrf52840/snippets/usb.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CONFIG_USB_DEVICE_STACK=y
11 changes: 11 additions & 0 deletions boards/arm/nrf52840dk_nrf52840/snippets/usb.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright (c) 2021, Commonwealth Scientific and Industrial Research
* Organisation (CSIRO) ABN 41 687 119 230.
*
* SPDX-License-Identifier: Apache-2.0
*/

&usbd {
compatible = "nordic,nrf-usbd";
status = "okay";
};
34 changes: 34 additions & 0 deletions cmake/modules/configuration_files.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,24 @@ elseif(EXISTS ${APPLICATION_CONFIG_DIR}/prj.conf)
set(CONF_FILE_INCLUDE_FRAGMENTS true)
endif()

# Append any .conf files from snippets
if(DEFINED SNIPPETS)
message(STATUS "Snippets: ${SNIPPETS}")
# Convert from space-separated snippets into snippet list
string(REPLACE " " ";" SNIPPETS_RAW_LIST "${SNIPPETS}")
# Search for (optional) ${SNIPPET}.conf files
foreach(SNIPPET IN LISTS SNIPPETS_RAW_LIST)
# Application level board snippets have precedence
if(EXISTS ${APPLICATION_SOURCE_DIR}/boards/${BOARD}/snippets/${SNIPPET}.conf)
string(APPEND CONF_FILE " ${APPLICATION_SOURCE_DIR}/boards/${BOARD}/snippets/${SNIPPET}.conf")
set(CONF_FILE_INCLUDE_FRAGMENTS true)
elseif(EXISTS ${BOARD_DIR}/snippets/${SNIPPET}.conf)
string(APPEND CONF_FILE " ${BOARD_DIR}/snippets/${SNIPPET}.conf")
set(CONF_FILE_INCLUDE_FRAGMENTS true)
endif()
endforeach()
endif()

if(CONF_FILE_INCLUDE_FRAGMENTS)
zephyr_file(CONF_FILES ${APPLICATION_CONFIG_DIR}/boards KCONF CONF_FILE BUILD ${CONF_FILE_BUILD_TYPE})
endif()
Expand Down Expand Up @@ -108,5 +126,21 @@ alternate .overlay file using this parameter. These settings will override the \
settings in the board's .dts file. Multiple files may be listed, e.g. \
DTC_OVERLAY_FILE=\"dts1.overlay dts2.overlay\"")

if(DEFINED SNIPPETS)
# Search for (required) ${SNIPPET}.overlay and (optional) ${SNIPPET}.cmake files
foreach(SNIPPET IN LISTS SNIPPETS_RAW_LIST)
# Application level board snippets have precedence
if(EXISTS ${APPLICATION_SOURCE_DIR}/boards/${BOARD}/snippets/${SNIPPET}.overlay)
string(APPEND DTC_OVERLAY_FILE " ${APPLICATION_SOURCE_DIR}/boards/${BOARD}/snippets/${SNIPPET}.overlay")
include(${APPLICATION_SOURCE_DIR}/boards/${BOARD}/snippets/${SNIPPET}.cmake OPTIONAL NO_POLICY_SCOPE)
elseif(EXISTS ${BOARD_DIR}/snippets/${SNIPPET}.overlay)
string(APPEND DTC_OVERLAY_FILE " ${BOARD_DIR}/snippets/${SNIPPET}.overlay")
include(${BOARD_DIR}/snippets/${SNIPPET}.cmake OPTIONAL NO_POLICY_SCOPE)
else()
message(FATAL_ERROR "Snippet ${SNIPPET} does not exist in ${APPLICATION_SOURCE_DIR}/boards/${BOARD}/snippets or ${BOARD_DIR}/snippets")
endif()
endforeach()
endif()

# The DTC_OVERLAY_FILE variable is now set to its final value.
zephyr_boilerplate_watch(DTC_OVERLAY_FILE)
4 changes: 2 additions & 2 deletions doc/develop/west/sign.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ from the :file:`zephyrproject` workspace you created in the

.. code-block:: console

west build -b YOUR_BOARD -s bootloader/mcuboot/boot/zephyr -d build-mcuboot
west build -b YOUR_BOARD -s zephyr/samples/hello_world -d build-hello-signed -- \
west build -b YOUR_BOARD bootloader/mcuboot/boot/zephyr -d build-mcuboot
west build -b YOUR_BOARD zephyr/samples/hello_world -d build-hello-signed -- \
-DCONFIG_BOOTLOADER_MCUBOOT=y \
-DCONFIG_MCUBOOT_SIGNATURE_KEY_FILE=\"bootloader/mcuboot/root-rsa-2048.pem\"

Expand Down
15 changes: 5 additions & 10 deletions scripts/west_commands/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
BUILD_USAGE = '''\
west build [-h] [-b BOARD[@REV]]] [-d BUILD_DIR]
[-t TARGET] [-p {auto, always, never}] [-c] [--cmake-only]
[-n] [-o BUILD_OPT] [-f]
[-n] [-o BUILD_OPT] [-f] [-s SNIPPET]
Copy link
Contributor

@tejlmand tejlmand Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Two comments on this:

  • Although -s has been deprecated for some time, then I think it's good practice to have a period between removing the argument before using that same argument for something new.
  • Why should snippet have it's own west build argument when CONF_FILE, DTC_OVERLAY_FILE, SHIELD etc. doesn't have ?

Copy link
Contributor Author

@JordanYates JordanYates Nov 26, 2021

Choose a reason for hiding this comment

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

  • Although -s has been deprecated for some time, then I think it's good practice to have a period between removing the argument before using that same argument for something new.

In an ideal world yes, but the end result is going to be the same. If -s is removed, any scripts using it will crash. If -s is reused, any scripts using it will crash. Unfortunately I can't go back and remove it a year ago. Of course if the name changes from "snippet" then there is no need to use -s.

  • Why should snippet have it's own west build argument when CONF_FILE, DTC_OVERLAY_FILE, SHIELD etc. doesn't have?

Because its goal is to cover common use cases, which IMO means the ease-of-use of west build -b board -s usb samples/basic/minimal over west build -b board samples/basic/minimal -- -DSNIPPETS="usb" warrants it.

Copy link
Contributor

@tejlmand tejlmand Nov 29, 2021

Choose a reason for hiding this comment

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

I would say -DOVERLAY_CONFIG or DTC_OVERLAY_FILE are much better candidates for having extra arguments than snippets.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Dec 3, 2021

Choose a reason for hiding this comment

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

I disagree; I think snippets do rise to the level of deserving their own option.

I want to ask what the practical differences between snippets and shields are at this point, in terms of their behavior. Shields seem to be a special case of snippets, morally speaking.

Taking @petejohanson since he's taken an interest in adding more comprehensive shield related options to west build elsewhere.

[source_dir] -- [cmake_opt [cmake_opt ...]]
'''

Expand Down Expand Up @@ -100,10 +100,10 @@ def do_add_parser(self, parser_adder):

parser.add_argument('-b', '--board',
help='board to build for with optional board revision')
# Hidden option for backwards compatibility
parser.add_argument('-s', '--source-dir', help=argparse.SUPPRESS)
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Dec 3, 2021

Choose a reason for hiding this comment

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

I totally get what you're doing here, and although I use this option every day, I would be willing to part ways with it in the name of making progress on this more important issue (assuming "snippet" is chosen).

But there are a few places in the docs that are using still:

doc/guides/west/sign.rst
29:   west build -b YOUR_BOARD -s bootloader/mcuboot/boot/zephyr -d build-mcuboot
30:   west build -b YOUR_BOARD -s zephyr/samples/hello_world -d build-hello-signed -- \

doc/guides/dts/howtos.rst
35:   west build -b qemu_cortex_m3 -s samples/hello_world --cmake-only

Those would need cleanup as well, pending resolution of the bikeshed debate.

parser.add_argument('-d', '--build-dir',
help='build directory to create or use')
parser.add_argument('-s', '--snippet', default=[], action='append',
help='''snippets to apply when building''')
self.add_force_arg(parser)

group = parser.add_argument_group('cmake and build tool')
Expand Down Expand Up @@ -137,17 +137,10 @@ def do_run(self, args, remainder):
self.config_board = config_get('board', None)
log.dbg('args: {} remainder: {}'.format(args, remainder),
level=log.VERBOSE_EXTREME)
# Store legacy -s option locally
source_dir = self.args.source_dir
self._parse_remainder(remainder)
# Parse testcase.yaml or sample.yaml files for additional options.
if self.args.test_item:
self._parse_test_item()
if source_dir:
if self.args.source_dir:
log.die("source directory specified twice:({} and {})".format(
source_dir, self.args.source_dir))
self.args.source_dir = source_dir
log.dbg('source_dir: {} cmake_opts: {}'.format(self.args.source_dir,
self.args.cmake_opts),
level=log.VERBOSE_EXTREME)
Expand Down Expand Up @@ -440,6 +433,8 @@ def _run_cmake(self, board, origin, cmake_opts):
cmake_opts = []
if self.args.cmake_opts:
cmake_opts.extend(self.args.cmake_opts)
if self.args.snippet:
cmake_opts.extend(['-DSNIPPETS={}'.format(" ".join(self.args.snippet))])

user_args = config_get('cmake-args', None)
if user_args:
Expand Down
1 change: 1 addition & 0 deletions scripts/west_commands/completion/west-completion.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,7 @@ __comp_west_build()

local build_args_opts="
--board -b
--snippet -s
--build-dir -d
--target -t
--pristine -p
Expand Down