Skip to content
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

Update embedded-tests and wiring docs #1652

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Jun 3, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

  • Update probe-rs in our self-hosted-runners
    • Reverted this as it was causing issues on H2.
  • Use the new embedded-test release
  • Updated the wiring on the RPi runners

Testing

Triggered the HIL workflow: https://github.com/esp-rs/esp-hal/actions/runs/9397776874

@SergioGasquez SergioGasquez added the skip-changelog No changelog modification needed label Jun 3, 2024
Copy link
Contributor

@JurajSadel JurajSadel 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!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

@bjoernQ bjoernQ added this pull request to the merge queue Jun 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 3, 2024
@SergioGasquez SergioGasquez added this pull request to the merge queue Jun 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 3, 2024
@MabezDev
Copy link
Member

MabezDev commented Jun 3, 2024

Weird... all the tests passed on the h2 run but the exit code was still 1 🤔

@SergioGasquez
Copy link
Member Author

Weird... all the tests passed on the h2 run but the exit code was still 1

Not all test were executed:

running 10 tests
test tests::test_ecc_affine_point_multiplication                      ... ok
test tests::test_ecc_affine_point_verification                        ... ok
test tests::test_ecc_afine_point_verification_multiplication          ... ok
test tests::test_ecc_jacobian_point_multiplication                    ... ok
test tests::test_jacobian_point_verification                          ... ok
test tests::test_ecc_afine_point_verification_jacobian_multiplication ... ok
test tests::test_ecc_point_addition_256                               ... 
    Finished in 1.024s

Also, the issue seems to be in probe-rs 0.24.0 for some reason, as if I execute the HIL workflow in esp-rs/main (which does not use the updated embedded-test), it also fails: https://github.com/esp-rs/esp-hal/actions/runs/9347902754/job/25726032013

Locally, I can run the whole test suite on H2 with probe-rs 0.24.0 and embedded-test 0.4.0

@SergioGasquez
Copy link
Member Author

Trying to update the probe-rs version on the H2 RPi to see if this solves the issue, it takes around 30 mins

@SergioGasquez SergioGasquez marked this pull request as draft June 3, 2024 11:25
@SergioGasquez
Copy link
Member Author

Reverted probe-rs to rev a6dd038 for the H2 RPi (BrnoRPIRS04) and seems to be working again. Converted this PR to draft until we finish the investigation

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Jun 3, 2024

For some reason, in the RPi runners, cargo install [email protected] --git https://github.com/probe-rs/probe-rs --locked leads to :

espressif@BrnoRPIRS05:/home/sergio $ probe-rs --version
probe-rs 0.24.0 (git commit: v0.22.0-336-g7b9adc83)

While on my local machine

❯ probe-rs --version
probe-rs 0.24.0 (git commit: a385a834)

Its worth noticing that we can't use the suggested installation method with sh script as they do not provide armv7 packages

Also, the rest of the self-hosted runners are using probe-rs 0.24.0 (git commit: v0.22.0-336-g7b9adc83), only the H2 fails with the ecc test

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Jun 4, 2024

Just used cargo install probe-rs-tools, as they suggest in the website https://probe.rs/docs/getting-started/installation/#installation to install latest released version (0.24.0), which results in:

espressif@BrnoRPIRS04:/home/sergio $ probe-rs --version
probe-rs 0.24.0 (git commit: crates.io)

This also makes the CI fail: https://github.com/esp-rs/esp-hal/actions/runs/9363281580/job/25773867739. Will, install rev a6dd038 to get it working again

Edit: H2 should be back to working state, as a6dd038 is installed

@SergioGasquez
Copy link
Member Author

All the RPi runners but the H2 are running probe-rs 0.24.0 (git commit: crates.io). H2 is still in the old probe-rs: probe-rs 0.23.0 (git commit: v0.22.0-291-ga6dd028e), installed with cargo install probe-rs-tools --git https://github.com/probe-rs/probe-rs --rev a6dd038 --force. HIL workflow is passing: https://github.com/esp-rs/esp-hal/actions/runs/9382664007

Not sure what to do with this PR, any ideas? Not sure what's wrong with the H2 runner, the tests run fine on my machine with [email protected].

@SergioGasquez SergioGasquez marked this pull request as ready for review June 6, 2024 08:52
@SergioGasquez
Copy link
Member Author

Reverted the probe-rs update, all runners are back to using 0.23.0 (rev a6dd038). The embedded-test update is working fine (https://github.com/esp-rs/esp-hal/actions/runs/9397776874/job/25881683769), so I guess its worth merging it.

@SergioGasquez SergioGasquez changed the title Update probe-rs and embedded-tests Update embedded-tests Jun 6, 2024
@SergioGasquez
Copy link
Member Author

Opened a probe-rs issue: probe-rs/probe-rs#2546

@SergioGasquez SergioGasquez changed the title Update embedded-tests Update embedded-tests and wiring docs Jun 6, 2024
@SergioGasquez SergioGasquez added this pull request to the merge queue Jun 12, 2024
Merged via the queue into esp-rs:main with commit e7f8f50 Jun 12, 2024
29 checks passed
@SergioGasquez SergioGasquez deleted the feat/probe-rs branch June 12, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants