Skip to content

Use probe-rs instead of cargo embed#299

Merged
geonnave merged 10 commits intolake-rs:mainfrom
WilliamTakeshi:remove_cargo-embed
Aug 12, 2024
Merged

Use probe-rs instead of cargo embed#299
geonnave merged 10 commits intolake-rs:mainfrom
WilliamTakeshi:remove_cargo-embed

Conversation

@WilliamTakeshi
Copy link
Copy Markdown
Collaborator

@WilliamTakeshi WilliamTakeshi commented Aug 2, 2024

I've taken a shot at removing cargo embed and using probe-rs instead. #298

It's working using cargo run --target="thumbv7em-none-eabihf"

Additionally, I've switched to using defmt in place of cortex_m_semihosting::hprintln. If this change is undesirable, please let me know and I can revert it.

However, I encountered an issue: I couldn't get the tests to work with QEMU, as my computer gets stuck on every test. I tried running older commits, but nothing seems to work. Does cargo run with the default target work for you?

I'm marking this as a draft so we can resolve the QEMU issue before merging.

Thanks!

@WilliamTakeshi WilliamTakeshi marked this pull request as draft August 2, 2024 18:15
@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Aug 4, 2024

Thanks for this PR!
Replacing hprintln with defmt is a good thing IMO.
Do we still need (defmt-)rtt when we have defmt?
As for the QEMU issue, I will test it tomorrow morning (although run-example-on-qemu passed on CI, so I think it will be fine).

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Aug 5, 2024

FYI, will test tomorrow instead since I cannot compile the psa backend in my M1 machine.

@WilliamTakeshi
Copy link
Copy Markdown
Collaborator Author

If replacing hprintln with defmt is fine, I will update this PR from draft to ready for review!

Do we still need defmt-rtt when we have defmt?

Yes, without defmt-rtt I was getting a bunch of linking issues like:

rust-lld: error: undefined symbol: _defmt_acquire
rust-lld: error: undefined symbol: _defmt_release
rust-lld: error: undefined symbol: _defmt_write

I found a issue that the solution was to keep the use defmt_rtt as _; to add a global logger

Let me know how the QEMU test goes!

@WilliamTakeshi WilliamTakeshi marked this pull request as ready for review August 5, 2024 16:10
@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Aug 6, 2024

I tried here, and I it indeeds gets stuck at

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.83s
     Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel /home/gfedrech/Developer/inria/dev/lakers-FORK/target/thumbv7m-none-eabi/debug/lakers-no_std`
Timer with period zero, disabling

On the other hand, the version on main prints:

$ cargo run  --no-default-features --features="crypto-psa" --release
    Finished `release` profile [optimized] target(s) in 4.36s
     Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel /home/gfedrech/Developer/inria/dev/lakers-FORK/target/thumbv7m-none-eabi/release/lakers-no_std`
Timer with period zero, disabling
Hello, lakers!
Test test_new_initiator passed.
Test test_p256_keys passed.
Test test_prepare_message_1 passed.
Test test_handshake passed.
All tests passed.

(although I just realised that the QEMU version doesn't work without the --release flag, even on main).


Have you tried using defmt-semihosting as mentioned here? It seems it should be used for testing with QEMU.

@WilliamTakeshi
Copy link
Copy Markdown
Collaborator Author

@geonnave Thanks for the defmt-semihosting suggestion. I tried it out, but it appears that in addition to adding defmt-semihosting, I need to integrate a printer, a decoder, and a parser to properly process and display the data from QEMU.

This is how I understood they do in their example. Without this printer/decoder/parser combo, the log output appears as gibberish.

Because of this, I reverted to using use cortex_m_semihosting::hprintln as info; for the QEMU case and updated the README to reflect these changes.

Let me know if you need any further adjustments!

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Aug 7, 2024

Thanks, left an editorial comment. Can you also please update the relevant section in the main README?

Feel free to add me as collaborator in your fork, this way I can also push some changes when needed (like updating the README).

@geonnave
Copy link
Copy Markdown
Collaborator

geonnave commented Aug 8, 2024

I also just noticed, could you rebase and use the scope: description pattern in the commit messages? Just to keep the same pattern as the rest of the commits. (basically just add the example: prefix to all commits)

@WilliamTakeshi
Copy link
Copy Markdown
Collaborator Author

I also just noticed, could you rebase and use the scope: description pattern in the commit messages? Just to keep the same pattern as the rest of the commits. (basically just add the example: prefix to all commits)

Sorry about that! I didn't knew you guys were using conventional commit messages. I've rebased the branch and updated the commit messages. Thanks for the heads up!

@geonnave geonnave merged commit 7cd68f1 into lake-rs:main Aug 12, 2024
@geonnave geonnave added the type:enhancement New feature or request label Aug 12, 2024
@WilliamTakeshi WilliamTakeshi deleted the remove_cargo-embed branch September 15, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants