Skip to content

Merge cortex-m-rt into this repository #391

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 586 commits into from
Jan 23, 2022
Merged

Merge cortex-m-rt into this repository #391

merged 586 commits into from
Jan 23, 2022

Conversation

adamgreig
Copy link
Member

@adamgreig adamgreig commented Jan 12, 2022

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

bors bot and others added 30 commits March 10, 2019 10:42
181: Fix CI cache r=therealprof a=Disasm



Co-authored-by: Vadim Kaushan <[email protected]>
179: Fix incorrect __EXCEPTIONS in lib.rs comment r=therealprof a=raz-a

Update lib.rs comment describing __INTERRUPTS variable to say __INTERRUPTS instead of __EXCEPTIONS

Co-authored-by: Raz Aloni <[email protected]>
Add feature flags into build.rs so SecureFault gets included.
183: Update cortex-m version requirement to allow using 0.6.0 r=therealprof a=adamgreig

See also rust-embedded/cortex-m-semihosting#32

Co-authored-by: Adam Greig <[email protected]>
…ct other packages)

Disable the semihosting test on thumbv8, as it's currently unsupported in cortex-m-semihosting
182: Add thumbv8m.main support. r=korken89 a=thejpster

* Add thumbv8m.main support.
* Also adds feature flags into build.rs so SecureFault gets included.

Co-authored-by: Jonathan 'theJPster' Pallant <[email protected]>
Most target triples with FPU end in `gnueabihf` or `musleabihf`, so this isn't catching them. Since the `has_fpu` function does, this shouldn't change behavior.
185: Remove redundant and incorrect target FPU check r=therealprof a=jonas-schievink

Most target triples with FPU end in `gnueabihf` or `musleabihf`, so this isn't catching them. Since the `has_fpu` function does, this shouldn't change behavior.

Co-authored-by: Jonas Schievink <[email protected]>
this undoes PR #118

I found a problem with keeping this section. Turns out that the input
`.stack_sizes` sections contain references to all functions compiled with `-Z
emit-stack-sizes` (the section contains the addresses of all those functions
after all) so keeping those section prevents the linker from removing *any* of
those functions. This is not a problem today because `-Z emit-stack-sizes` is
*opt-in* and only used to analyze a program.

However, I am proposing a rust-lang/rust PR to build the `compiler-builtins`
crate with `-Z emit-stack-sizes`. That change will cause *all* the functions in
that crate to be kept in binaries that link to `cortex-m-rt`, regardless of
whether the crate author uses `-Z emit-stack-sizes` or not, leading a increase
in binary size of about 14 KB (`.text` section).

To prevent issues with that rust-lang/rust PR I propose we undo PR #118. On the
bright side, the tools that were depending on this (`cargo-stack-sizes` and
`cargo-call-stack`) no longer do so in their latest releases so nothing is lost
on the tooling front with this change.
187: Allow nightly to fail in Travis r=therealprof a=korken89

With nightly being a bit skiddish, lets allow nightly to fail.

Anyone against? @rust-embedded/cortex-m 

- [x] @adamgreig 
- [x] @korken89
- [x] @thejpster
- [ ] @ithinuel 
- [x] @therealprof 

Co-authored-by: Emil Fresk <[email protected]>
186: do not KEEP the .stack_sizes section r=therealprof a=japaric

this undoes PR #118

I found a problem with keeping this section. Turns out that the input
`.stack_sizes` sections contain references to all functions compiled with `-Z
emit-stack-sizes` (the section contains the addresses of all those functions
after all) so keeping those section prevents the linker from removing *any* of
those functions. This is not a problem today because `-Z emit-stack-sizes` is
*opt-in* and only used to analyze a program.

However, I am proposing a rust-lang/rust PR to build the `compiler-builtins`
crate with `-Z emit-stack-sizes`. That change will cause *all* the functions in
that crate to be kept in binaries that link to `cortex-m-rt`, regardless of
whether the crate author uses `-Z emit-stack-sizes` or not, leading a increase
in binary size of about 14 KB (`.text` section).

To prevent issues with that rust-lang/rust PR I propose we undo PR #118. On the
bright side, the tools that were depending on this (`cargo-stack-sizes` and
`cargo-call-stack`) no longer do so in their latest releases so nothing is lost
on the tooling front with this change.

r? @rust-embedded/cortex-m

Co-authored-by: Jorge Aparicio <[email protected]>
184: 0.6.8 release r=therealprof a=korken89



Co-authored-by: Emil Fresk <[email protected]>
Co-authored-by: Daniel Egger <[email protected]>
190: Align sections r=korken89 a=NickeZ

Hey, I had the same issue as in #189. Don't ask me why this works I looked at other linker scripts.

Fixes #189

Co-authored-by: Niklas Claesson <[email protected]>
that contains static variables that will not be initialized by the runtime

this implements only the linker section described in RFC #116 so that downstream
libraries / framework can leverage it without (a) forking this crate or (b)
requiring the end user to pass additional linker scripts to the linker when
building their crate.

This commit doesn't add the user interface described in RFC #116; instead it
documents how to use this linker section in the advanced section of the crate
level documentation
192: add a .uninit section r=therealprof a=japaric

that contains static variables that will not be initialized by the runtime

this implements only the linker section described in RFC #116 so that downstream
libraries / framework can leverage it without (a) forking this crate or (b)
requiring the end user to pass additional linker scripts to the linker when
building their crate.

This commit doesn't add the user interface described in RFC #116; instead it
documents how to use this linker section in the advanced section of the crate
level documentation

Co-authored-by: Jorge Aparicio <[email protected]>
prepare a new patch release that includes PRs #190 and #192
194: v0.6.9 r=adamgreig a=japaric

prepare a new patch release that includes PRs #190 and #192

Co-authored-by: Jorge Aparicio <[email protected]>
198: Discard .ARM.exidx, closes #197 r=thejpster a=adamgreig



Co-authored-by: Adam Greig <[email protected]>
199: v0.6.10 r=korken89 a=Disasm



Co-authored-by: Vadim Kaushan <[email protected]>
thejpster
thejpster previously approved these changes Jan 12, 2022
therealprof
therealprof previously approved these changes Jan 18, 2022
@therealprof
Copy link
Contributor

Let's ship it then?

@adamgreig
Copy link
Member Author

sgtm!

bors r=therealprof

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
Copy link
Contributor

bors bot commented Jan 20, 2022

Build failed:

@adamgreig
Copy link
Member Author

adamgreig commented Jan 20, 2022

Ugh, why is it failing now but not before... did something change in 1.58 👀

Edit: Yep, looks like it works on 1.57 but is now broken on 1.58, boo. I guess we'd have found out sooner or later anyway. I'll see if we can fix it in a way that doesn't break the MSRV check.

@adamgreig adamgreig dismissed stale reviews from therealprof and thejpster via 27434e0 January 21, 2022 00:50
@adamgreig
Copy link
Member Author

That should do it 🤞

@thejpster
Copy link
Contributor

Looks like there's still one red cross?

@adamgreig
Copy link
Member Author

Argh, missed that, thanks. Hopefully this fixes it.

@thejpster
Copy link
Contributor

Huh. We should probably do daily or weekly builds so we catch this stuff that changes underneath us.

@adamgreig
Copy link
Member Author

We do, though currently only for cortex-m: https://github.com/rust-embedded/cortex-m/blob/master/.github/workflows/cron.yml

@adamgreig
Copy link
Member Author

OK, green tick!

@adamgreig
Copy link
Member Author

I've also added cortex-m-rt's test to the weekly scheduled CI. Hopefully good to merge now.

@thejpster
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 23, 2022

👎 Rejected by too few approved reviews

@thejpster
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 23, 2022

@bors bors bot merged commit c350114 into master Jan 23, 2022
@bors bors bot deleted the merge-rt branch January 23, 2022 14:27
@newAM newAM mentioned this pull request Jan 23, 2022
bors bot added a commit that referenced this pull request Apr 4, 2022
355: Add on-target tests to CI r=thejpster a=newAM

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](https://github.com/newAM/totally-not-a-cortex-m-fork/runs/5345451343?check_suite_focus=true) (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:

```console
$ 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.

[`defmt-test`]: https://github.com/knurling-rs/defmt/tree/main/firmware/defmt-test
[`defmt`]: https://ferrous-systems.com/blog/defmt/
[`probe-rs`]: https://probe.rs/
[`probe-run`]: https://github.com/knurling-rs/probe-run
[`rtt-target`]: https://crates.io/crates/rtt-target
[`stm32wlxx-hal`]: https://github.com/stm32-rs/stm32wlxx-hal
[knurling-rs]: https://knurling.ferrous-systems.com/


Co-authored-by: Alex Martens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.