Multi image #1508

tejlmand · 2019-11-15T09:23:59Z

This PR introduces the use of multi image builds using external process for CMake configure and external project for building.

It removes the need for ${IMAGE} in code.
It relates to the following set of PRs:

TODO:

NordicBuilder · 2019-11-15T09:26:50Z

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.

tejlmand · 2019-11-18T10:06:29Z

@SebastianBoe great with the ToDo list introduced.
But I would like to suggest that the list is split in two,

ToDo's that ensures that the same functionality as previous solution is ensure, such as: PM files, mergehex, etc.
ToDo's with extended functionality, such as: partial re-configuration (only some images re-run CMake)

SebastianBoe · 2019-11-18T11:15:04Z

The parital-reconfiguration TODO is not about extending functionality, just about understanding the consequences of the current design.

I believe all TODOs are about ensuring we get as close as possible to the same functionality as before.

tejlmand · 2019-11-18T11:49:14Z

The parital-reconfiguration TODO is not about extending functionality, just about understanding the consequences of the current design.

Then I might have misread that TODO, cause with existing solution, a change in any <subimage>/zephyr/.config file will cause the CMake for all images to rerun, cause they all share the same CMake instance.
That is why I read the partial reconf. as a new possibility.

SebastianBoe · 2019-11-18T11:52:37Z

Not sure what you mean by same CMake instance. A single CMake instance has a reconfigure dependency on all .config files.

But, is it possible, in the current design, for a sub-image to re-configure (without modifying it's .config), without this triggering a re-configure for all images, and if so should this, or should this not be supported.

EDIT: I suspect it is possible and it should not be supported, but I haven't investigated yet.

mbolivar · 2019-11-18T15:07:59Z

Create snapshots (git references) for each repo that has it's history re-written

To clarify our discussion offline, we are only still considering this for zephyr and mcuboot, right? Other repositories will simply be pointed back at upstream.

tejlmand · 2019-11-19T06:49:11Z

Create snapshots (git references) for each repo that has it's history re-written

To clarify our discussion offline, we are only still considering this for zephyr and mcuboot, right? Other repositories will simply be pointed back at upstream.

That's my proposal.
And I believe @carlescufi agrees as well.

cmake/multi_image.cmake

carlescufi · 2019-11-19T12:47:10Z

Create snapshots (git references) for each repo that has it's history re-written

To clarify our discussion offline, we are only still considering this for zephyr and mcuboot, right? Other repositories will simply be pointed back at upstream.

That's my proposal.
And I believe @carlescufi agrees as well.

Yes, that's correct. For other repos we'll just point to master of the upstream counterparts.

tejlmand · 2019-11-19T14:31:47Z

@carlescufi @mbolivar manifest now points upstream

mbolivar · 2019-11-19T16:10:31Z

@carlescufi @mbolivar manifest now points upstream

Excellent. I believe then that this comment in west.yml no longer applies:

    # The projects below are included for completeness, but they are
    # not known to work properly with the NCS multi-image / partition
    # manager patchset. Please report any issues.
    #
    # https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/scripts/partition_manager/partition_manager.html

The reason these were segregated is because these modules' CMakeLists.txt files had not been audited for IMAGE prefixes. Now that this prefix is no longer necessary, I believe we can unify the two lists of projects, right?

ru-fu

I added another commit with updates and some questions.

doc/nrf/ug_multi_image.rst

SebastianBoe · 2019-12-05T11:32:24Z

@ru-fu : Please re-assess the review

ru-fu

The docs look good.

SebastianBoe · 2019-12-06T13:54:50Z

The DOC failure is in

nrf/rst/doc/nrf/samples/bluetooth/mesh/light/README.rst

and is not our fault, it is a known CI issue.

hakonfam

Tested with 10090ns sample with mcuboot and secure bootloader.

Verified that building of S1 candidate of mcuboot works correctly, and that second mcuboot update to S0 also works.

tejlmand

Looking good.
Only a small couple of minor comments, so great work: 👍

Added one more commit: 8a2093c

to ensure toolchain and board dir are propagated to child images.
This could especially be important to SES.

Feel free to squash the additional commit into existing work.

cmake/multi_image.cmake

tejlmand · 2019-12-11T13:21:59Z

cmake/multi_image.cmake

+  execute_process(
+    COMMAND ${CMAKE_COMMAND}
+    -G${CMAKE_GENERATOR}
+    ${EXTRA_MULTI_IMAGE_CMAKE_ARGS} # E.g. --trace-expand


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

FYI @ru-fu : 7ba41e1

tejlmand · 2019-12-11T13:36:29Z

cmake/multi_image.cmake

+    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


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)

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

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.

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.

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).

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?

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 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 💥 😄

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.

doc/nrf/ug_multi_image.rst

This commit introduces the use of CMake External Project for multi-image builds. This commit also updates the manifest file to include the multi-image updates in the following zephyr modules - zephyr - mcuboot - nrfxlib It updates the manifest file to track upstream zephyr vewrsion of the folowing zephyr modules - tinycbor - mbedtls - mcumgr Signed-off-by: Torsten Rasmussen <[email protected]>

ru-fu

I see the PR is merged already, but please update the docs as commented.

doc/nrf/ug_multi_image.rst

sigvartmh · 2019-12-13T11:21:08Z

This PR seems to have broken fota_download through so I would assume this is broken for everything which uses PM?

[0:00:25.458,862] <err> fota_flash_block: Unable to determine flash sector size
[00:00:25.458,892] <err> fota_flash_block: flash_progressive_erase error -19 offset=0x00000000

Emitted from zephyr/subsys/dfu/img_util/flash_img.c

SebastianBoe · 2019-12-13T11:22:39Z

Will look into it. Steps to reproduce?

sigvartmh · 2019-12-13T11:28:09Z

Intermittent error it seems was able to rebuild and it ran successfully with this patch.
Hence unable to reproduce.

nvlsianpu · 2019-12-19T08:42:17Z

west.yml

      path: modules/lib/tinycbor
-      revision: 86d5ed5dd544b107d2d0882961a19c2c6cb06572
+      revision: 0fc68fceacd1efc1ce809c5880c380f3d98b7b6e


@de-nordic Any idea why tinycbor reference should be zephyr upsteream and not NCS fork?

@de-nordic Any idea why tinycbor reference should be zephyr upsteream and not NCS fork?

Not instantly. I am looking into this.

This is incorrect. There should be no outside reference.

What should have been done instead is update to NCS fork of tinycbor (fw-nrfconnect-tinycbor) and reference to that update.

The diff between these two SHA's is only removing IMAGE prefixes. So I believe this is correct.

When there is no difference between NCS and zephyrproject it is policy to directly use zephyrproject.

cd ncs/modules/lib/tinycbor/ git remote add us https://github.com/zephyrproject-rtos/tinycbor git difftool 86d5ed5dd544b107d2d0882961a19c2c6cb06572 0fc68fceacd1efc1ce809c5880c380f3d98b7b6e

When there is no difference between NCS and zephyrproject it is policy to directly use zephyrproject.

OK, so if this is accepted policy then OK.

tejlmand added the DNM label Nov 15, 2019

tejlmand requested review from mbolivar, SebastianBoe, hakonfam, carlescufi and rlubos November 15, 2019 09:23

tejlmand requested review from anangl, ioannisg and lemrey as code owners November 15, 2019 09:24

tejlmand force-pushed the multi_image branch from 942d1fe to adba242 Compare November 15, 2019 09:39

SebastianBoe force-pushed the multi_image branch from d6e89ae to 79576f8 Compare November 18, 2019 15:26

tejlmand commented Nov 19, 2019

View reviewed changes

cmake/multi_image.cmake Outdated Show resolved Hide resolved

tejlmand force-pushed the multi_image branch from 79576f8 to abcd43f Compare November 19, 2019 09:09

tejlmand force-pushed the multi_image branch 3 times, most recently from cda776e to 55f75e5 Compare November 19, 2019 14:30

mbolivar mentioned this pull request Nov 19, 2019

cmake: scripts: generate zip files for app and mcuboot updates nrfconnect/sdk-mcuboot#68

Closed

SebastianBoe force-pushed the multi_image branch from 63df801 to 38dc27e Compare December 2, 2019 17:45

ru-fu reviewed Dec 3, 2019

View reviewed changes

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

SebastianBoe reviewed Dec 3, 2019

View reviewed changes

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

SebastianBoe force-pushed the multi_image branch 4 times, most recently from 90940b5 to 049ef9f Compare December 5, 2019 11:17

ru-fu approved these changes Dec 5, 2019

View reviewed changes

SebastianBoe force-pushed the multi_image branch 2 times, most recently from 73ac24a to c027dc4 Compare December 6, 2019 11:28

hakonfam approved these changes Dec 6, 2019

View reviewed changes

thst-nordic force-pushed the multi_image branch from c027dc4 to 463c424 Compare December 9, 2019 11:16

SebastianBoe force-pushed the multi_image branch 2 times, most recently from ab390ef to 48b08e2 Compare December 10, 2019 13:52

SebastianBoe approved these changes Dec 11, 2019

View reviewed changes

tejlmand commented Dec 11, 2019

View reviewed changes

tejlmand commented Dec 12, 2019

View reviewed changes

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

SebastianBoe force-pushed the multi_image branch 2 times, most recently from 999ec7d to 3b5c06b Compare December 12, 2019 13:59

SebastianBoe force-pushed the multi_image branch from 3b5c06b to 9588c77 Compare December 12, 2019 16:21

SebastianBoe merged commit cb0e63f into nrfconnect:master Dec 12, 2019

ru-fu reviewed Dec 13, 2019

View reviewed changes

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

doc/nrf/ug_multi_image.rst Show resolved Hide resolved

tejlmand mentioned this pull request Dec 16, 2019

multi image: support built-in CMake flags instead of proprietary #1650

Merged

nvlsianpu reviewed Dec 19, 2019

View reviewed changes

Multi image #1508

Multi image #1508

Conversation

tejlmand commented Nov 15, 2019 • edited by SebastianBoe Loading

NordicBuilder commented Nov 15, 2019 • edited Loading

tejlmand commented Nov 18, 2019

SebastianBoe commented Nov 18, 2019 • edited Loading

tejlmand commented Nov 18, 2019

SebastianBoe commented Nov 18, 2019 • edited Loading

mbolivar commented Nov 18, 2019

tejlmand commented Nov 19, 2019

carlescufi commented Nov 19, 2019

tejlmand commented Nov 19, 2019

mbolivar commented Nov 19, 2019

ru-fu left a comment

Choose a reason for hiding this comment

SebastianBoe commented Dec 5, 2019

ru-fu left a comment

Choose a reason for hiding this comment

SebastianBoe commented Dec 6, 2019

hakonfam left a comment

Choose a reason for hiding this comment

tejlmand left a 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

tejlmand Dec 11, 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

tejlmand Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ru-fu left a comment

Choose a reason for hiding this comment

sigvartmh commented Dec 13, 2019

SebastianBoe commented Dec 13, 2019

sigvartmh commented Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand commented Nov 15, 2019 •

edited by SebastianBoe

Loading

NordicBuilder commented Nov 15, 2019 •

edited

Loading

SebastianBoe commented Nov 18, 2019 •

edited

Loading

SebastianBoe commented Nov 18, 2019 •

edited

Loading

tejlmand Dec 11, 2019 •

edited

Loading

tejlmand Dec 16, 2019 •

edited

Loading

sigvartmh commented Dec 13, 2019 •

edited

Loading

SebastianBoe Dec 19, 2019 •

edited

Loading