This seems reasonably well implementable on RIOT-OS, but the token size is too small (RIOT's irq_disable / restore lugs around a c_int).

I suggest to have the actual type (which may be type aliased for convenience) depend on a crate feature.

There would be mutually exclusive crate features (likely u8, u16, u32, usize), but that's not an issue because they would only be selected by the crate that provides the interrupt-free operation, and there can only be one of those anyway.

From what I can tell, this library compiles fine for wasm32-unknown-unknown but there is no actual existing implementation (not that I can find on Google?) and you'll end up with

 (import "env" "_critical_section_1_0_release" (func $_critical_section_1_0_release))
 (import "env" "_critical_section_1_0_acquire" (func $_critical_section_1_0_acquire))

in wasm-dis of your output from this error in the browser:

Uncaught TypeError: Error resolving module specifier “env”. Relative module specifiers must start with “./”, “../” or “/”.

I see in the browser.

I see here WASM support was called out: https://github.com/rust-embedded/critical-section/blob/main/CHANGELOG.md#v022---2021-09-13

I also see a list of implementations in the README: https://github.com/rust-embedded/critical-section/tree/main#usage-in-no-std-binaries

I just don't see one that would work for WASM, so I wanted to make sure I wasn't missing something/wrong that one doesn't exist. If it does exist, maybe it could be added to the README?

It looks like to use this crate, we need to also provide a critical-section implementation. Do the maintainers of this project know if there is a list of these implementations maintained somewhere? specifically, I'm looking for something that would work on windows with no_std support.

I've been working on patching in AVR support to a series of libraries around the embedded-nal ecosystem, and eventually needed to patch this one as well.

In doing so, I realized that the Cargo.toml in this repository does not reflect the recent 0.2.4 version bump. This generally complicated my patching journey, and was overall a bit unexpected.

Is there a specific reason that version bump commits aren't merged into main? Looking over the history I see this is not the first instance of this happening. The result is that, for anyone trying to patch this lilbrary in order to fix another one, the workflow around [patch.crates-io] is a bit clunky -- one must either explicitly move their checkout to the tag for the release they're patching, add the version bump to their own checkout, or modify the consumer library's Cargo.toml to depend upon a compatible version.

Critical-section 1.0 removed all builtin impls, because the default of just disabling interrupts is not appropriate in many cases, and is unsound for multicore systems.

However, there are still crates out there depending on the old version, and it's easy to accidentally include it into the dependency tree. That leads to errors which are difficult to debug. For that reason, the old implementation should be removed from the ecosystem, somehow.

The best way to do this was discussed on matrix. This ticket tries to summarize the results of those discussions.

One quick and easy approach would be to just yank all 0.2 versions. So that's the baseline. The solution we finally choose should be at least as good as that one. Until yesterday, that was the planned approach. But then it was noticed that more crates than expected still depend on it indirectly, mainly by depending on an older version of atomic-polyfill. Notably, the latest released version of heapless does this.

These are the properties the solution should have, if possible:

• Make sure that nobody accidentally uses the bad default implementation of just disabling interrupts
• Don't make version 0.2.x a required dependency indefinitely, by depending on it from the 1.0/1.1 version
• Don't break existing crates which indirectly depend on the old version of critical-section
  • (It's ok to break crates with only direct dependencies, as they are easily fixed)
• Don't break if there is a manually supplied custom-impl ¹

Currently, it looks like most of these can be fulfilled with the following approach:

• Publish a version 0.2.8 of critical section which does
  • Implement the critical-section by using critical-section 1.1 internally, if the RestoreState fits into an u8
  • Only implement with (but not acquire/release) if the RestoreState is larger
• Yank versions <= 0.2.7

Some WIP experiments are available at #29 and https://github.com/jannic/critical-section/tree/reverse-compat2 (no polished code, those branches were just used to exchange ideas while discussing this on matrix.)

Hi all,

Correct me if I'm wrong, but my understanding is that Rust doesn't currently have a stable ABI (even when using the same compiler?). If I understand correctly, this would mean that the following scenario is currently not supported by critical-section:

• A static library libfoo.a is compiled. It relies on a critical-section implementation (i.e. has the acquire and release symbols undefined).
• A binary links to libfoo.a and provides a critical-section implementation (i.e. defines the acquire and release symbols).

This scenario is useful when a binary is generic over some foo functionality and cannot list all possible options as features and optional dependencies (and it may also be possible that foo is written in another language and doesn't depend on critical-section).

If _critical_section_1_0_acquire, _critical_section_1_0_release, and RawRestoreState where using the C ABI instead of a Rust ABI, this scenario would work.

Do you think this statement is correct? And if yes, do you think this scenario is something critical-section would be willing to support? And if yes, do you think it would be feasible to use a C ABI for the critical-section ABI?

Thanks!

I would like a way to use mutable globals without unsafe at usage (i.e. contrary to using static mut). I'm in a single core environment and I know that I don't have re-entrancy (i.e. if I have a mutable reference to the global, I will only access it through that reference). I currently have 2 options.

Option 1: Implement a Mutex type myself

use portable_atomic::{AtomicBool, Ordering::SeqCst};
static LOCKED: AtomicBool = AtomicBool::new(false);
static mut DATA: FooBar = FooBar::new();
fn lock<R>(operation: impl FnOnce(&mut FooBar) -> R) -> R {
    assert!(!LOCKED.swap(true, SeqCst));
    let result = operation(unsafe { &mut DATA });
    assert!(LOCKED.swap(false, SeqCst));
    result
}

This is nice because if interrupts are disabled it's only for a very short amount of time when swapping the atomic bool. But this is not nice because I have to use unsafe and implement the whole thing myself. And I'm asking myself: is this even sound?

Option 2: Use critical_section::Mutex

This is nice because I can reuse something existing without unsafe. This is not nice because interrupts are disabled for the whole critical operation. If I know there are no re-entrancy issues (and I'm fine panicking if that's the case because it's a bug), I would prefer interrupts to only be disabled during lock and unlock operations, not during the whole critical section.

Questions

1. Does this assessment make sense? (I'm not even sure the single core requirement is of any use, probably just the re-entrancy is enough, or the guarantee that I won't lock the mutex before next unlock.)
2. Did I miss an existing crate that would solve my problem?
3. Would critical-section be a good candidate to provide such feature? (e.g. providing a PanicMutex or SyncRefCell) Should I ask portable-atomic instead? (but the name does not match) Or should it be a different crate altogether?

Thanks a lot!

The token changes + emitting bare_metal::CriticalSection are really useful, is it worth pushing a 0.3 release now? Perhaps a 1.0 is better, to avoid fragmentation, if so do we have a known set of issues/features/blockers for 1.0?

Mutexes are mostly used for static variables, and also a lot of times the variable needs to be initialized later.

The current Mutex implementation doesn't feel very ergonomic:

1. to allow mutable access + late initialization to a variable, we need to use Mutex<RefCell<Option<T>>> with var.borrow(cs).borrow_mut().replace(value) for initialization and var.borrow(cs).borrow_mut().as_mut().unwrap() for mutable access. These are very long expressions for simple purposes.
2. RefCell comes with an extra isize 32bit space with Option adding another u8 4bit overhead. Each time accessing a mutable reference to a variable basically involves two unwraps, once for checking uniqueness and once for checking initialization, which is unnecessary.

I am thinking of an easier to use + more efficient implementation:

pub struct Mutex<T>(UnsafeCell<MutexInner<T>>);

struct MutexInner<T> {
    state: MutexInnerState,
    value: MaybeUninit<T>,
}

pub struct LockGuard<'cs, T>(&'cs mut MutexInner<T>);

We can use a single enum to keep track of the state of the mutex cell. So the value can be

• uninitialized (None),
• locked (borrowed),
• unlock (free to borrow).

enum MutexInnerState {
    Locked,
    Uinit,
    Unlock,
}

The mutex can be either initialized with a value or not.

impl<T> Mutex<T> {
    /// Creates a new mutex.
    pub const fn new(value: T) -> Self {
        Self(UnsafeCell::new(MutexInner {
            state: MutexInnerState::Unlock,
            value: MaybeUninit::new(value),
        }))
    }

    /// Creates a new unit mutex.
    pub const fn new_uinit() -> Self {
        Self(UnsafeCell::new(MutexInner {
            state: MutexInnerState::Uinit,
            value: MaybeUninit::uninit(),
        }))
    }
}

Value can be initialized once if it was not initialized (otherwise panic).

impl<T> Mutex<T> {
    /// Value initialization.
    ///
    /// panic if already initialized.
    pub fn init<'cs>(&'cs self, _cs: &'cs CriticalSection, value: T) {
        let inner = unsafe { &mut *self.0.get() };
        if let MutexInnerState::Uinit = inner.state {
            inner.state = MutexInnerState::Unlock;
            inner.value = MaybeUninit::new(value);
        } else {
            panic!()
        }
    }
}

Locking the mutex returns None if try_lock fails or if the value is uninitialized.

impl<T> Mutex<T> {
    /// Try to lock the mutex.
    pub fn try_lock<'cs>(&'cs self, _cs: &'cs CriticalSection) -> Option<LockGuard<'cs, T>> {
        let inner = unsafe { &mut *self.0.get() };
        match inner.state {
            MutexInnerState::Uinit | MutexInnerState::Locked => None,
            MutexInnerState::Unlock => {
                inner.state = MutexInnerState::Locked;
                Some(LockGuard(inner))
            }
        }
    }
}

The LockGuard restore lock state back to Unlock on drop.

impl<'cs, T> Drop for LockGuard<'cs, T> {
    fn drop(&mut self) {
        self.0.state = MutexInnerState::Unlock;
    }
}

Miscellaneous.

impl<T> Drop for Mutex<T> {
    fn drop(&mut self) {
        let inner = unsafe { &mut *self.0.get() };
        if let MutexInnerState::Unlock | MutexInnerState::Locked = inner.state {
            unsafe { inner.value.assume_init_drop() }
        }
    }
}

impl<'cs, T> Deref for LockGuard<'cs, T> {
    type Target = T;

    #[inline]
    fn deref(&self) -> &Self::Target {
        unsafe { self.0.value.assume_init_ref() }
    }
}

impl<'cs, T> DerefMut for LockGuard<'cs, T> {
    #[inline]
    fn deref_mut(&mut self) -> &mut Self::Target {
        unsafe { self.0.value.assume_init_mut() }
    }
}

unsafe impl<T> Sync for Mutex<T> where T: Send {}

Usage

Now we can write like

// compared to `Mutex <RefCell<Option<Rtc<RTC0>>>>`
static RTC: Mutex<Rtc<RTC0>> = Mutex::new_uinit();

// initialization
cortex_m::interrupt::free(|cs| RTC.init(cs, rtc0));

// access
#[interrupt]
fn RTC0() {
    cortex_m::interrupt::free(|cs| {
        if let (Some(mut rtc), Some(mut other)) = (RTC.try_lock(cs), OTHER.try_lock(cs)) {
            other.handle_interrupt(&mut rtc);
        }
    });
}

Of course we should not use the name Mutex directly since we need to have backward compatibility. I am thinking of naming it MutexCell or perhaps MutexOption. If anyone finds it useful maybe we can PR it into the library.

Currently the docs.rs page for critical-section does not mention trait Impl or macro custom_impl, which only appears in public API when a particular feature is set and contains some documentation.
I think docs.rs Cargo metadata should be set to allow docs.rs show all the API. cfg_attr(..., doc(feature(...)) trick may be additionally used to render "This is supported on crate feature ... only" badges on those APIs.

Is there a way for a HAL (or similar) to provide a critical-section by default, with the option to disable it if needed?

This would be the most user friendly behavior I can currently think of:

• the HAL crate should provide an implementation by default, so users just need to depend on the HAL to have the implementation activated. (A HAL usually knows the hardware and therefore knows quite well what critical-section implementation would be safe.)
• an optional feature custom-impl would disable that default impl. That way, in some special cases, a user might provide an own implementation, eg. if the target is multicore, but the firmware only uses a single core, so a complex and expensive critical-section could be replaced by just disabling interrupts

However, this doesn't work: To be able to implement the critical section, the HAL would need to select on of the restore-state-* features of critical-section. It is not possible to disable that feature when custom-impl is selected. Cargo just provides no way to say "enable this feature of a dependency only if this other dependency is not enabled" (or does it, somehow?).

The second best approach would be:

• the HAL crate should provide an implementation if a default-enabled feature provide-critical-section-impl is selected, so users still just need to depend on the HAL to have the implementation activated.
• to be able to provide a custom impl, the dependency on the HAL would need to be declared with default-features = false.

This would work, but not well: It's not uncommon that several crates in the dependency tree depend on the HAL. If only one of those forgets to set default-features = false, the implementation is included. So a user who wants to provide a custom impl would not only need to add default-features = false in one dependency, but might have to hunt down unintended activation of the feature via indirect dependencies.

The remaining option is to just not provide the impl by default, but require a feature to be activated manually. So every binary (but not libraries!) depending on the HAL would need to add features = [ "provide-critical-section-impl" ].

Did I miss any other options? Which approach would meet the intentions of the critical-section authors?

It would be useful if there was a changelog entry for 0.2.8, particularly as this release breaks compilation. (Ideally the new changelog entry for 0.2.8 would specify a remedy for the broken compilation.)

I added a dependency on nrf-softdevice and compiled and run successfully. I then enabled it's feature critical-section-impl. I got this linker error:

  = note: rust-lld: error: undefined symbol: _critical_section_acquire
          >>> referenced by lib.rs:12 (/home/daniel/.cargo/registry/src/github.com-1ecc6299db9ec823/critical-section-0.2.5/src/lib.rs:12)
          >>>               embassy_nrf-157f5751c86f3327.embassy_nrf.810121b8-cgu.13.rcgu.o:(critical_section::acquire::h4d7a7bd07cfb7249) in archive /home/daniel/niss/plant/target/thumbv7em-none-eabihf/debug/deps/libembassy_nrf-157f5751c86f3327.rlib
          >>> referenced by lib.rs:12 (/home/daniel/.cargo/registry/src/github.com-1ecc6299db9ec823/critical-section-0.2.5/src/lib.rs:12)
          >>>               embassy-04a2f64e8bc8daf6.embassy.4964beb4-cgu.6.rcgu.o:(critical_section::acquire::h81bb27e6d92b2c20) in archive /home/daniel/niss/plant/target/thumbv7em-none-eabihf/debug/deps/libembassy-04a2f64e8bc8daf6.rlib
          
          rust-lld: error: undefined symbol: _critical_section_release
          >>> referenced by lib.rs:21 (/home/daniel/.cargo/registry/src/github.com-1ecc6299db9ec823/critical-section-0.2.5/src/lib.rs:21)
          >>>               embassy_nrf-157f5751c86f3327.embassy_nrf.810121b8-cgu.13.rcgu.o:(critical_section::release::hfec835aae3ad7b36) in archive /home/daniel/niss/plant/target/thumbv7em-none-eabihf/debug/deps/libembassy_nrf-157f5751c86f3327.rlib
          >>> referenced by lib.rs:21 (/home/daniel/.cargo/registry/src/github.com-1ecc6299db9ec823/critical-section-0.2.5/src/lib.rs:21)
          >>>               embassy-04a2f64e8bc8daf6.embassy.4964beb4-cgu.6.rcgu.o:(critical_section::release::h00dffdccd91523f4) in archive /home/daniel/niss/plant/target/thumbv7em-none-eabihf/debug/deps/libembassy-04a2f64e8bc8daf6.rlib

With a cargo clean everything worked again.

My guess is this is a known limitation of the techniques you use. If so, it would have saved me some time if the docs for critical-section suggested trying cargo clean.

.cargo/config.toml file content:

[build]
target = "avr-specs/avr-atmega328p.json"

[target.'cfg(target_arch = "avr")']
runner = "ravedude uno -cb 57600"

[unstable]
build-std = ["core,alloc"]

problem:

  Compiling critical-section v0.2.7
   Compiling hash32 v0.2.1
   Compiling nom v5.1.2
   Compiling fugit v0.3.6
error: cannot find macro `llvm_asm` in this scope
   --> C:\Users\atmel\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\critical-section-0.2.7\src\lib.rs:127:13
    |
127 |             llvm_asm!(
    |             ^^^^^^^^

error: cannot find macro `llvm_asm` in this scope
   --> C:\Users\atmel\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\critical-section-0.2.7\src\lib.rs:139:17
    |
139 |                 llvm_asm!("sei" :::: "volatile");
    |                 ^^^^^^^^

error[E0635]: unknown feature `llvm_asm`
 --> C:\Users\atmel\.cargo\registry\src\mirrors.ustc.edu.cn-61ef6e0cd06fb9b8\critical-section-0.2.7\src\lib.rs:3:42    
  |
3 | #![cfg_attr(target_arch = "avr", feature(llvm_asm))]
  |                                          ^^^^^^^^

The old llvm_asm! has been removed. Therefore, it doesn't compile on new rustc anymore.