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

Multi image #1508

Merged
merged 1 commit into from
Dec 12, 2019
Merged

Multi image #1508

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 CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
# Point to NCS root directory.
set(NRF_DIR ${CMAKE_CURRENT_LIST_DIR} CACHE PATH "NCS root directory")

include(cmake/multi_image.cmake)

zephyr_include_directories(include)

add_subdirectory(ext)
Expand Down
180 changes: 180 additions & 0 deletions cmake/multi_image.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
#
# Copyright (c) 2019 Nordic Semiconductor ASA
#
# SPDX-License-Identifier: LicenseRef-BSD-5-Clause-Nordic
#

if(IMAGE_NAME)
set_property(
TARGET zephyr_property_target
APPEND_STRING
PROPERTY shared_vars
"set(${IMAGE_NAME}KERNEL_HEX_NAME ${KERNEL_HEX_NAME})\n"
)

set_property(
TARGET zephyr_property_target
APPEND_STRING
PROPERTY shared_vars
"list(APPEND ${IMAGE_NAME}BUILD_BYPRODUCTS ${PROJECT_BINARY_DIR}/${KERNEL_HEX_NAME})\n"
)

file(GENERATE OUTPUT ${CMAKE_BINARY_DIR}/shared_vars.cmake
CONTENT $<TARGET_PROPERTY:zephyr_property_target,shared_vars>
)
endif(IMAGE_NAME)

function(image_board_selection board_in board_out)
# It is assumed that only the root app will be built as non-secure.
# This is not a valid assumption as there might be multiple non-secure
# images defined.
# TODO: Allow multiple non-secure images by using Kconfig to set the
# secure/non-secure property rather than using a separate board definition.
if(${board_in} STREQUAL nrf9160_pca10090ns)
set(${board_out} nrf9160_pca10090 PARENT_SCOPE)
message("Changed board to secure nrf9160_pca10090 (NOT NS)")

elseif(${board_in} STREQUAL nrf9160_pca20035ns)
set(${board_out} nrf9160_pca20035 PARENT_SCOPE)
message("Changed board to secure nrf9160_pca20035 (NOT NS)")

elseif(${board_in} STREQUAL actinius_icarus_ns)
set(${board_out} actinius_icarus PARENT_SCOPE)
message("Changed board to secure actinius_icarus (NOT NS)")

else()
set(${board_out} ${board_in} PARENT_SCOPE)
endif()
endfunction()

function(add_child_image name sourcedir)
string(TOUPPER ${name} UPNAME)
SebastianBoe marked this conversation as resolved.
Show resolved Hide resolved

if (CONFIG_${UPNAME}_BUILD_STRATEGY_USE_HEX_FILE)
assert_exists(CONFIG_${UPNAME}_HEX_FILE)
SebastianBoe marked this conversation as resolved.
Show resolved Hide resolved
message("Using ${CONFIG_${UPNAME}_HEX_FILE} instead of building ${name}")
elseif (CONFIG_${UPNAME}_BUILD_STRATEGY_SKIP_BUILD)
message("Skipping building of ${name}")
else()
# Build normally
add_child_image_from_source(${name} ${sourcedir})
endif()
endfunction()

function(add_child_image_from_source name sourcedir)
message("\n=== child image ${name} begin ===")

# Construct a list of variables that, when present in the root
# image, should be passed on to all child images as well.
list(APPEND
SHARED_MULTI_IMAGE_VARIABLES
BOARD_DIR
ZEPHYR_MODULES
ZEPHYR_EXTRA_MODULES
ZEPHYR_TOOLCHAIN_VARIANT
GNUARMEMB_TOOLCHAIN_PATH
EXTRA_KCONFIG_TARGETS
)

foreach(kconfig_target ${EXTRA_KCONFIG_TARGETS})
list(APPEND
SHARED_MULTI_IMAGE_VARIABLES
EXTRA_KCONFIG_TARGET_COMMAND_FOR_${kconfig_target}
)
endforeach()

unset(image_cmake_args)
list(REMOVE_DUPLICATES SHARED_MULTI_IMAGE_VARIABLES)
foreach(shared_var ${SHARED_MULTI_IMAGE_VARIABLES})
if(DEFINED ${shared_var})
list(APPEND image_cmake_args
-D${shared_var}=${${shared_var}}
)
endif()
endforeach()

# Set ${name}_BOARD based on what BOARD is set to.
image_board_selection(${BOARD} ${name}_BOARD)

get_cmake_property(VARIABLES VARIABLES)
get_cmake_property(VARIABLES_CACHED CACHE_VARIABLES)

set(regex "^${name}_.+")

list(FILTER VARIABLES INCLUDE REGEX ${regex})
list(FILTER VARIABLES_CACHED INCLUDE REGEX ${regex})

foreach(var_name
${VARIABLES}
${VARIABLES_CACHED}
)
# This regex is guaranteed to match due to the filtering done
# above, we only re-run the regex to extract the part after
# '_'. We run the regex twice because it is believed that
# list(FILTER is faster than doing a string(REGEX on each item.
string(REGEX MATCH "^${name}_(.+)" unused_out_var ${var_name})
list(APPEND image_cmake_args
-D${CMAKE_MATCH_1}=${${var_name}}
)
endforeach()

file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/${name})
execute_process(
COMMAND ${CMAKE_COMMAND}
-G${CMAKE_GENERATOR}
${EXTRA_MULTI_IMAGE_CMAKE_ARGS} # E.g. --trace-expand
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you provide some more information on this ?
How is it intended to be used, cause I don't see this added in documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs added

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @ru-fu : 7ba41e1

-DIMAGE_NAME=${name}_
${image_cmake_args}
${sourcedir}
WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/${name}
RESULT_VARIABLE ret
)

set_property(DIRECTORY APPEND PROPERTY
CMAKE_CONFIGURE_DEPENDS
${CMAKE_BINARY_DIR}/${name}/zephyr/.config
)

if(NOT ${ret} EQUAL "0")
message(FATAL_ERROR "CMake generation for ${name} failed, aborting. Command: ${ret}")
endif()

message("=== child image ${name} end ===\n")

# Include some variables from the child image into the parent image
# namespace
include(${CMAKE_BINARY_DIR}/${name}/shared_vars.cmake)

# Increase the scope of this variable to make it more available
set(${name}_KERNEL_HEX_NAME ${${name}_KERNEL_HEX_NAME} CACHE STRING "" FORCE)

include(ExternalProject)
ExternalProject_Add(${name}_subimage
SOURCE_DIR ${sourcedir}
BINARY_DIR ${CMAKE_BINARY_DIR}/${name}
BUILD_BYPRODUCTS ${${name}_BUILD_BYPRODUCTS} # Set by shared_vars.cmake
CONFIGURE_COMMAND ""
BUILD_COMMAND ${CMAKE_MAKE_PROGRAM}
${EXTRA_MULTI_IMAGE_BUILD_OPT} # E.g. -v
Copy link
Contributor Author

@tejlmand tejlmand Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see where you want to go, question is, do we want to open this possibility now, through CMake.

or should we actually describe to user that it is possible to do:
ninja -C spm -v if verbose output from the child image build is requested.

That is, do we want to document:

cmake -DEXTRA_MULTI_IMAGE_BUILD_OPT='-v' ...
ninja                     # (This will give verbose build output for all child images but not parent )

or

cmake ...
ninja -C spm -v # (This will give verbose build output for spm child image)
ninja           # (build parent and other child image with normal build output)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC directly invoking child images can corrupt the build in certain situations so I'd rather not encourage this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC directly invoking child images can corrupt the build in certain situations so I'd rather not encourage this.

Not correct, cause we always rebuild child image if need be.
https://github.com/NordicPlayground/fw-nrfconnect-nrf/blob/48b08e23acb2628b8182cf4608447c498ffa49c4/cmake/multi_image.cmake#L155

This means that when running:

ninja -C spm
ninja

you will normally see:

$ ninja -C spm 
ninja: Entering directory `spm'
...
[179/179] Linking C executable zephyr/zephyr.elf

$ ninja 
[1/209] Preparing syscall dependency handling
[15/209] Performing build step for 'spm_subimage'
ninja: no work to do.
[21/209] Performing build step for 'mcuboot_subimage'
...
Merged /projects/github/ncs/nrf/samples/nrf9160/aws_fota/build/zephyr/app_signed.hex
$

Now, in this case, the child image was up-to-date.
Let's call that scenario 0.

Now, if we have the following two scenarios:

$ ninja -C spm 
ninja: Entering directory `spm'
...
[179/179] Linking C executable zephyr/zephyr.elf

$ ninja 
[1/209] Preparing syscall dependency handling
# Scenario 1: Something in build system touches child image dependencies at this build stage.
# Thus requiring a rebuild of spm_subimage.
[15/209] Performing build step for 'spm_subimage'
ninja: no work to do. # In scenario 1) a rebuild would occur instead.
# Scenario 2: Something in build system touches child image dependencies at this build stage.
# Thus requiring a rebuild of spm_subimage,
# but as spm_subimage is already built, there is a problem.
[21/209] Performing build step for 'mcuboot_subimage'
...
Merged /projects/github/ncs/nrf/samples/nrf9160/aws_fota/build/zephyr/app_signed.hex
$

Now, in scenario 1, everything is fine, because the child image will be rebuild with correct files and it does not matter how many times user has executed ninja -C spm.

In scenario 2 however, the dependencies are not correctly defined in CMake, and we can never trust the final result, as child image may be out of date.
In scenario 2) we have a more fundamental issue, that is not related to the user running ninja -C spm.

Copy link
Contributor

Choose a reason for hiding this comment

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

scenario 2 is that the child image depends on a file that is generated by the parent image right?

I think our current strategy is for the parent to express this dependency. If the user always invokes the parent then this works fine. If the child expresses this dependency I'm worried we might get a loop, or be invoking ninja/cmake more often than necesary.

So as I see it scenario 2 is about whether we encourage invoking ninja -C spm or not, as this determines whether dependencies are resolved correctly or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm saying I don't trust it when child images are invoked directly.

I am not aware of any dependency issues when the parent image is invoked.

I don't see how the always-outdated status of child-images relates to the scenario we are discussing, because this is a property of the parent image (which is not even invoked).

Copy link
Contributor

Choose a reason for hiding this comment

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

how would that ever happen

This is what I understood your proposal here was:

ExternalProject_Add(${name}_subimage
   ...
   DEPENDS  <parent_target-generating::file-generated-by-parent>
    )

Or was this code meant to be in the parent image?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still suggest we remove this special argument

As a compromise, I would be willing to remove the documentation, then users won't have to read about an option that there are very few use-cases for.

But I would like to keep it present so I don't have to modify sources when I need to add a flag to ninja.

Copy link
Contributor Author

@tejlmand tejlmand Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would still suggest we remove this special argument

As a compromise, I would be willing to remove the documentation, then users won't have to read about an option that there are very few use-cases for.

But I would like to keep it present so I don't have to modify sources when I need to add a flag to ninja.

I think we can close the discussion here as this PR is merged.
I do acknowledge users might want to do be able to increase verbosity on child image builds (although I would personally prefer ninja -C <path> -v) but if supporting it in the general, I would really prefer we stick to existing CMake flags.
Therefore I created PR #1650 to continue the discussion over there.

btw. always great with passionate developers, even though it also means disagreements from time to time 💥 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would that ever happen

ExternalProject_Add(${name}_subimage
   ...
   DEPENDS  <parent_target-generating::file-generated-by-parent>
    )

Or was this code meant to be in the parent image?

yep, that code was supposed to be in parent code, to ensure child is always built AFTER file has been generated by parent.

INSTALL_COMMAND ""
BUILD_ALWAYS True
)

foreach(kconfig_target
menuconfig
guiconfig
${EXTRA_KCONFIG_TARGETS}
)
add_custom_target(${name}_${kconfig_target}
${CMAKE_MAKE_PROGRAM} ${kconfig_target}
WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/${name}
USES_TERMINAL
)
endforeach()

set_property(
GLOBAL APPEND PROPERTY
PM_IMAGES
"${name}"
)
endfunction()
Loading