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

Use critical_section for Peripherals::take. #651

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Aug 24, 2022
@reitermarkus reitermarkus force-pushed the critical-section branch 2 times, most recently from fcb0914 to bd33444 Compare August 24, 2022 00:22
@reitermarkus reitermarkus force-pushed the critical-section branch 5 times, most recently from 5584c86 to 28e0c54 Compare August 24, 2022 01:12
Emilgardis
Emilgardis previously approved these changes Aug 24, 2022
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

lgtm, this is great :D

do we need another reviewer maybe?

@adamgreig
Copy link
Member

I'm a bit concerned about what this means for platforms that don't have critical-section support yet. I know msp430 is working on it but having some codegen size issues, and I don't know anything about mips_mcu. It might be worth waiting a little longer for those architectures to get support before releasing svd2rust that uses it?

Having it as an optional dependency/feature seems useful though and does mean those platforms don't need immediate critical-section support, even end-users could add simple interrupt-based implementations to use take() if needed.

@reitermarkus
Copy link
Member Author

It might be worth waiting a little longer

I don't think it's worth waiting. Other architectures will have to implement these only once they upgrade svd2rust anyways. I have opened issues for the affected architectures to give them a heads up though.

@adamgreig
Copy link
Member

Oh, thanks, I didn't see the edit to the first comment. SGTM to me then.

@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

- Use `critical_section::with` instead of `interrupt::free` for `Peripherals::take`.
Copy link
Member

Choose a reason for hiding this comment

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

A mention to take not being provided if critical-section is not enabled could probably be added here

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM,too. Thanks a lot @reitermarkus for taking this on.

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 24, 2022

Build succeeded:

@bors bors bot merged commit 7daa9ed into rust-embedded:master Aug 24, 2022
@reitermarkus reitermarkus deleted the critical-section branch August 24, 2022 15:53
bors bot added a commit to stm32-rs/stm32-rs that referenced this pull request Oct 18, 2022
770: Add `critical-section` feature. r=adamgreig a=reitermarkus

Depends on rust-embedded/svd2rust#651.

Co-authored-by: Markus Reiter <me@reitermark.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants