Skip to content

Add on-target tests to CI #355

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

Merged
merged 1 commit into from
Apr 4, 2022
Merged

Add on-target tests to CI #355

merged 1 commit into from
Apr 4, 2022

Conversation

newAM
Copy link
Member

@newAM newAM commented Oct 9, 2021

A week ago in the rust-embedded matrix chat @adamgreig mentioned that on-target CI would be helpful for the cortex-m and cortex-m-rt crates. This is a pull-request to make that happen.

History: Bootstrapping and cortex-m-rt

The first problem I came across was the amount of boilerplate required to build an on-target binary without cortex-m-rt.

There are two paths forward here:

  1. Introduce a lot of boilerplate in cortex-m, which will largely be copy-pasted from cortex-m-rt.
  2. Relocate cortex-m-rt to this workspace.

In (#391) cortex-m-rt was relocated to this workspace to fix the bootstrapping problem.

Test Execution

Tests run with QEMU and physical hardware.

QEMU uses the LM3S6965 Cortex-M3 target because it is widely used.

The physical hardware is a NUCLEO-F070RB connected to a self-hosted runner. In the future more hardware can be added to cover more CPUs.

Due to reliability concerns with self-hosted runners only QEMU will be a required status check in CI, the physical hardware checks will be informational only.

CI Software

The CI software for running on physical hardware is simply a self-hosted github actions runner. A working demonstration of this can be found here (the repository does not appear as a fork to work around github actions limitations with forks).

The runner uses probe-run to execute the tests on embedded hardware. probe-run was chosen for several reasons:

  • Actively maintained by knurling-rs, with an actively maintained back-end from probe-rs.
  • Written in rust. Understanding the code does not require contributors to learn a new language.
  • Designed with on-target testing as a primary goal (for use with defmt-test).
  • Automatic unwinding and backtrace display.

Test Harness

This PR introduces a test harness, minitest.

minitest is almost identical to defmt-test, the only difference is that it replaces defmt with rtt-target because defmt introduces a dependency cycle on cortex-m.

This is harness is very minimal, adding only 327 lines of rust:

$ tokei testsuite/minitest 
===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Markdown                1            7            0            4            3
 TOML                    2           41           34            0            7
-------------------------------------------------------------------------------
 Rust                    3          406          350            5           51
 |- Markdown             1            8            0            7            1
 (Total)                            414          350           12           52
===============================================================================
 Total                   6          454          384            9           61
===============================================================================

The test harness does introduce some abstraction, and may not be suitable for all tests. Lower-level tests are still possible without the harness using asm::udf to fail the test, and asm::bkpt to exit without failure.

Reliability and Uptime

I have been doing automatic on-target testing for the stm32wlxx-hal using probe-run. Over hundreds of automatic runs spanning several months I have had no failures as a result of external factors (USB connectivity, programming errors, ect.). I do not anticipate on-target CI being perfect, but at the same time I do not anticipate frequent problems.

@newAM newAM requested a review from a team as a code owner October 9, 2021 19:09
@newAM newAM marked this pull request as draft October 9, 2021 19:09
@adamgreig
Copy link
Member

This is a great idea and thanks for putting the draft together!

  • I think moving cortex-m-rt into this repository is probably the best way around the bootstrapping issues and it's why we already moved cortex-m-semihosting and panic-semihosting into this repository. I don't foresee any particular issues there.
  • Having a second target for thumbv7em-none-eabihf, like the F2 or H7, would allow testing more of the cortex-m features. If we had only one target probably a thumb7 would be the obvious choice as we could run v6 binaries on it too. That said, I don't think it's super urgent: many tests can just target thumbv6 and thus work on almost any Cortex-M hardware people might have lying around.
  • The memory.x file could live inside the testsuite/ directory and use a build.rs script to copy it into OUT_DIR, rather than cluttering the top-level directory
  • One mitigation for CI security or potential for abuse is to gate the tests on pushes to staging or trying, which only members and bors can push to, so the checks would only run after bors try or bors r+. The downside is that failures wouldn't be reported right away to contributors though, so we could also consider this as a fallback plan if the current setup gets abused.
  • Personally I'm not too worried about the bus factor or QoS risks; if necessary we can always remove the check from the bors requirements or anyone can run the tests manually locally. I think the benefits would massively outweigh the risks.
  • The added complexity for adding new features is probably something that should already exist, but we can review how difficult it is to write new tests once we have some in place.

My vote would be to plan to go ahead with this, but first merge cortex-m-rt into this repository; any thoughts @rust-embedded/cortex-m ?

@thejpster
Copy link
Contributor

Out of interest, what cortex-m features can be tested on real hardware, that can't be tested on virtual hardware (e.g. qemu)?

@newAM
Copy link
Member Author

newAM commented Oct 25, 2021

Out of interest, what cortex-m features can be tested on real hardware, that can't be tested on virtual hardware (e.g. qemu)?

There are RTL accurate simulators that can test everything except analog behavior, but they are not practical here because of licensing restrictions for ARM IP, and because they are insanely slow.

QEMU is a wonderful tool, most simulators are written by hardware designers for their use-case, QEMU is written by software developers for their use-case. As a result of this QEMU skips a few steps.

  1. Cycle accuracy. QEMU is a transaction level model. This is a good design choice for speeding up execution, but it does mean timing-sensitive code bugs can be missed with QEMU.
  2. Everything is modeled independently of the RTL, and as a result some features go unimplemented, or worse, incorrectly implemented. I went digging through the QEMU source, but unfortunately the armv7 CPU MMIO does not use the standard create_unimplemented_device functions, so it is difficult to determine what is not implemented. At the very least is looks like the DWT is unimplemented:

https://github.com/qemu/qemu/blob/c5b2f559814104f4145f8bc310f4d33c7ead8f49/hw/arm/armv7m.c#L363-L366

It is also worth noting that QEMU does not solve the "works on my machine problem"; QMEU is not deterministic, same as on-target testing. Zephyr (RTOS) uses QEMU extensively, and they have to stay on-top of this problem: zephyrproject-rtos/zephyr#12553

I have used a lot of different of pre-silicon simulation at work; typically when silicon is available we focus our testing efforts on-target because it is the best representation available. That said, QEMU can still be a good choice because it avoids bus factor / QOS. Neither option is a bad choice, I can make another draft of this PR using QEMU if desired.

@thejpster
Copy link
Contributor

I appreciate the comprehensive response - thanks!

I think I would like to see QEMU support as well, simply because it scales better and doesn't involve physical infrastructure that is voluntarily hosted. But I totally buy your arguments that on-device testing adds something beyond what QEMU can do and is hence highly desirable.

Maybe we can talk to someone at Arm about a Fast Model licence...

@newAM
Copy link
Member Author

newAM commented Oct 25, 2021

I think I would like to see QEMU support as well, simply because it scales better and doesn't involve physical infrastructure that is voluntarily hosted. But I totally buy your arguments that on-device testing adds something beyond what QEMU can do and is hence highly desirable.

QEMU support is definitely doable. To clarify, is your desire to have QEMU be the CI target with the option to run on-target locally or the other way around?

Maybe we can talk to someone at Arm about a Fast Model licence...

I would like whatever testing method we choose to be easily accessible for outside contributors. The ARM fast model would be a better fit for this usecase than QEMU, but I am concerned that it would introduce too much of a barrier for contributions.

@newAM
Copy link
Member Author

newAM commented Oct 30, 2021

This was discussed in the weekly meeting: https://github.com/rust-embedded/wg/blob/master/minutes/2021-10-26.md

There is some hesitation to add on-target testing without an SLA, which is completely understandable. Based on that feedback I updated the draft PR:

  • Changed CI to use QEMU instead of the self-hosted runner
  • Added compile-time features to select RTT or semihosting for logging
    • Allows for the flexibility to easily run on-target or run with QEMU
  • Moved memory.x to the testsuite directory per @adamgreig 's feedback

The tests can still be run on physical hardware. There is still a discussion to be resolved as to what approach to take with regards to gating/non-gating and automatic/manual.

@thejpster noted that the Cortex-M55 FPV is free for use (no licensing issues). I plan to explore that option this weekend as well.

bors bot added a commit that referenced this pull request Jan 20, 2022
391: Merge cortex-m-rt into this repository r=therealprof a=adamgreig

This PR merges the cortex-m-rt repository (with history) into this repo, inside the `cortex-m-rt` folder which is added to the workspace. The main advantage is easier combined testing of cortex-m with cortex-m-rt (including on-hardware tests e.g. #355), and in the future easier changes across the two projects.

The MSRV of cortex-m-rt is bumped 1.39 -> 1.40 to align it with cortex-m itself.

I've updated the CI to run the same tests and checks as before, and updated references to the old URL.

If/after this is merged, I propose adding a note to the old repo's README and then archiving it.

An alternative to this technique would be adding all the files in one new commit (not preserving history), if anyone thinks that would be neater.

NB: This PR also adds an inline to ITM to fix a clippy hard error.

For future reference, the git work was:

```
cd cortex-m-rt
git filter-repo --to-subdirectory-filter cortex-m-rt
cd ../cortex-m
git remote add rt ../cortex-m-rt
git fetch rt
git merge --allow-unrelated-histories rt/master
```

Co-authored-by: bors[bot] <bors[bot]@users.noreply.github.com>
Co-authored-by: Raz Aloni <[email protected]>
Co-authored-by: Vadim Kaushan <[email protected]>
Co-authored-by: Jonathan 'theJPster' Pallant <[email protected]>
Co-authored-by: Adam Greig <[email protected]>
Co-authored-by: Jonas Schievink <[email protected]>
Co-authored-by: Jorge Aparicio <[email protected]>
Co-authored-by: Emil Fresk <[email protected]>
Co-authored-by: Daniel Egger <[email protected]>
Co-authored-by: Niklas Claesson <[email protected]>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
bors bot added a commit that referenced this pull request Jan 23, 2022
391: Merge cortex-m-rt into this repository r=thejpster a=adamgreig

This PR merges the cortex-m-rt repository (with history) into this repo, inside the `cortex-m-rt` folder which is added to the workspace. The main advantage is easier combined testing of cortex-m with cortex-m-rt (including on-hardware tests e.g. #355), and in the future easier changes across the two projects.

The MSRV of cortex-m-rt is bumped 1.39 -> 1.40 to align it with cortex-m itself.

I've updated the CI to run the same tests and checks as before, and updated references to the old URL.

If/after this is merged, I propose adding a note to the old repo's README and then archiving it.

An alternative to this technique would be adding all the files in one new commit (not preserving history), if anyone thinks that would be neater.

NB: This PR also adds an inline to ITM to fix a clippy hard error.

For future reference, the git work was:

```
cd cortex-m-rt
git filter-repo --to-subdirectory-filter cortex-m-rt
cd ../cortex-m
git remote add rt ../cortex-m-rt
git fetch rt
git merge --allow-unrelated-histories rt/master
```

Co-authored-by: bors[bot] <bors[bot]@users.noreply.github.com>
Co-authored-by: Jonathan 'theJPster' Pallant <[email protected]>
Co-authored-by: Adam Greig <[email protected]>
Co-authored-by: Jonas Schievink <[email protected]>
Co-authored-by: Jorge Aparicio <[email protected]>
Co-authored-by: Emil Fresk <[email protected]>
Co-authored-by: Daniel Egger <[email protected]>
Co-authored-by: Niklas Claesson <[email protected]>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
Co-authored-by: Vadim Kaushan <[email protected]>
@newAM newAM force-pushed the master branch 4 times, most recently from ba8945f to c90228d Compare January 23, 2022 20:42
@newAM
Copy link
Member Author

newAM commented Jan 23, 2022

I updated this pull-request for #391 which solves the bootstrapping problem; CI is passing now! 🎉

I also removed the demonstration do-nothing tests and added a couple basic tests.

@adamgreig
Copy link
Member

adamgreig commented Jan 23, 2022

This is looking great, thanks! Perhaps it would be worth adding the new CI flow to bors' required statuses?

I'd still be interested in having it run automatically on hardware as a best-effort thing: perhaps qemu on GitHub's own CI and required for bors, and also (tries to) runs a job on your hardware, but without being required for bors so it wouldn't block merging if it wasn't working?

Edit: To be clear, no need for on-hardware testing as part of this PR or anything, we could go ahead with just qemu if you like. Just making it clear I still think there's a lot of value in automated on-hardware testing too, in addition to it being easy to run on-hardware tests manually from the repo.

Is there anything else before this is "ready for review"?

@newAM
Copy link
Member Author

newAM commented Jan 23, 2022

This is looking great, thanks! Perhaps it would be worth adding the new CI flow to bors' required statuses?

Missed that, fixed, thanks!

I'd still be interested in having it run automatically on hardware as a best-effort thing: perhaps qemu on GitHub's own CI and required for bors, and also (tries to) runs a job on your hardware, but without being required for bors so it wouldn't block merging if it wasn't working?

Edit: To be clear, no need for on-hardware testing as part of this PR or anything, we could go ahead with just qemu if you like. Just making it clear I still think there's a lot of value in automated on-hardware testing too, in addition to it being easy to run on-hardware tests manually from the repo.

I can do that, everything is setup because I run on-target testing for the stm32wlxx-hal anyway.

@thejpster expressed some SLA concerns before - what do you think about this approach instead?

Is there anything else before this is "ready for review"?

Just cleaning up the introduction message so that the now-irrelevant details do not get committed by bors 😄

@newAM
Copy link
Member Author

newAM commented Feb 24, 2022

@thejpster any concerns with the above - adding on-target testing with real hardware to CI (in addition to QEMU), but without the real hardware CI runs being a gating check to mitigate SLA issues?

thejpster
thejpster previously approved these changes Feb 24, 2022
Copy link
Contributor

@thejpster thejpster left a comment

Choose a reason for hiding this comment

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

Looks great. Love the really thorough documentation.

@thejpster
Copy link
Contributor

Apologies it took so long to get to this, too.

@newAM newAM changed the title [RFC] Add on-target tests to CI Add on-target tests to CI Feb 26, 2022
@newAM newAM marked this pull request as ready for review February 26, 2022 17:20
@newAM
Copy link
Member Author

newAM commented Feb 26, 2022

Apologies it took so long to get to this, too.

No problem!

Is there anything else before this is "ready for review"?

Just cleaning up the introduction message so that the now-irrelevant details do not get committed by bors smile

I cleaned up the PR body to reflect the current plan, re-added the HIL tests, and marked this as ready for review 👍

Before this gets merged the self-hosted runner needs to get added; I will need the github actions runner registration token for this repository, or access to the repository to do this myself.

bors bot added a commit to rust-embedded/wg that referenced this pull request Mar 8, 2022
605: Add @newAM to the Cortex-M team r=adamgreig a=newAM

As discussed in the rust-embedded meeting today; see [this comment](rust-embedded/cortex-m#355 (comment)) for additional context.

Co-authored-by: Alex Martens <[email protected]>
@newAM
Copy link
Member Author

newAM commented Mar 27, 2022

The final steps are done and this should be good to go now! I added the self-hosted runner and rebased onto origin/master.

Repo admins can see the runner in the settings here: https://github.com/rust-embedded/cortex-m/settings/actions/runners

Here's the new job with the runner: https://github.com/rust-embedded/cortex-m/actions/runs/2048592598
The hil-compile-rtt job runs on the github runners, then it passes the binaries off to hil-stm32 to test them on the self-hosted runner.

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your perseverance! I'll leave this to give other @rust-embedded/cortex-m peolpe a chance to look over and if there's no objections we can merge in a couple of days?

@thejpster
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 4, 2022

@bors bors bot merged commit b9e1832 into rust-embedded:master Apr 4, 2022
bors bot added a commit that referenced this pull request Apr 12, 2022
429: Exclude testsuite from cron, fixes #427 r=therealprof a=adamgreig

Turns out #427 was caused by #355 after all, because the CI workflow was updated to exclude testsuite from its default `cargo test`, but the cron workflow wasn't.

For now this seems like the simplest fix; eventually it might be nice to have cron run the testsuite too (at least in qemu), but really cron's there to catch surprise compilation failures when new Rust versions or new dependency releases come out, so I don't think it's urgent.

Co-authored-by: Adam Greig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants