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

defmt-semihosting: use critical_section for synchronization. #943

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

driveraid
Copy link

@driveraid driveraid commented Mar 5, 2025

This change allows defmt-semihosting to be used in multi-core contexts. It updates synchronization to match that of defmt-rtt.

markus added 2 commits March 5, 2025 08:47
This change allows defmt-semihosting to be used in multi-core
contexts.
@driveraid driveraid marked this pull request as draft March 5, 2025 07:58
@jonathanpallant
Copy link
Contributor

Thank you for the PR - I agree this kind of change needs doing.

Unfortunately defmt_rtt's approach here isn't perfect. Instead of a static mut there should probably be an UnsafeCell and instead of three variables, there should probably be one static struct Context to keep it all tidy. I also want to switch to the semihosting crate, because it works on more architectures than just Cortex-M.

@driveraid
Copy link
Author

Do you want all of those in the same PR? Or is it perhaps better in the interest of keeping single-purpose PRs to let this PR merge and cover the other requests in follow-up PRs?

@driveraid
Copy link
Author

... I mean, since there's some refactoring to do (update defmt-rtt to do the same etc)

@driveraid driveraid marked this pull request as ready for review March 5, 2025 12:00
@jonathanpallant
Copy link
Contributor

This is great as-is, so LGTM.

@jonathanpallant jonathanpallant added this pull request to the merge queue Mar 5, 2025
@jonathanpallant jonathanpallant mentioned this pull request Mar 5, 2025
3 tasks
Merged via the queue into knurling-rs:main with commit 335fe81 Mar 5, 2025
23 checks passed
@driveraid
Copy link
Author

Draft PR up for one struct Context: #944

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.

2 participants