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-rtt: Update to critical-section 1.0 #689

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Conversation

Dirbaio
Copy link
Contributor

@Dirbaio Dirbaio commented Aug 10, 2022

This is a breaking change, because 1.0 no longer supplies any critical section implementation by default. The user has to opt-in to one instead (for example, by enabling the critical-section-single-core feature in cortex-m: rust-embedded/cortex-m#447).

Thankfully it needs bumping only defmt-rtt, and not defmt. So it won't cause the painful ecosystem split like the defmt 0.2 -> 0.3 update.

TODO before merging:

@Urhengulas
Copy link
Member

Urhengulas commented Aug 11, 2022

Thank you for the PR. Is there the possibility to make defmt-rtt enable the critical-section-single-core feature or does it always need to be done by the end user?

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Aug 11, 2022

Is there the possibility to make defmt-rtt enable the critical-section-single-core feature or does it always need to be done by the end user?

Has to be done by the end user.

They might be using a multicore chip, in which case using cortex-m/critical-section-single-core would be unsound. They'd need to use another impl, likely from the chip's HAL, using spinlocks to sync the multiple cores. rp2040-hal has one like this, for example.

Or they might not be using cortex-m at all :)

If defmt-rtt force-enables cortex-m/critical-section-single-core, the user can't use other critical section implementations at all (enabling 2 impls on the same binary intentionally causes linking to fail).

@Urhengulas Urhengulas added the breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version label Aug 12, 2022
bors bot added a commit to embassy-rs/embassy that referenced this pull request Aug 17, 2022
901: Update to critical-section 1.0, atomic-polyfill 1.0 r=Dirbaio a=Dirbaio

TODO
- [x] Wait for cortex-m 0.7.6 release rust-embedded/cortex-m#449
- [x] ~~Wait for defmt-rtt 0.4 release knurling-rs/defmt#689 we're still going to use defmt 0.3 for now, which will use the `critical-section` 0.2 default impl, which works.
- [x] Wait for critical-secton `std` impl rust-embedded/critical-section#22

Co-authored-by: Dario Nieuwenhuis <[email protected]>
@Urhengulas
Copy link
Member

There is a small concern of getting the defmt-rtt and defmt version out of sync, which might be confusing to users. But I think we can resolve this soonish.

Additionally, can you please add a section explaining the need to opt-in to a implementation to the docs.rs docs (the doc-comment in the beginning of lib.rs).

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Sep 4, 2022

@Urhengulas

There is a small concern of getting the defmt-rtt and defmt version out of sync, which might be confusing to users.

This is unavoidable if you want to keep compatibility in the main defmt crate. Updating defmt is super problematic. You can't mix major versions of it in the same binary, so if a lib you want to use uses defmt 0.3 you're stuck on that version. This is not a problem for defmt-rtt. It's only used from the main binary, so the end user chooses the version, which can be defmt-rtt 0.4 which is compatible with defmt 0.3.

But I think we can resolve this soonish.

I hope it doesn't mean defmt 0.4, right? 😰

Additionally, can you please add a section explaining the need to opt-in to a implementation to the docs.rs docs (the doc-comment in the beginning of lib.rs).

Done!

@Urhengulas
Copy link
Member

Urhengulas commented Sep 5, 2022

Additionally, can you please add a section explaining the need to opt-in to a implementation to the docs.rs docs (the doc-comment in the beginning of lib.rs).

Done!

Thanks 😄

I hope it doesn't mean defmt 0.4, right? 😰

No no. We want to avoid that as well. We were just thinking how to make it easier for the users. We decided that it is good enough if we document which defmt and defmt-rtt versions are compatible with one another.

@Urhengulas
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 5, 2022

Build succeeded:

@bors bors bot merged commit 16d7f0d into knurling-rs:main Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants