Skip to content

Use general semihosting crate instead of cortex-m specific crate#1046

Merged
knoellle merged 4 commits intoknurling-rs:mainfrom
Siddhant779:user/sidd/semihosting-crate
May 4, 2026
Merged

Use general semihosting crate instead of cortex-m specific crate#1046
knoellle merged 4 commits intoknurling-rs:mainfrom
Siddhant779:user/sidd/semihosting-crate

Conversation

@Siddhant779
Copy link
Copy Markdown
Contributor

@Siddhant779 Siddhant779 commented Apr 2, 2026

Defmt-test build fails for non-CortexM thumb targets. Instead of using a semihosting cortex-m specific crate, use a general semihosting crate.

Fixes #829

@Siddhant779 Siddhant779 marked this pull request as draft April 2, 2026 15:49
@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from a7f0df0 to 345595c Compare April 2, 2026 21:15
@Siddhant779 Siddhant779 marked this pull request as ready for review April 2, 2026 21:24
@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from 345595c to 67372fd Compare April 2, 2026 21:33
@Siddhant779 Siddhant779 marked this pull request as draft April 2, 2026 21:49
@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch 2 times, most recently from 2bd6d1f to 6cf0bc8 Compare April 6, 2026 17:59
@Siddhant779 Siddhant779 marked this pull request as ready for review April 6, 2026 18:04
jonathanpallant
jonathanpallant reviewed Apr 7, 2026
View reviewed changes
Comment thread firmware/qemu/src/bin/panic.rs
jonathanpallant
jonathanpallant previously approved these changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jonathanpallant jonathanpallant left a comment

Choose a reason for hiding this comment

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

One nit that I spotted, but in general this seems fine to me. I use the semihosting crate extensively in the aarch32 repository and it works fine over there, and broadening support beyond cortex-m for this crate I think is useful.

@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from 6cf0bc8 to b0ab752 Compare April 14, 2026 15:40
@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from b0ab752 to d99fd08 Compare April 16, 2026 15:37
@Siddhant779
Copy link
Copy Markdown
Contributor Author

Siddhant779 commented Apr 16, 2026

Hi! We're interested in using the changes from this PR in our project. Is there a timeline for when this might get merged and included in a new crates.io release of defmt-test? Currently our only option would be to pull from git, which isn't ideal for our workflow.

Happy to help with anything needed to move this forward. Thanks!

@knoellle
Copy link
Copy Markdown
Member

knoellle commented Apr 20, 2026

Hello @Siddhant779, thank you for your contribution.
Jonathan already gave his blessing and the changes look fine to me as well.
I'd be happy to push this forward with you, I think we can merge it once the CI is happy.
Checking internally regarding a release.

@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from d99fd08 to e521368 Compare April 23, 2026 15:56
@Siddhant779
Copy link
Copy Markdown
Contributor Author

Siddhant779 commented Apr 23, 2026

Hello @Siddhant779, thank you for your contribution. Jonathan already gave his blessing and the changes look fine to me as well. I'd be happy to push this forward with you, I think we can merge it once the CI is happy. Checking internally regarding a release.

Sounds good thanks @knoellle! Looks like the only test that is failing is host (ubuntu-latest, stable) which seems to be failing on main, so not sure if it's something my PR is introducing. Please keep me updated regarding a release

Thanks

@knoellle
Copy link
Copy Markdown
Member

knoellle commented Apr 24, 2026

The main CI breakage should be fixed with

Please rebase and try again. Enable merge when ready when you're happy with it.
We're also going to be making a release in the coming weeks. :)

@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from 82ddf18 to 9311186 Compare April 26, 2026 20:17
knoellle
knoellle previously approved these changes May 4, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@knoellle knoellle left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
Code LGTM but there is a conflict with #1044 in the changelog entries

Siddhant779 added 4 commits May 4, 2026 09:32
@Siddhant779
Use general semihosting crate instead of cortex-m specific crate
eaca9c9 
Defmt-test build fails for non-CortexM thumb targets. Instead of using
a semihosting cortex-m specific crate, use a general semihosting crate.

Fixes knurling-rs#829
Signed-off-by: Siddhant Pandit <70386749+Siddhant779@users.noreply.github.com>
@Siddhant779
Update QEMU test fw to use generic semihosting crate
43cd201 
Replace cortex-m-semihosting with semihosting in firmware/qemu to match
the defmt-test change.

Signed-off-by: Siddhant Pandit <70386749+Siddhant779@users.noreply.github.com>
@Siddhant779
Add changelog entry for semihosting crate migration
9ff469f 
Signed-off-by: Siddhant Pandit <70386749+Siddhant779@users.noreply.github.com>
@Siddhant779
Exclude defmt-test from cargo-semver-checks
de89aae 
defmt-test now depends on the semihosting crate, which cannot compile
on x86_64. This prevents cargo-semver-checks from running cargo-doc on
the host

Signed-off-by: Siddhant Pandit <70386749+Siddhant779@users.noreply.github.com>
@Siddhant779 Siddhant779 force-pushed the user/sidd/semihosting-crate branch from 9311186 to de89aae Compare May 4, 2026 14:35
@Siddhant779 Siddhant779 requested a review from knoellle May 4, 2026 14:41
@Siddhant779
Copy link
Copy Markdown
Contributor Author

Siddhant779 commented May 4, 2026

Thank you for your contribution! Code LGTM but there is a conflict with #1044 in the changelog entries

Could i get re-approval; Just rebased the PR to deal with the conflict

knoellle
knoellle approved these changes May 4, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@knoellle knoellle left a comment

Choose a reason for hiding this comment

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

LGTM

@knoellle knoellle added this pull request to the merge queue May 4, 2026
Merged via the queue into knurling-rs:main with commit d477bf3 May 4, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knoellle knoellle knoellle approved these changes

@jonathanpallant jonathanpallant Awaiting requested review from jonathanpallant

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

defmt-test build fails for non-CortexM thumb targets

3 participants

@Siddhant779 @knoellle @jonathanpallant
⚡