-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
embedded-test
integration
#2292
Conversation
When a test fails on riscv, I get the following output, if I print a stacktrace:
We should de-noise this output :) |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Thanks for all your work here @t-moe! I've updated my fork of esp-hal and did some local test of our HIL tests, here is a summary of the current status:
Edit: Did some testing flashing some ESP32-C2 apps (using
|
49cae5e
to
9154aae
Compare
9154aae
to
b431b24
Compare
I would like to get a first round of reviews for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of what I can see are stylistical nitpicking. This is really exciting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good from my perspective, just some nits around log levels now that things are working
Actually, the architecture matching (#2292 (comment)) was dumb, after reverting the changes (#2308):
So both archs work fine with how it was. |
Since we now support Xtensa devices, tried to run tests with this branch for Xtensa targets: ESP32 Output Error
ESP32-S2 Output Error
|
I tried to improve the semihosting stuff today. Recall the two problems (top comment)
Especially the first point needs careful review. Please look at a4789d5 . I have only a risc-v here, and of course I validated the change there, but for the other platforms I need your eyes. Thank you! |
I've also fixed the remaining ToDo and you can now obtain a test report on stdout, so that it can be processed by the CI pipeline. e.g. |
Just had a look and it looks good to me. I've also retested Xtensa and RISC-V targets:
|
C2/C3 issue could be a known limitation of defmt where the debugger reads the control block before its fully initialized. S2 looks fine, but the panic message should be printed somewhere (look at the test outcome). ESP32, unknown what this issue is. Edit: We don't yet have ESP32 in CI, so we shouldn't block this PR on it. The C2/C3 issue is intermittent and unlikely to be related to this PR. I think the only thing worth investigating is where the panic message has gone for the S2, this is something that could be relevant to all targets. |
@t-moe I think the only thing to solve here is panic messages disappearing. I was aware of a similar issue in #1890. It looks like the same sort of issue. |
I think you dont see the panic message on the S2 because of the same rtt error as on the C2 +C3. See the output from the S2 here: #2292 (comment) |
Ah sorry, I didn't see the logs in that comment 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We've been using this in esp-hal for a the last week, and no issues so far. Thanks for working on this @t-moe!
Thanks for working on this, let's hammer out issues separately. |
Yay 🥳 , thanks for merging this. |
I have been able to run the example embedded-test project on my S2 without too much trouble. The output is a bit weird to read with defmt output (the first message has no preceding newline because the test status doesn't expect one), but that's nothing serious, so good job! @SergioGasquez do you have a cloneable reproducer for your S2/ESP32 issues? Are you confident that the tests you're running are supposed to pass? |
Same tests are running successfully for other targets, and they should be the same in ESP32. |
…l request probe-rs#2292 This reverts commit a4789d5. This commit was squashed into 8e60d01.
…ll request probe-rs#2292 This reverts commit 3b6a3b1. This commit was squashed into 8e60d01.
I've rebased/rewritten
feature/testing
.probe-rs run
will now autodetect whether it is a test binary or a normal binary. You no longer needprobe-rs test
(adapt your cargo runner!)New `probe-rs run --help` output
Remaining TODOs:
decode_semihosting_syscall
in every call toCoreInterface::status()
, even if the target has not been continued between the calls. This is inefficient and leads to strange error messages (e.g. semihosting operation=0x00, because the register in question was overriden by the semihosting return value)SYS_GET_CMDLINE
), should write a default return value back to the CPU register, so that even if the semihosting operation is not answered by the application, the core reads something valid back.RustRover can handle the current output just fine though
Test it
In order to test this branch, you need to use a diferent branch of embedded-test: https://github.com/probe-rs/embedded-test/compare/next?expand=1
I've also updated the example project: https://github.com/probe-rs/embedded-test-example/tree/next
You can test the whole thing for the esp32c6 with the following commands: