twister: tests: mcuboot: Add MCUBoot tests and Twister upgrade to support the tests workflow #36729
Conversation
github-actions
bot
added
area: Documentation
area: Tests
PerMac
requested review from
nvlsianpu,
hakehuang,
d3zd3z,
de-nordic,
galak and
wangnuannuan
PerMac
force-pushed
the
pr/mcuboot_tests
branch
2 times, most recently
from
b20bca4
to
43da63e
Compare
|
@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. |
PerMac
changed the title
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>
| @@ -0,0 +1,256 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
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
|
I run the tests on reel_board. All 8 test cases failed with the build failure: |
| - "Failed reading image headers; Image=0" | ||
| - "Unable to find bootable image" | ||
| - OnTarget: | ||
| image: hello1 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
Add readme to help with the review Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
|
@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 |
|
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.
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.
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. |
|
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. |
|
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. |
I think what you say sounds reasonable, but I haven't looked at the PR and I don't maintain twister itself. |
There was a problem hiding this 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.
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. |
|
I agree with you @nashif in most points, just with a caveat to
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 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. |
|
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. |
|
Just a note that |
|
@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". |
|
Move the stage build to west, so that twister only call |
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 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. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
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.