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.

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

twister: add sysbuild support #49552

Merged
merged 3 commits into from
Oct 4, 2022
Merged

twister: add sysbuild support #49552

merged 3 commits into from
Oct 4, 2022

Conversation

danieldegrasse
Copy link
Collaborator

@danieldegrasse danieldegrasse commented Aug 25, 2022

This PR adds support for sysbuild to twister. This will enable test cases that require multiple images, such as verifying MCUBoot support, or running tests on the second core of some dual core systems. The changes are designed to be as unintrusive as possible, so sysbuild can be used with existing projects with no impact.

Twister can be instructed to use sysbuild via the sysbuild YAML tag, and will then build the project with sysbuild. By default, no external sysbuild projects will be compiled in, but they can be enabled via the extra_args YAML tag for tests.

Enabling sysbuild will not affect twister's filtering options, and when multiple images are included into the build sysbuild will parse Kconfig and devicetree values from the default sysbuild image (the main application being build, that is not added via ExternalZephyrProject_Add()). This PR does not enable twister to filter based on configuration settings in the external sysbuild images.

This PR also adds an MCUBoot test, which will verify that twister sysbuild support is working correctly, as well as testing MCUBoot support. This test is currently enabled for the mimxrt1060_evk and frdm_k64f

@danieldegrasse danieldegrasse requested a review from nashif as a code owner August 25, 2022 21:50
@danieldegrasse
Copy link
Collaborator Author

@tejlmand FYI

Copy link
Collaborator

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

great to see this 🎉

Couple of questions before +1.

Comment on lines +330 to +336
# We must parse the domains.yaml file to determine the
# default sysbuild application
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really need to do that ?
The self.source_dir should be the default sysbuild application so any special reason that cannot be used here or that it is undesired to use it ?

I can envision future complex scenarios where we want to use sysbuild for multiple samples where default makes less sense, but I don't beleive that's the case here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do I believe, because twister does not parse the source application, it parses the generated .config and edt.pickle files in the build folder for an application. This leaves us with the following options:

  • Don't parse domains.yaml, and simply search the root of the build folder: This will result in searching the "meta" sysbuild application, that includes all other applications in the build as external projects. This will work, but we won't be able to do useful Kconfig or devicetree filtering
  • Parse the domains.yaml and select the default application's build directory: This will enable Kconfig and devicetree filtering on the primary application only. It's the solution I have enabled currently due to the low complexity and impact
  • Parse the domains.yaml and generate filter data for all applications: This is probably the best solution long term, but I avoided it in this PR because I wasn't certain how best to handle instructing twister which domain you wanted a filter to apply to

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do I believe, because twister does not parse the source application, it parses the generated .config and edt.pickle files in the build folder for an application. This leaves us with the following options:

Thanks for the explanation.
The path you have chosen makes most sense at this stage.

scripts/twister Outdated
Comment on lines 48 to 49
If true, build the sample using the sysbuild infrastructure. This
will disable filtering based on Kconfig or devicetree information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is just temporary in order to limit the scope, then fine, but long term we should be able to still allow Kconfig or dt filtering and just use the .config or dt from the main sample build folder instead.

But I can accept the limitation for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to update this, thank you for pointing it out. KConfig and devicetree filtering are supported, just only on the main systembuild application

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been updated to reflect the actual support present in this PR

@zephyrbot zephyrbot added platform: ESP32 Espressif ESP32 area: RISCV RISCV Architecture (32-bit & 64-bit) labels Aug 26, 2022
# Workaround for not being able to have commas in macro arguments
DT_CHOSEN_Z_CODE_PARTITION := zephyr,code-partition

config BUILD_OUTPUT_ADJUST_LMA
Copy link
Collaborator

@nordicjm nordicjm Sep 20, 2022

Choose a reason for hiding this comment

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

This does not work for devices where the secondary slot is not part of the primary flash, e.g. in an external flash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So such boards shouldn't be used as build targets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems like a big limitation. Would it not be better to have a Kconfig option to generate for the secondary slot instead? The list of devices might not be large at present but it is going to only grow with time (devices where slot1 and slot0 differ):

arm/usb_kw24d512/usb_kw24d512.dts
arm/rm1xx_dvk/rm1xx_dvk.dts
arm/pinnacle_100_dvk/pinnacle_100_dvk.dts
arm/lpcxpresso55s36/lpcxpresso55s36.dts
arm/lpcxpresso55s28/lpcxpresso55s28.dts
arm/lpcxpresso55s16/lpcxpresso55s16_common.dtsi
arm/lpcxpresso55s06/lpcxpresso55s06_common.dtsi
arm/lpcxpresso54114/lpcxpresso54114_m4.dts
arm/frdm_kw41z/frdm_kw41z.dts
arm/disco_l475_iot1/disco_l475_iot1.dts
arm/bt610/bt610.dts
arm/bl5340_dvk/bl5340_dvk_cpuapp_common.dts

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the second slot being on an external device isn't tested much because dev boards usually don't have external flash. But, it does seem common in production. I think it is an important case to consider.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the second slot being on an external device isn't tested much because dev boards usually don't have external flash. But, it does seem common in production. I think it is an important case to consider.

I had a quick grep through the boards and it seems 63 boards have QSPI entries in their boards files, 63/500 is 13% of all supported boards, which isn't a small number.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, "isn't tested very much" by things like our testing infrastructure. I'm sure there are lots of targets that do this, and that it is pretty common in production.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not work for devices where the secondary slot is not part of the primary flash, e.g. in an external flash.

What would the required steps here be in order to generate an image for the secondary flash? The image needs to be build targeting the primary slot, then the programming tool (pyocd, jlink,...) needs to be instructed to load the image at a given offset, correct? Or if the image cannot be loaded using jlink or another west runner, what would be the method to flash it?

Perhaps the work in #50393 could be leveraged to pass a load address to the runner?

Copy link
Collaborator

@nordicjm nordicjm Sep 27, 2022

Choose a reason for hiding this comment

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

For nordic parts, if the secondary slot it in QSPI then it is memory mapped, 0x12000000 for nRF52840 and 0x10000000 for nRF5340, this can be programmed directly to the QSPI flash using nrfjprog, I would assume other flash runners would not support programming the QSPI. There is no offset given to nrfjprog with e.g. a bin file, it needs a properly addressed hex file. For SPI flash on other nordic parts, e.g. nRF52832, they cannot be flashed from nrfjprog, flashing of such a flash would be down to whomever creates the board (it would need to be a custom flash runner)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the slot is memory mapped, and nrfjprog respects the offsets in an Intel HEX file, then I believe this solution should work. It might be necessary to use a different expression here for Nordic parts with QSPI, but the core of the approach (shifting the LMA of the image, and then having that offset propagate to the HEX file) should still work provided my assumption about nrfjprog is true. Does this seem correct, or am I misunderstanding the issue with supporting Nordic's parts using this sample?

@nashif
Copy link
Member

nashif commented Sep 24, 2022

What's more, I think twister should remove the --erase from west parameters if a user added them in the CLI (e.g. using --west-flash=--erase), because using --erase screws mcuboot. @nashif @mbolivar-nordic What do you think about such approach?

I do not think it is a good idea for twister to remove anything that was added by the caller on the command line. If this option is not supported, twister needs to error out. Removing options silently will just cause problems and caller will be left wondering what happened...

@nordicjm
Copy link
Collaborator

The erase/recover part needs to be fixed or blocked entirely, it's not acceptable that doing a twister erase or west flash --recovery programs 3 different files and recovers the module prior to each leaving it useless #50421 there shouldn't need to be different flashing commands for if you are using sysbuild vs not using sysbuild to do the same thing, adding sysbuild should be transparent to the user in this regard.

@danieldegrasse
Copy link
Collaborator Author

@nashif I agree that silently ignoring command line parameters isn't the best way to handle this issue, but the alternative seems to be breaking @PerMac's nightly testing flow. @PerMac, you mentioned that there are only some subset of tests (I know you called out TFM specifically) that require an erase. Maybe the best option here is to add the ability to encode a requirement to erase the flash before flashing a sample into twister?

@PerMac
Copy link
Member

PerMac commented Sep 29, 2022

@nashif @danieldegrasse I guess adding a feature where a test could order an erase as a part of the setup would be nice. I agree the most transparent would be producing an error when sysbuild test is called with not compatible CLI arguments. I don't want to sound like "it dosn't work with my way of running twister, so I will block it", just wanted to show, that sysbuild tests require extra "guarding logic", since they require a specific workflow and some indirect and harder to follow errors can pop out. As the easiest solution for now I see adding a "sysbuild" tag to sysbuild tests/samples. I (and ofc others) could then split the workflow calling twister twice, with --exclude-tag`--tag` args

@danieldegrasse
Copy link
Collaborator Author

I (and ofc others) could then split the workflow calling twister twice, with --exclude-tag--tag args

@PerMac Ok, if you can still use this implementation within your tests by running sysbuild tests separately, I will update this PR so that twister will skip the test if called without options.west_flash set to true, or with --erase passed to the west options. This way existing test runs will not break, and users can enable sysbuild tests by making sure their twister runs satisfy these flags

Add support for building with sysbuild using twister, via the "sysbuild"
yaml property in testsuites. This will currently disable Kconfig and
devicetree filtering.

Signed-off-by: Daniel DeGrasse <[email protected]>
Add sysbuild flag to twister supported options, with documentation on
how Kconfig and devicetree will be parsed

Signed-off-by: Daniel DeGrasse <[email protected]>
Add test to verify mcuboot support. This test is only enabled for specific
platforms, since it it not possible to filter for mcuboot support.
Sysbuild support is required to flash multiple samples using twister.

Signed-off-by: Daniel DeGrasse <[email protected]>
@zephyrbot zephyrbot requested a review from chen-png September 29, 2022 22:04
@danieldegrasse
Copy link
Collaborator Author

@PerMac @mniestroj I've updated this PR based on your feedback. The current state is as follows:

  • Sysbuild support parses the KConfig and DTS of the "main" application being built with sysbuild for filtering purposes
  • platform_allow is still in use- see the discussion above for the future solution using PR cmake: package_helper CMake script #41302
  • When twister is run with --west-flash=--erase, or without --west-flash (both unsupported settings) then tests requiring sysbuild support will be skipped, and a warning will be printed (@nordicjm from one of your prior comments, it appears that --west-flash=--recover may also be an unsupported flag?)
  • Documentation has been updated to indicate the above behavior when a test requires sysbuild

@PerMac
Copy link
Member

PerMac commented Oct 4, 2022

Thanks for the changes, works as described

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.