Skip to content

qemu-arm: Rework port to provide a proper REPL and run tests through a pty serial port #15624

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 6 commits into from
Aug 28, 2024

Conversation

dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented Aug 9, 2024

Summary

Currently, the qemu-arm (and qemu-riscv) port has two build modes:

  • a simple test that executes a Python string
  • a full test that uses tinytest to embed all tests within the firmware, then executes that and captures the output

This is very different to all the other ports. A difficulty with using tinytest is that with the large number of tests we have the firmware overflows its virtual flash size. It's also hard to run tests via .mpy files and with the native emitter.

Being different to the other ports also means an extra burden on maintenance.

This PR reworks the qemu-arm port so that it has a single build target that creates a standard firmware which has a REPL. When run under qemu-system-arm, the REPL acts like any other bare-metal port, complete with soft reset (and you can use machine.reset() to turn it off and exit qemu-system-arm).

This gives lots of benefits:

  • allows playing with a REPL without hardware
  • allows running the test suite as it would on a bare-metal board, by making qemu-system-arm redirect the UART serial of the virtual device to a /dev/pts/xx file, and then running run-tests.py against that serial device
  • skipping tests is now done via the logic in run-tests.py and no longer needs multiple places to define which tests to skip (tools/tinytest-codegen.py, ports/qemu-arm/tests_profile.txt)
  • allows testing/using mpremote with the qemu-arm port

Eventually the qemu-riscv port would have a similar change. (And then also zephyr, maybe.)

Testing

You can run make test in the qemu-arm port to run the test suite.

CI is updated to use the new scheme.

Tests all pass.

Prior to this PR the test results were:

743 tests ok.  (121 skipped)

With this PR the test results are:

753 tests performed (22673 individual testcases)
753 tests passed
138 tests skipped

(More tests are skipped because more are included in the run. But overall more tests pass.)

Trade-offs and Alternatives

The approach in this PR is an alternative to #15577 and #15609.

This allows us to (eventually) remove the tinytest submodule and framework, and allows simplifying tests/run-tests.py. The downside is that we no longer support tinytest if users want to use it in their own test framework (maybe they cannot get a serial port easily but still want to run tests). But IMO it's not worth supporting tinytest when it's not clear that anyone is even using it.

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.43%. Comparing base (c8838b5) to head (d9a0fdd).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #15624   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files         163      163           
  Lines       21295    21295           
=======================================
  Hits        20961    20961           
  Misses        334      334           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Aug 9, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

This looks really good! Standardising and consolidating the test methods is a big win in my view.

The only suggestion I'd make is to please put some information in the qemu-arm README about how to use the REPL, how to run tests, etc.

One thing I'm curious about is whether semihosting is necessary for this to work well, vs -serial stdio? Do we need to implement semihosting on other qemu ports for this approach to work there?

@dpgeorge
Copy link
Member Author

One thing I'm curious about is whether semihosting is necessary for this to work well, vs -serial stdio? Do we need to implement semihosting on other qemu ports for this approach to work there?

I tried hard to get the semihosting serial stuff working, but I just couldn't work out a way to get qemu-system-arm to redirect the semihosting serial to an independent /dev/pts/X device, and so it wouldn't work properly with run-tests.py (nor mpremote). So in the end I implemented UART RX for all supported boards on this port, and that works will with redirection to /dev/pts/X.

But I left the semihosting code there (see mphalport.c) in case it's useful in the future. After all, it is a board-agnostic way to do serial IO.

That said, semihosting is still needed for the implementation of exit(), which is used by machine.reset() to cleanly close the emulator (if the user wants to do this, eg at the REPL).

For qemu-riscv: I didn't look into that yet. Maybe semihosting will work there, or maybe it also needs a UART for proper redirection to /dev/pts/X.

@projectgus
Copy link
Contributor

@dpgeorge Makes sense, thanks. I had misunderstood that the REPL is over serial in this PR not semihosting.

@dpgeorge dpgeorge force-pushed the qemu-console-interface branch from df00d02 to e437d67 Compare August 14, 2024 01:33
@dpgeorge
Copy link
Member Author

The only suggestion I'd make is to please put some information in the qemu-arm README about how to use the REPL, how to run tests, etc

Now done.

@agatti
Copy link
Contributor

agatti commented Aug 26, 2024

I had a bit of free time today to try this out for RISC-V, so feel free to take a look at the last two commits in https://github.com/agatti/micropython/tree/qemu-console-interface-rv32.

Running REPL and tests work, however I have some failures I still have to look at (can't look at those until tomorrow):

736 tests performed (22589 individual testcases)
732 tests passed
134 tests skipped: ...
4 tests failed: framebuf_polygon int_big_div int_big_lshift int_big_mul

If you want to take a look at those failures you can use the testdebug makefile target, connect gdb to TCP port 1234, and then take it from there. Maybe the ..debug makefile targets could be added to the qemu-arm port too?

@dpgeorge
Copy link
Member Author

I had a bit of free time today to try this out for RISC-V,

Actually... my plan after this PR was to merge both qemu-arm and qemu-riscv into a single port, say qemu. They have a lot overlapping, and the semihosting stuff is factored out to shared/runtime/semihosting_<arch>.[ch]. So I think it would work to merge them, although I haven't tried that yet.

After all, the esp32 and rp2 ports support MCUs with different architectures.

@agatti
Copy link
Contributor

agatti commented Aug 26, 2024

That makes sense. Technically semihosting implementations can also be unified, as RISC-V follows Arm on that front: https://github.com/riscv-non-isa/riscv-semihosting/blob/f74a3f47bfaaaf9dfb614a452c3fe4635deee140/src/intro.adoc?plain=1#L19-L21. With some minor changes to the current code, that can handle Aarch32, Aarch64, RV32, and RV64 in a single implementation.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

New README looks good to me!

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
So multiple boards can be built at once.

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Currently, the qemu-arm (and qemu-riscv) port has two build modes:
- a simple test that executes a Python string; and
- a full test that uses tinytest to embed all tests within the firmware,
  then executes that and captures the output.

This is very different to all the other ports.  A difficulty with using
tinytest is that with the large number of tests the firmware overflows its
virtual flash size.  It's also hard to run tests via .mpy files and with
the native emitter.  Being different to the other ports also means an extra
burden on maintenance.

This commit reworks the qemu-arm port so that it has a single build target
that creates a standard firmware which has a REPL.  When run under
qemu-system-arm, the REPL acts like any other bare-metal port, complete
with soft reset (use machine.reset() to turn it off and exit
qemu-system-arm).

This approach gives many benefits:
- allows playing with a REPL without hardware;
- allows running the test suite as it would on a bare-metal board, by
  making qemu-system-arm redirect the UART serial of the virtual device to
  a /dev/pts/xx file, and then running run-tests.py against that serial
  device;
- skipping tests is now done via the logic in `run-tests.py` and no longer
  needs multiple places to define which tests to skip
  (`tools/tinytest-codegen.py`, `ports/qemu-arm/tests_profile.txt` and also
  `tests/run-tests.py`);
- allows testing/using mpremote with the qemu-arm port.

Eventually the qemu-riscv port would have a similar change.

Prior to this commit the test results were:

    743 tests ok.  (121 skipped)

With this commit the test results are:

    753 tests performed (22673 individual testcases)
    753 tests passed
    138 tests skipped

More tests are skipped because more are included in the run. But overall
more tests pass.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the qemu-console-interface branch from e437d67 to d9a0fdd Compare August 28, 2024 01:57
@dpgeorge dpgeorge merged commit d9a0fdd into micropython:master Aug 28, 2024
66 checks passed
@dpgeorge dpgeorge deleted the qemu-console-interface branch August 28, 2024 02:26
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