On a multi-core system, disabling interrupts does not prevent the other core from operating, and so values protected by a bare_metal::Mutex will be incorrectly marked Sync.

Since the overwhelming majority of embedded use cases are single-core, I propose putting a prominent warning in the Mutex docstring for now, and working to develop a safe multi-core extension to the Mutex which can be enabled with a feature gate. Probably something using an atomic to implement a spinlock on top of requiring a CriticalSection.

This function can be compiled and leaks a reference to the stack:

fn bad(cs: &bare_metal::CriticalSection) -> &u32 {
    let x = bare_metal::Mutex::new(42u32);
    x.borrow(cs)
}

The solution is to change the type signature of borrow to:

pub fn borrow<'cs>(&'cs self, _cs: &'cs CriticalSection) -> &'cs T

It seems inefficient to pass around a &'a CriticalSection, since reference to zero-sized types are not themselves zero-sized in rust. In a lot of cases, this would be compiled out, but it is not guaranteed.

It would be better if CriticalSection was defined like this:

struct CriticalSection<'a> {
    _0: PhantomData<&'a ()>,
}

Instead of passing &'a CriticalSection you would then pass a CriticalSection<'a> around. This would have the same purpose

Is there a reason Mutex does not have a borrow_mut()?

Happy to open a PR if desired :)

Based on Matrix discussions, I think I understand why the current Mutex API was chosen. However, using Mutex<RefCell<T>> is a bit more painful than I think it could be. Would it be possible to add an inherent block for Mutex<RefCell<T>>? It would basically combine .borrow(cs) with some of RefCell's methods, so that you don't end up with things like .borrow(cs).borrow_mut().

I can make a PR, but first I wanted to see if it would be a non-starter for some reason. It wouldn't be breaking, right?

We have Mutex. However, opening a critical section (disabling interrupts) to just read or write a Mutex<Cell<u8>> seems a bit overkill: the read or write is a single instruction, which makes it impossible for an interrupt to hit in the middle.
How about this crate also provides some form of atomics with respect to what the Send and Sync markers mean in our context?

(I beleive they would be implemented this way, I'd be very willing to open a PR)

use core::cell::UnsafeCell;

pub struct SingleCoreAtomicUsize {
	inner: UnsafeCell<usize>,
}

/// Safety:
/// We only provide `get` and `set`, which all take a single instruction, so no interrupt may hit in the middle
unsafe impl Sync for SingleCoreAtomicUsize {}

impl SingleCoreAtomicUsize {
	pub const fn new(val: usize) -> Self {
		Self {
			inner: UnsafeCell::new(val),
		}
	}
	pub fn get(&self) -> usize {
		unsafe { *self.inner.get() }
	}
	pub fn set(&self, val: usize) {
		unsafe { *self.inner.get() = val }
	}
}

I personally think CriticalSection should not be Clone, because of two use case requiring that:

• interrupt::enable_cs: Safely enable interrupts, consuming a critical section. This can't work if there can be another copy lying around.
• Mutex::borrow_mut: I'm quite sure this would allow to mutably borrow data in a mutex, by taking a &mut CriticalSection. This would only allow to mutably borrow one mutex as once, but I think it is still better than Mutex<RefCell<_>>.

All actual use cases ought to work by passing a &CriticalSection, although I don't think this needs to be used often. Should the size of &CriticalSection be a problem, one could imagine a SharedCriticalSection ZST borrowing a CriticalSection.

I was just checking how well my crates would play with the 2018 edition and I was able to work around most quirks except for a few with the most major probably being that it seems to be impossible to share peripherals via the good old Mutex<RefCell<Option<_>>> static mechanism due to the inavailability of const_fn:

error[E0015]: calls in statics are limited to tuple structs and tuple variants
  --> examples/i2c_haldriver_printmagserial.rs:31:62
   |
31 | static MAG3110: Mutex<RefCell<Option<Mag3110<I2c<TWI1>>>>> = Mutex::new(RefCell::new(None));
   |                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: a limited form of compile-time function evaluation is available on a nightly compiler via `const fn`
  --> examples/i2c_haldriver_printmagserial.rs:31:62
   |
31 | static MAG3110: Mutex<RefCell<Option<Mag3110<I2c<TWI1>>>>> = Mutex::new(RefCell::new(None));
   |                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is there any good workaround we can use here? Not being able to pass around peripherals and data safely into/out of interrupt handlers would be pretty bad, IMHO.

Part of rust-embedded/wg#383

Blockers:

• remove const-fn now it's in Rust
• specify our MSRV
• Review and merge #20 to fix #7
• Review against https://rust-lang.github.io/api-guidelines/ – happening in #26
• Mutex is not safe on multi-core systems - #12
• Should Peripheral be removed?
• What to do with the Nr trait? (it's gone)

As per rust-embedded/r0#8

Summary

Per #22, we should audit the library against the API Guidelines prior to 1.0 to avoid making breaking changes after the fact.

The checklist is below.
As items are reviewed, we can mark them as completed.
New issues should be opened for items that fail the review.

https://rust-lang.github.io/api-guidelines/checklist.html

Rust API Guidelines Checklist

• Naming (crate aligns with Rust naming conventions)
  • Casing conforms to RFC 430 (C-CASE)
  • Ad-hoc conversions follow as_, to_, into_ conventions (C-CONV)
  • Getter names follow Rust convention (C-GETTER)
  • Methods on collections that produce iterators follow iter, iter_mut, into_iter (C-ITER)
  • Iterator type names match the methods that produce them (C-ITER-TY)
  • Feature names are free of placeholder words (C-FEATURE)
  • Names use a consistent word order (C-WORD-ORDER)
• Interoperability (crate interacts nicely with other library functionality)
  • Types eagerly implement common traits (C-COMMON-TRAITS)
    • Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug,
      Display, Default
  • Conversions use the standard traits From, AsRef, AsMut (C-CONV-TRAITS)
  • Collections implement FromIterator and Extend (C-COLLECT)
  • Data structures implement Serde's Serialize, Deserialize (C-SERDE)
  • Types are Send and Sync where possible (C-SEND-SYNC)
  • Error types are meaningful and well-behaved (C-GOOD-ERR)
  • Binary number types provide Hex, Octal, Binary formatting (C-NUM-FMT)
  • Generic reader/writer functions take R: Read and W: Write by value (C-RW-VALUE)
• Macros (crate presents well-behaved macros)
  • Input syntax is evocative of the output (C-EVOCATIVE)
  • Macros compose well with attributes (C-MACRO-ATTR)
  • Item macros work anywhere that items are allowed (C-ANYWHERE)
  • Item macros support visibility specifiers (C-MACRO-VIS)
  • Type fragments are flexible (C-MACRO-TY)
• Documentation (crate is abundantly documented)
  • Crate level docs are thorough and include examples (C-CRATE-DOC)
  • All items have a rustdoc example (C-EXAMPLE)
  • Examples use ?, not try!, not unwrap (C-QUESTION-MARK)
  • Function docs include error, panic, and safety considerations (C-FAILURE)
  • Prose contains hyperlinks to relevant things (C-LINK)
  • Cargo.toml includes all common metadata (C-METADATA)
    • authors, description, license, homepage, documentation, repository,
      readme, keywords, categories
  • Crate sets html_root_url attribute "https://docs.rs/CRATE/X.Y.Z" (C-HTML-ROOT)
  • Release notes document all significant changes (C-RELNOTES)
  • Rustdoc does not show unhelpful implementation details (C-HIDDEN)
• Predictability (crate enables legible code that acts how it looks)
  • Smart pointers do not add inherent methods (C-SMART-PTR)
  • Conversions live on the most specific type involved (C-CONV-SPECIFIC)
  • Functions with a clear receiver are methods (C-METHOD)
  • Functions do not take out-parameters (C-NO-OUT)
  • Operator overloads are unsurprising (C-OVERLOAD)
  • Only smart pointers implement Deref and DerefMut (C-DEREF)
  • Constructors are static, inherent methods (C-CTOR)
• Flexibility (crate supports diverse real-world use cases)
  • Functions expose intermediate results to avoid duplicate work (C-INTERMEDIATE)
  • Caller decides where to copy and place data (C-CALLER-CONTROL)
  • Functions minimize assumptions about parameters by using generics (C-GENERIC)
  • Traits are object-safe if they may be useful as a trait object (C-OBJECT)
• Type safety (crate leverages the type system effectively)
  • Newtypes provide static distinctions (C-NEWTYPE)
  • Arguments convey meaning through types, not bool or Option (C-CUSTOM-TYPE)
  • Types for a set of flags are bitflags, not enums (C-BITFLAG)
  • Builders enable construction of complex values (C-BUILDER)
• Dependability (crate is unlikely to do the wrong thing)
  • Functions validate their arguments (C-VALIDATE)
  • Destructors never fail (C-DTOR-FAIL)
  • Destructors that may block have alternatives (C-DTOR-BLOCK)
• Debuggability (crate is conducive to easy debugging)
  • All public types implement Debug (C-DEBUG)
  • Debug representation is never empty (C-DEBUG-NONEMPTY)
• Future proofing (crate is free to improve without breaking users' code)
  • Sealed traits protect against downstream implementations (C-SEALED)
  • Structs have private fields (C-STRUCT-PRIVATE)
  • Newtypes encapsulate implementation details (C-NEWTYPE-HIDE)
  • Data structures do not duplicate derived trait bounds (C-STRUCT-BOUNDS)
• Necessities (to whom they matter, they really matter)
  • Public dependencies of a stable crate are stable (C-STABLE)
  • Crate and its dependencies have a permissive license (C-PERMISSIVE)