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

Jump to bottom

Fix s1 13188 #6537

Closed
wants to merge 6 commits into from
Closed

Fix s1 13188 #6537

wants to merge 6 commits into from

Conversation

hakonfam
Copy link
Contributor

Only here for the test result, will be merged in the upmerge

@hakonfam hakonfam added the DNM label Jan 13, 2022
@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. CI-boot-dfu-test doc-required PR must not be merged without tech writer approval. manifest CI-all-test Run All integration tests manifest-zephyr labels Jan 13, 2022
@github-actions
Copy link

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@f295fb7 nrfconnect/sdk-zephyr#690 nrfconnect/sdk-zephyr#690/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Jan 13, 2022
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@github-actions github-actions bot removed the CI-all-test Run All integration tests label Jan 13, 2022
@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Jan 19, 2022
@hakonfam hakonfam requested a review from tejlmand January 19, 2022 13:43
@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Jan 19, 2022
tejlmand
tejlmand reviewed Jan 19, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Much better 👍

Some small observations.

cmake/multi_image.cmake Show resolved Hide resolved
cmake/multi_image.cmake Outdated Show resolved Hide resolved
cmake/multi_image.cmake Outdated Show resolved Hide resolved
cmake/multi_image.cmake Outdated Show resolved Hide resolved
cmake/multi_image.cmake Outdated Show resolved Hide resolved
samples/Kconfig
Comment on lines +94 to +95
config NCS_IS_VARIANT_IMAGE
bool "Image is a variant build of another image [READ ONLY]"
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the intention is to have this information available in source code.

I assume the number of cases where source code itself need to be aware of this are very few.
So maybe we should handle those case specifically.

Not so fond of user-accessible setting denoted READ ONLY.
Alternative, we can look into the possibility of making this promptless, and handled differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a better mechanism than Kconfig.

Adjusting how the option is declared might make sense though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how we can modify the option if we make it promptless. Wouldn't we have to add some kconfig source code to the build?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a better mechanism than Kconfig.

Simply trying to understand exactly the intention of this.
If going to be used in code or similar, Kconfig is best way, if this is purely a build system to build system knowledge, then pure CMake variable could be an option.

I see a risk in having a very generic symbol with a prompt for the following reasons:

Now, making a variant is a generic term, and it's true that for now every time we build a variant it will be for slot 1, so in that case it could be fine, but we could also consider if it would be better to have a specific setting to indicate what setting the variant adjusts.

If a variant build in future needs the slot, but also other settings in addition, then all those could be selected by a higher level symbol. I believe that will make it easier to understand what changes a variant build imposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

an example of how we may provide variant specific Kconfig options, not generally available can be seen here:
hakonfam#12

@github-actions github-actions bot removed the CI-all-test Run All integration tests label Jan 19, 2022
@github-actions github-actions bot added the CI-all-test Run All integration tests label Jan 19, 2022
@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Jan 20, 2022
@hakonfam
tests: fw_info: remove 53ns board
cd3ae02 
This test builds s1 variants for secure boot, that is not supported
for non-secure applications without MCUboot enabled.

Ref: NCSDK-13188
Signed-off-by: Håkon Øye Amundsen <[email protected]>
@hakonfam
cmake: multi_image: use preload file instead of CLI arguments
238fd9f 
Handling lists within lists in cmake is prone to error.
Replace the current mechanism of passing child image parameters
as '-D' style arguments with a generated preload file.

Ref: NCSDK-13188
Signed-off-by: Håkon Øye Amundsen <[email protected]>
@hakonfam
cmake: multi_image: generate preload file for root image ('app')
a34c552 
This can be used when creating variant images of the root image.

Ref: NCSDK-13188
Signed-off-by: Håkon Øye Amundsen <[email protected]>
@hakonfam
cmake: multi_image: add argument for passing preload image
16b7d41 
A user can now pass an image

Signed-off-by: Håkon Øye Amundsen <[email protected]>
@hakonfam
cmake: use add_child_image with preload instead of s1.cmake
a7d640c 
Leverage multi image mechanism for building variant image instead of
creating a new linker script and re-linking the base image.

Ref: NCSDK-13188

Signed-off-by: Håkon Øye Amundsen <[email protected]>
@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Jan 20, 2022
@hakonfam
multi_image: partition_manager: fix bug in bootlader/pm.yml
38759bc 
MCUboot child image kconfig options are not available when preprocesing
this file. Hence, checking if CONFIG_PARTITION_SIZE_MCUBOOT_PAD = 0
will always be true.

Add a Kconfig option which is used to pass information to non-mcuboot
child images that mcuboot is included in the build and use this
instead.

This places s1_image to an incorrect location both when mcuboot is
included and not.

Ref: NCSDK-13188

Signed-off-by: Håkon Øye Amundsen <[email protected]>
@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Jan 20, 2022
@SebastianBoe
Copy link
Contributor

This has been pushed to the upmerge branch

It is still open for review and review fixes, but in the upmerge branch, not here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristopherBittner ChristopherBittner ChristopherBittner left review comments

@SebastianBoe SebastianBoe Awaiting requested review from SebastianBoe

@tejlmand tejlmand Awaiting requested review from tejlmand

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. CI-all-test Run All integration tests DNM manifest manifest-zephyr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hakonfam @NordicBuilder @SebastianBoe @tejlmand @ChristopherBittner