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: tests: mcuboot: Add MCUBoot tests and Twister upgrade to support the tests workflow #36729

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

@PerMac
Copy link
Member

@PerMac PerMac commented Jul 5, 2021

Add the possibility to run tests with multiple images and stages. Such workflow is required for mcuboot tests, where the bootloader and two other images has to be build, signed and flashed consecutively.
Add twister tests for mcuboot using new multi-image and stages features. The tests are based on workflow from run_tests.sh and Makefile at https://github.com/mcu-tools/mcuboot/tree/main/samples/zephyr
Content of "tests/subsys/mcuboot/hello-world" is copied from the above link. Only imgtool_conf.yaml is new there and is used to allow for configuration of signing based on the tested platform.

@nvlsianpu
Copy link
Collaborator

@nvlsianpu nvlsianpu commented Jul 6, 2021

@d3zd3z This is outstanding patch I mentioned. It aim to allow to test mcuboot on-boards.

Worth to emphasize that this patch objective is no to introduce batch build (of multiple dependent images) extension for zephyr but allow to test complex scenarios on boards.

Loading

@PerMac PerMac changed the title twister: tests: mcuboot twister: tests: mcuboot: Add MCUBoot tests and Twister upgrade to support the tests workflow Jul 6, 2021
PerMac added 4 commits Jul 9, 2021
Enhance Twister with features allowing for building and running tests
with multiple images and stages. Most of the new features are defined
in stageslib.py. twisterlib.py required some refactoring as well:
- run_custom_script() was extracted from DeviceHandler so it can be
  used in a greater scope. Error handling was added to this method
- handle() from DeviceHandler now allows executing user-defined command
- process() from ProjectBuilder was modified to handle tests with
  multiple images being built and multiple stages executed in a single
  test.

Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
Add twister tests for mcuboot. The tests are based on workflow from
run_tests.sh and Makefile at
https://github.com/mcu-tools/mcuboot/tree/main/samples/zephyr

Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
By refactoring DeviceHandler.handle() the same device can be used again
when passed as hardware_fixed argument. A small piece of the code was
moved from handle() to a separate method get_and_lock_device() which
is called instead. In addition, at the end of handle(), the device is
not released if the method was called for given hardware. This is
needed for tests where multiple stages are chained and have to be
executed on the same device (e.g. mcuboot tests).

Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
If a test has multiple stages all of them has to be executed on
the same device. Device is locked before execution of stages
and released after all stages are executed, or when a test is
terminated with an error.

Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
@PerMac PerMac force-pushed the pr/mcuboot_tests branch from 09d9e28 to ff0f84a Jul 9, 2021
@@ -0,0 +1,256 @@
#!/usr/bin/env python3
Copy link
Collaborator

@hakehuang hakehuang Jul 10, 2021

Choose a reason for hiding this comment

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

maybe itis more complex to use a framework, I just see this
https://beam.apache.org/documentation/programming-guide/#configuring-pipeline-options
and this
https://pypyr.io/
which maybe a ready framework

Loading

@s-kelvin
Copy link
Collaborator

@s-kelvin s-kelvin commented Jul 13, 2021

I run the tests on reel_board. All 8 test cases failed with the build failure:

-- Generated device_extern.h: /home/geniussh/zephyrproject/zephyr/twister-out/reel_board/tests/subsys/mcuboot/ecdsa/mcuboot.ecdsa.bad/hello1/zephyr/include/generated/device_extern.h
Parsing /home/geniussh/zephyrproject/zephyr/Kconfig
/home/geniussh/zephyrproject/zephyr/scripts/kconfig/kconfig.py: twister-out/reel_board/tests/subsys/mcuboot/ecdsa/mcuboot.ecdsa.bad/hello1/Kconfig/Kconfig.modules:14: Could not open '/home/geniussh/zephyrproject/zephyr/' (in 'osource "$(ZEPHYR_LORAMAC_NODE_KCONFIG)"') (EISDIR: Is a directory)
CMake Error at /home/geniussh/zephyrproject/zephyr/cmake/kconfig.cmake:264 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /home/geniussh/zephyrproject/zephyr/cmake/app/boilerplate.cmake:555 (include)
  /home/geniussh/zephyrproject/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:24 (include)
  /home/geniussh/zephyrproject/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:35 (include_boilerplate)
  CMakeLists.txt:16 (find_package)


-- Configuring incomplete, errors occurred!
Error: could not find CMAKE_PROJECT_NAME in Cache

Loading

- "Failed reading image headers; Image=0"
- "Unable to find bootable image"
- OnTarget:
image: hello1
Copy link
Collaborator

@hakehuang hakehuang Jul 14, 2021

Choose a reason for hiding this comment

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

for this target we need a different flash address, for pyocd we need provide '-a 0x20000' , and for jlink we need provide a script file, and the information can be get from

boards/<arch>/<board>/<board>.dts. An example .dts
boot_partition: for MCUboot itself
image_0_primary_partition: the primary slot of Image 0
image_0_secondary_partition: the secondary slot of Image 0

so we need provide a scipt that can combine the three images into one. and then flash

Loading

Copy link
Member Author

@PerMac PerMac Jul 14, 2021

Choose a reason for hiding this comment

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

I think I don't understand fully your comment. I guess getting data from dts can be done, but after discussion with mcuboot developers, we think it is better to start with user-given data and later we can enhance this to use dts.
As for combining multiple images into one: this PR is for the opposite approach. MCUboot tests require separate consecutive flashes and checking the output after each of them to monitor if the image was accepted, rejected etc.

Loading

Copy link
Collaborator

@hakehuang hakehuang Jul 14, 2021

Choose a reason for hiding this comment

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

@PerMac , it is not about dts, it is about progra hello1 and hello2, for frdm_k64f I need add a offset by -a
you can find it at the bootloader\mcuboot\samples\zephyr line 172

flash_hello1:
	$(PYOCD) flash -a 0x20000 signed-hello1.bin

flash_hello2:
	$(PYOCD) flash -a 0x80000 signed-hello2.bin

Loading

Copy link
Collaborator

@d3zd3z d3zd3z Jul 16, 2021

Choose a reason for hiding this comment

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

If you program the hex files instead of the bin files, pyocd should get the address from the hex file itself. The only thing that is needed is to erase the entire flash before the bootloader is flashed, otherwise the initial behavior is unpredictable, as it depends on what was previously in the other slots.

My comment about waiting on the DTS is mainly about trying to divide the work up. Having something able to select among targets is better than the current test, which is just hard-coded for another target. Getting the information from the DTS is even better, but something that makes more sense as a future PR.

Loading

Add readme to help with the review

Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
@PerMac
Copy link
Member Author

@PerMac PerMac commented Jul 19, 2021

@s-kelvin Have you tried running other twister tests on reel_board? The error in your comment looks to me like something missing in your environment rather than an issue with this PR

Loading

@evgeniy-paltsev evgeniy-paltsev self-requested a review Jul 29, 2021
@PerMac
Copy link
Member Author

@PerMac PerMac commented Aug 23, 2021

I've heard that the main concern against this PR is that it tries to introduce multi-image building, which should better be handled at the build system level. However, I will try to explain the purpose of this PR from my perspective.
The main goal of this PR is to add tools allowing for performing more complex test scenarios in Twister. The current workflow is quite limited. It

  1. builds
  2. flashes
  3. check results

of a single image being flashed on a board and doesn't allow for any interaction with the board during the test. MCUboot tests require more steps within a single test. Three independent images are built and signed, each image is flashed consecutively on the same board, and then the board is reset. The output is verified after each step. This kind of test is currently not supported.
In my solution, I am using the already existing build system. Each image is built independently with its own arg, there is only a wrapper in Twister, that allows to switch between each build catalog and allows to have the workflow executed under a single test scenario.
IMO ability to perform more complex testing scenarios should be the part of the test runner, i.e. Twister. Like in MCUBoot tests, where a reset of a board is required and the behavior of the firmware is tested. My PR proposes tools for easy implementation of such tests with the introduction of test stages that a user can define in a testcase.yaml. The stages library can be expanded to allow for more interactions during a test. In particular, the plan is to allow scenarios like:

  1. build an image
  2. flash it
  3. verify the output
  4. reset the board
  5. verify the output

The point of the PR is to automatize steps that a user would have to perform manually during a more complex scenario testing. IMO having such orchestration under the build system instead of the test runner would obfuscate the workflow of such tests and would be harder for developers to implement. My idea was to provide a simple way of adding configurable test stages on a testcase.yaml level which are then interpreted and executed by twister.
@nashif @galak @mbolivar-nordic @carlescufi what are your thoughts on this topic? Who else should also be asked?

Loading

@trond-snekvik
Copy link
Contributor

@trond-snekvik trond-snekvik commented Aug 23, 2021

To add to @PerMac's arguments, I think this approach would allow us to replace the BabbleSim test shell scripts with a standardized Twister approach. This would make it much easier to add tests based on BabbleSim (especially for external repos, which cannot hook into the current BabbleSim test system at all), and we wouldn't have to run it as a separate stage in CI.

I made #33388 to make this happen a few months ago, but abandoned it in anticipation of this PR, which resolves most of the issues I encountered. Neither the build system, buildkite or the github actions provide sufficient functionality to implement this at the moment.

Loading

@d3zd3z
Copy link
Collaborator

@d3zd3z d3zd3z commented Aug 25, 2021

As an mcuboot maintainer, I would love to be able to see mcuboot able to be tested using twister. Our primary on target testing is still done by manually run scripts, since most test frameworks operate under the assumption that code will be loaded once, and run, and the output used to determine if it passed.

Loading

@mbolivar-nordic
Copy link
Member

@mbolivar-nordic mbolivar-nordic commented Aug 25, 2021

[...] @mbolivar-nordic [...] what are your thoughts on this topic? Who else should also be asked?

I think what you say sounds reasonable, but I haven't looked at the PR and I don't maintain twister itself.

Loading

@abrodkin abrodkin requested review from ruuddw and YuguoWH Aug 26, 2021
Copy link
Member

@nashif nashif left a comment

I still maintain that multi-image building should not be part of twister and should be handled and implemented as a separate feature that twister just calls and interacts with, similar to west build and west flash with its own input and configuration files. For example, this could be implemented as a west extension that twister just calls if a configuration file exists for multi images, few reasons:

  • twister is not a build tool, it is for testing something that was built using some other means.
  • This add very complex logic and very complex syntax to twister, I keep hearing that twister is way to complex already with its own options.
  • The main issue we will have, there will be no way for a developer to reproduce the built images in a sane way outside of twister. You will get a failure in CI and have no idea how to reproduce the same outside of twister. This is IMO a must, you should be able to build the same thing and produce the same binaries by just using west. Integrating the build logic into twister will make this impossible.
  • Providing a multi-image build facility is a nice to have as a standalone feature and outside twister.

Loading

@d3zd3z
Copy link
Collaborator

@d3zd3z d3zd3z commented Sep 13, 2021

@nashif:

The main issue we will have, there will be no way for a developer to reproduce the built images in a sane way outside of twister.

This is a pretty big deal to me. And, I say this after spending a bunch of time in MCUboot figuring out how to reproduce a travis failure on my local machine.

I like the idea of testing this, but I agree with Anas that most of the functionality should be somewhere else. One difficulty with this is that we've had issues with multiple builds and cmake before, and as I understand something is blocking this.

Loading

@PerMac
Copy link
Member Author

@PerMac PerMac commented Sep 20, 2021

I agree with you @nashif in most points, just with a caveat to

This adds very complex logic and very complex syntax to Twister, I keep hearing that twister is way too complex already with its own options.

Complexity itself is not necessarily a problem for software. The problems start when the code and functionality are hard to understand. To be honest it took me a while trying to find a way how to add new functionality within the existing workflow, mainly due to low modularity where one function does all the things. I tried to use good coding practices when writing stageslib.py, where most new functionality comes. I didn't want to modify the twisterlib.py code too much and tried to follow the existing structure only modifying crucial parts, like extracting a function to lock a single device. Nevertheless, I am open to suggestions on what to improve in my code.

I fully agree that we want the build to be easily reproducible without twister. That's why I follow exactly the same workflow as it was used in twister for building, only the build directory is changed between builds in the background. But I think I might figure out a way to make it even cleaner and easier to reproduce.

I understand that "multi-image" shouldn't be interwind with Twister. Luckily, in MCUboot tests, there are 3 consecutive builds that don't have to relate to each other. Would you @nashif then agree to some temporary middle-ground solution? I can try to make the building stage working in a way, that there will be 3 consecutive calls for "west build" with clearly defined arguments, and anyone would be able to rerun it independently from twister by just calling "west build" commands that will also be printed out in the verbose output? Later on, when the multi-image builder will exist in Zephyr, we would just replace the paths from where the images are flashed? I can also update the configuration, so it can be tested on reel_board.

If so, could you please check other parts of my proposed solution? In most cases, Twister is just used as a wrapper, invoking other scripts, like west sign but within a single workflow, so a scenario could be run without interferences.

I would like to know where are we standing now. I've heard that these kinds of bootloader tests can be worth having in Zephyr. I am open to all suggestions related to the code/workflow and how it can be improved. However, if my argumentation for the overall idea of more complex test scenarios doesn't work for you I will have to start working on another solution for it.

Loading

@PerMac
Copy link
Member Author

@PerMac PerMac commented Sep 20, 2021

The steps required for MCUBoot tests are listed in this specification PR mcu-tools/mcuboot#955. It was based on /mcuboot/samples/zephyr/makefile and /mcuboot/samples/zephyr/run-tests.sh. The descriptions of test cases were copied from these files.

Loading

@d3zd3z
Copy link
Collaborator

@d3zd3z d3zd3z commented Sep 27, 2021

Just a note that /mcuboot/samples/zephyr/run-tests.sh is out of date, and probably should just be removed. The sample tests are run using run-tests.go (and there is a build-tests.go as well). Not only does this actually check the results, but the shell version might be a bit out of date as far as what tests should be run.

Loading

@carlescufi
Copy link
Member

@carlescufi carlescufi commented Oct 18, 2021

@nashif @galak @JordanYates @d3zd3z @trond-snekvik

I would like to unblock this situation. We've had discussions around multi-image (i.e. multiple images built from Zephyr source) dozens of times before, and we always hit the same brick wall: "not generic enough".
Maybe it's time, as @galak has suggested a couple of times in the past, to at least address the biggest user of multi-image: MCUboot. This affects not only testing and Twister, but also things like partitions and linker sections and tests that require multiple peers
We can wait forever to come up with a generalized solution, or we can at least try to solve the bootloader/MCUboot problem.

Loading

@hakehuang
Copy link
Collaborator

@hakehuang hakehuang commented Oct 18, 2021

Move the stage build to west, so that twister only call west build --multi config_file
And can use ‘west export-build` to show the config files exported
And use ‘west flash --multi image-A@address,image-B@address’/config_file to do multi-image flash

Loading

@nashif
Copy link
Member

@nashif nashif commented Oct 19, 2021

I would like to unblock this situation. We've had discussions around multi-image (i.e. multiple images built from Zephyr source) dozens of times before, and we always hit the same brick wall: "not generic enough".

Where did anyone talk about this being not generic? The comment #36729 (review) still applies. I do not think this should go into twister and instead should be implemented outside as a standalone feature that twister can call like it now calls west build(?) or west flash.

Btw, one minute after this comment was posted, all stakeholders joined a meeting and spent 1 hour talking about this PR and how we should move forward with it, and I think we reached some level of agreement, what a coincidence.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Twister
To do
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants