Skip to content

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Aug 10, 2022

Picking up #433 since it seems stalled. Changes from #433 are:

  • Update to critical-section 1.0.0-alpha.2
  • Use bool restore token
  • Name Cargo feature critical-section-single-core.

TODO before merging:

Co-Authored-By: Markus Reiter @reitermarkus

@Dirbaio Dirbaio requested a review from a team as a code owner August 10, 2022 16:11
@rust-highfive
Copy link

r? @adamgreig

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Aug 10, 2022
@Dirbaio Dirbaio changed the title Add implementation for crticial-section 1.0 Add implementation for critical-section 1.0 Aug 10, 2022
@reitermarkus
Copy link
Member

Picking up #433 since it seems stalled.

It's not stalled. I was just waiting for another review, same as my other open PRs. Feel free to ping me next time if you want me to rebase a PR.

Thanks anyways for picking this up.

The workflow pull_request change was meant to be temporary so CI can run on my fork.

src/interrupt.rs Outdated
asm!("cpsie i", options(nomem, nostack, preserves_flags));
}

/// Execute closure `f` in an interrupt-free context.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this method should now refer to critical-section, something like this:

Suggested change
/// Execute closure `f` in an interrupt-free context.
/// Execute closure `f` in an interrupt-free context.
///
/// This method does not synchronise multiple cores and may disable required
/// interrupts on some platforms; see the critical-section crate for a cross-platform
/// way to enter a critical section which provides a `CriticalSection` token.
///
/// This crate provides an implementation for critical-section that simply disables
/// all interrupts which may be enabled with the `critical-section-single-core` feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

bors bot added a commit that referenced this pull request Aug 11, 2022
448: Add implementation for critical-section 1.0, for cortex-m v0.7.x r=adamgreig a=Dirbaio

This is a subset of #447 without any breaking changes, just adding the `critical-section` implementation.

The goal is to release it in 0.7.6 so `critical-section` becomes usable without having to wait for cortex-m 0.8.


TODO before merging:

- [x] Wait for `critical-section 1.0` release rust-embedded/critical-section#19

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

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Aug 11, 2022
447: Add implementation for critical-section 1.0 r=adamgreig a=Dirbaio

Picking up #433 since it seems stalled. Changes from #433 are:
- Update to `critical-section 1.0.0-alpha.2`
- Use `bool` restore token
- Name Cargo feature `critical-section-single-core`.

TODO before merging:

- [x] Wait for `critical-section 1.0` release rust-embedded/critical-section#19

Co-Authored-By: Markus Reiter `@reitermarkus` 

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

bors bot commented Aug 12, 2022

Build failed:

@adamgreig
Copy link
Member

bors retry

should go after #450 merges...

@bors
Copy link
Contributor

bors bot commented Aug 12, 2022

@bors bors bot merged commit 0e53054 into rust-embedded:master Aug 12, 2022
bors bot added a commit to rust-embedded/svd2rust that referenced this pull request Aug 24, 2022
651: Use `critical_section` for `Peripherals::take`. r=therealprof a=reitermarkus

- `cortex_m` rust-embedded/cortex-m#447
- `msp430` rust-embedded/msp430#13
- `riscv` rust-embedded/riscv#110
-  `xtensa_lx` esp-rs/xtensa-lx#20, esp-rs/esp-hal#151
- `mips_mcu` kiffie/pic32-rs#5


Co-authored-by: Markus Reiter <[email protected]>
bors bot added a commit to knurling-rs/defmt that referenced this pull request Sep 5, 2022
689: defmt-rtt: Update to critical-section 1.0 r=Urhengulas a=Dirbaio

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:

- [x] Wait for `critical-section 1.0` release rust-embedded/critical-section#19

Co-authored-by: Dario Nieuwenhuis <[email protected]>
bors bot added a commit to nrf-rs/nrf-usbd that referenced this pull request Sep 27, 2022
9: Update to critical-section 1.0 r=jonas-schievink a=Dirbaio

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 (comment)).

TODO before merging:

- [x] Wait for `critical-section 1.0` release rust-embedded/critical-section#19

Co-authored-by: Dario Nieuwenhuis <[email protected]>
bors bot added a commit to rust-embedded/riscv that referenced this pull request Oct 13, 2022
110: Add critical-section 1.0 implementation, fix multicore unsoundness. r=almindor a=Dirbaio

~~Requires #109~~

This adds a [critical-section](https://github.com/rust-embedded/critical-section) implementation for single-core chips, based on disabling all interrupts.

`interrupt::free` is is unsound on multicore systems because it only disables interrupts in the
current core. For multicore chips, a chip-specific critical section implementationis needed instead. Unsoundness is fixed by not returning the `CriticalSection` token.

This is a breaking change.

This is the riscv equivalent of rust-embedded/cortex-m#447 and rust-embedded/cortex-m#448



Co-authored-by: Dario Nieuwenhuis <[email protected]>
@reitermarkus reitermarkus mentioned this pull request Dec 4, 2023
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-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants