[RFC] Peripherals as scoped singletons

[japaric]

Motivation

Today, in svd2rust and cortex-m APIs, peripherals are modeled as global singletons that require
some synchronization, e.g. a critical section like interrupt::free, to be modified.

The consequences of this choice is that (a) driver APIs are not ergonomic as one would expect, see
below:

use stm32f103xx::TIM6;

// Periodic timer
struct Timer<'a>(&'a TIM6);

impl<'a> Timer<'a> { .. }

fn main() {
    interrupt::free(|cs| {
        let tim6 = TIM6.borrow(cs);
        let timer = Timer(tim);
        timer.init(..);
        // ..
        timer.resume();
    });
}

interrupt!(TIM6, tim6);

fn tim6() {
    interrupt::free(|cs| {
        let tim6 = TIM6.borrow(cs);
        let timer = Timer(tim);
        timer.clear_update_flag();
        // ..
    });
}

Here the Timer abstraction has to be reconstructed in each execution context. One would prefer to
instantiate Timer as a static variable and then use that from each execution context. However,
that's not possible with this Timer struct because of the non-static lifetime of the inner field.
(It is possible to remove the inner field from Timer at the expense of having a critical section
per method call -- which has worse performance than the non-static lifetime approach).

Even worst is that (b) driver abstractions can be easily broken due to the global visibility
property of peripherals. This means that there's no way to e.g. make sure that TIM6 is only used
as a Timer in main and tim6. Nothing prevents you, or some other crate author, from silently
using TIM6 in other execution context -- TIM6 doesn't even have to appear as a function argument
because it's always in scope. This issue not only breaks abstractions; you can also have race
conditions on TIM6 -- yes, even with interrupt::free you can have race conditions and that's
totally memory safe per Rust definition -- if Timer is being used in other execution contexts
and you don't know about it.

So, what can we do to fix these issues? We can remove the root of all these problems: global
visibility.

This RFC is proposing to go from global singletons to scoped singletons. Instead of having
peripheral singletons with global visibility you'll have to explicitly import a peripheral
singleton into scope, into the current execution context. Because we are talking about singletons
here you can only import singleton P into scope (execution context) S once. IOW, if you imported P
into an execution context S then you can't import P into another scope S'.

This RFC not only addresses the problems (a) and (b) mentioned above; it also helps us tackle the
problem of compile time checked configuration -- more about that later on.

Detailed design

Zero sized proxies that represent a peripheral register block, as shown below, will be added to
cortex-m and svd2rust generated crates.

pub struct GPIOA { _marker: PhantomData<*const ()> }

// Peripherals are `Send` but *not* `Sync`
unsafe impl Send for GPIOA {}

impl Deref for GPIOA {
    type Target = gpioa::RegisterBlock;

    fn deref(&self) -> &gpio::RegisterBlock { /* .. */ }
}

These proxies will be impossible to directly instantiate. Instead there will be a guarded function
that returns all the proxies only once.

pub struct Peripherals {
    pub GPIOA: GPIOA,
    pub GPIOB: GPIOB,
    // ..
}

impl Peripherals {
    pub fn all() -> Option<Self> {
        interrupt::free(|_| unsafe {
            static mut USED = false;

            if USED {
                None
            } else {
                USED = true;
                Some(Peripherals { .. })
            }
        })
    }
}

The user will be able to access the proxies like this:

fn main() {
    let p = Peripherals::all().unwrap(); // first call: OK
    //let p = Peripherals::all().unwrap(); // a second call would panic!

    p.GPIOA.bsrr.write(|w| /* .. */);
    p.GPIOB.bsrr.write(|w| /* .. */);
}

Thus the proxies are singletons: there can only exist a single instance of each of them during the
whole lifetime of the program. The proxies are also scoped, in this case they are tied to main, so
they are not visible to other execution contexts, unless they are explicitly moved into another
execution context.

Zero cost

An unsafe, unguarded variant of Peripherals::all will also be provided:

static mut USED = false;

impl Peripherals {
    pub fn all() -> Option<Self> {
        interrupt::free(|_| unsafe { /* .. */ })
    }

    // NOTE for safety this function can only be called once and before `Peripherals::all` is called
    pub unsafe fn _all() -> Self {
        USED = true;
        Peripherals { .. }
    }
}

When only the unsafe variant is used the cost of having scoped singletons becomes zero:

fn main() {
    let p = unsafe { Peripherals::_all() };

    p.GPIOA.bssr.write(|w| /* .. */)
}

This program has the same cost as using a global, unsynchronized GPIOA register block (which is
what you see in C HALs).

Sidestepping the proxy

Each peripheral will provide a static method, ptr, that returns a raw pointer into the register
block:

impl GPIOA {
    pub fn ptr() -> *const gpioa::RegisterBlock {
        0x4001_0800 as *const _
    }
}

This is useful for implementing safe APIs that perform "unsynchronized" reads on registers that
have no side effect when read:

impl DWT {
    /// Reads the cycle counter
    // NOTE this is a static method and doesn't require an instance of `DWT`
    pub fn cyccnt() -> u32  {
        unsafe { (*DWT::ptr()).cyccnt.read() }
    }
}

// time something
let before = DWT::cyccnt();
// ..
let after = DWT::cyccnt();
let elapsed = after.wrapping_sub(before);

Enabling new APIs

Scoped singletons effectively give move semantics to peripherals. This enables richer, type safer
APIs than what can be expressed with the current peripheral API. Let's see some examples:

(you can find some initial experiments with these APIs in the singletons branch of the f3
repository)

Type state as a contract

Digital I/O pins can be configured as inputs, outputs or to some special functionality like serial,
SPI or I2C. In some cases you want to configure a pin to operate in a certain mode for the duration
of the whole program; that is you don't want the pin to be re-configured by mistake.

Type state is a good way to encode this property: a type is used to encode the state of an object.
To transition the object from a state to another it needs to be moved so that the previous state
can no longer be used.

Here's a tentative GPIO API that uses type state:

use blue_pill::GpioExt;

let p = Peripherals::all().unwrap();

// from `GpioExt`: `fn pins(self) -> gpioa::Pins`
// `Pins` is a struct that contains all the pins. Each pin is (also) a singleton (!).
let pins = p.GPIOA.pins();

// `fn as_output(self) -> Output<Self>`
// `as_output` configures a pin as an output pin
let pa0 = pins.PA0.as_output();

// `fn as_input(self) -> Input<Self>`
// `as_input` configures a pin as an input pin
let pa1 = pins.PA1.as_input();

// `Output::high(&mut self)`
// set PA0 high (to 3.3V)
pa0.high();

// `Input::is_high(&self) -> bool`
if pa1.is_high() {
    // ..
}

// this would cause a compile error because `PA0` is not in the `Input` state
// pa0.is_high();

// this would cause a compile error because `PA1` is not in the `Output` state
// pa1.high();

// this would cause a compile error because `GPIOA` was consumed by the `pins` call
// p.GPIOA.moder.modify(|_, w| /* configure PA0 as an input */)

Here the Input and Output newtypes are used to encode the type state. The most important part
here is that GPIOA is consumed to generate the individual pins and thus it can't no longer be
used to configure the pins -- which would break Input / Output contract of "this pin can only
be used as an input for the rest of the program".

Compile time pin allocation

On modern microcontrollers a single pin can be configured to have one of many functionalities
(Serial, PWM, I2C, etc.). This lets vendor pack more peripherals in a microcontroller without
increasing the number of pins.

A problem that arises from this flexibility is that one might wrongly configure two, or more,
peripherals to use the same pin. With move semantics you can construct an API that ensures that
this can't happen:

let p = Peripherals::all().unwrap();
let pins = p.GPIOA.pins();

// use PA9 and PA10 for the USART1 serial port

// NOTE consumes `pa9`
let tx = pa9.as_usart1_tx();

// NOTE consumes `pa10`
let rx = pa10.as_usart1_rx();

// `Serial::new` in turn consumes `tx` and `rx`
let serial = Serial::new(p.USART1, (tx, rx), 115_200.bps());

// this would error because `pa9` was consumed above
//let ch2 = pa9.as_tim1_ch2();
//let pwm = Pwm::new(p.TIM1, ch2, 10.khz());

Here we have high level abstractions like Serial consume the pins they are going to use. This way
once one such abstraction is created no other abstraction can't use any of the pins the first one is
using.

Non overlapping register access

The "split" technique used for GPIO pins can also be used to split a peripherals in "parts" that (a)
modify different registers and (b) modify non overlapping parts of a same register. This comes in
handy with peripherals like the DMA which usually interacts with several other peripherals.

Vendors usually design their DMA peripherals so that even if the settings related to different
channels are stored in a single register that register can be modified atomically using e.g. "clear
flag" bits (no RMW operation required to clear a flag). If the vendor doesn't provide such mechanism
bit banding can probably be used in its place, if the device has support for it.

RTFM protects peripherals at the register block level. Without move semantics, to clear "transfer
complete" interrupt flags from two interrupts handlers running at different priorities you would
need a lock in the lowest priority handler:

// priority = 2
fn dma1_channel1(r: DMA1_CHANNEL1::Resources) {
    r.DMA1.lock(|dma1| {
        // clear the transfer complete flag for this channel
        dma1.ifcr.write(|w| w.ctcif1().set_bit());
        // ..
    });
}

// priority = 3
fn dma1_channel2(r: DMA1_CHANNEL2::Resources) {
    let dma1 = r.DMA1;
    // clear the transfer complete flag for this channel
    dma1.ifcr.write(|w| w.ctcif2().set_bit())
    // ..
}

But with move semantics you can split the DMA in channels and assign exclusive access to each
channel to each interrupt handler:

fn init(p: init::Peripherals) -> init::LateResourceValues {
    let channels = p.DMA1.split();

    // ..

    init::LateResourceValues { CH1: channels.1, CH2: channels.2 }
}

// priority = 2
// resources = [CH1]
fn dma1_channel1(r: DMA1_CHANNEL1::Resources) {
    // clear the transfer complete flag for this channel
    r.CH1.clear_transfer_complete_flag();
}

// priority = 3
// resources = [CH2]
fn dma1_channel2(r: DMA1_CHANNEL2::Resources) {
    // clear the transfer complete flag for this channel
    r.CH2.clear_transfer_complete_flag();
}

Thus no locking is needed. Each channel will operate on a non-overlapping portion of the shared
IFCR register.

Configuration freezing

In some cases you want to configure the core and peripherals clocks to operate at certain
frequencies during initialization and then make sure that these frequencies are not changed later at
runtime.

Again, move semantics can help here by "discarding" the peripheral in charge of clock configuration
once the clock has been configured:

use blue_pill::RccExt;

let p = Peripherals::all().unwrap();

// .. use `p.RCC`, or a higher level API based on it, to configure the clocks ..

// from `RccExt`: `fn clocks(self) -> Clocks`
// `clocks` contains information about the operating frequency of the peripheral buses
let clocks = p.RCC.freeze();

// To compute USART1 internal prescalers and achieve the desired baud rate,
// information about the current clock configuration is required.
// `clocks` provides that information
let serial = Serial::new(p.USART1, (tx, rx), &clocks, 115_200.bps());

Here, once the clock is configured, its configuration gets frozen by consuming / discarding the
RCC peripheral. With this ... move the clock frequencies can no longer be changed. Freezing RCC
returns a Clocks struct that contains the frequency of every peripheral bus. This information is
required to properly configure the operating frequency of each peripheral so it's passed to
peripherals' init functions.

Drawbacks

Regression when not using RTFM

And you still want to use interrupts.

RTFM supports moving runtime initialized resources into tasks (interrupts) at zero, or very little,
cost but if you are not using RTFM then a static variable is required to share a peripheral
between e.g. main and an interrupt handler. Putting a peripheral singleton in a static variable
means making it globally visible which means you have global singletons again, and all their
disadvantages, but with worse performance (because an Option and a RefCell are needed, see
below) than what you get with today's API.

With this RFC:

// you want to share `GPIOA` between `main` and `exti0`
// but this makes it global so any other execution context can also use it
static GPIOA: Mutex<RefCell<Option<GPIOA>>> = Mutex::new(RefCell::new(None));

fn main() {
    // initialize `GPIOA`
    let p = Peripherals::all().unwrap();
    interrupt::free(move |cs| {
        *GPIOA.borrow(cs).borrow_mut() = Some(p.GPIOA);
    });

    loop {
        interrupt::free(|cs| {
            let gpioa = GPIOA.borrow(cs).borrow().as_ref().unwrap();

            // ..
        });

        // ..
    }
}

interrupt!(EXTI0, exti0);

fn exit0() {
    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs).borrow().as_ref().unwrap();

        // ..
    });
}

With today's API:

// you don't have the RefCell + Option overhead but you still have global visibility

fn main() {
    loop {
        interrupt::free(|cs| {
            let gpioa = GPIOA.borrow(cs);

            // ..
        });

        // ..
    }
}

interrupt!(EXTI0, exti0);

fn exit0() {
    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        // ..
    });
}

Compare this to RTFM + this RFC:

app! {
    resources: {
         static GPIOA: GPIOA;
    },

    idle: {
        resources: [GPIOA],
    },

    tasks: {
        EXTI0: {
            path: exti0,
            resources: [GPIOA],
        },
    },
}

fn init(p: init::Peripherals) -> init::LateResourceValues {
    init::LateResourceValues {
        GPIOA: p.GPIOA,
    }
}

fn idle(t: &mut Threshold, r: idle::Resources) -> ! {
    loop {
        r.GPIOA.claim(threshold, |gpioa| {
            // do stuff with `r.GPIOA`
        });

        // ..
    }
}

fn exti0(r: EXTI0::Resources) {
     // do stuff with `r.GPIOA`
}

No Mutex, no RefCell, no Option and no global visibility. Plus only idle needs a lock to
access GPIOA. Without this RFC, even with RTFM, it's possible to access GPIOA from
an execution context that's not idle or exti0 due to this issue / bug / hole:
japaric/cortex-m-rtfm#13.

It breaks the world

Breaking changes in cortex-m, svd2rust and cortex-m-rtfm are required to implement this.

Unresolved questions

• Should we have finer grained access to peripherals as in a GPIOA::take() that returns
  Option<GPIOA> (in addition to Peripherals::all() -- one would invalidate the other)?

• The unsafe variant, Peripherals::_all, needs a better name.

Implementation bits

Note that the implementation is a bit crude at this point. Expect bugs. Still, I'm posting now to
get feedback and to allow others to experiment.

• The f3 repo contains a high level API, and examples that use it, based on this RFC.
• turn peripherals into scoped singletons cortex-m#65
• Peripherals as scoped singletons #158
• japaric/cortex-m-rtfm#50

cc @pftbest @thejpster @therealprof @hannobraun @nagisa @kjetilkjeka

---

EDIT: I realized that it may not be obvious how the RFC solves problem (a) so here's an example:

Let's say that you want to use a serial port in idle and in some interrupt handler. You know that
idle only needs to use the transmitter half the serial port and the interrupt handler only needs
to use the receiver part.

app! {
    resources: {
        static TX: Tx;
        static RX: Rx;
    },

    idle: {
        resources: [TX],
    },

    tasks: {
        EXTI0: {
            path: exti0,
            resources: [RX],
        },
    },
}

fn init(p: init::Peripherals) -> init::LateResourceValues {
    // consumes `GPIOA`
    let pins = p.GPIOA.pins();

    // consumes `PA9`
    let pa9 = pins.PA9.as_usart1_tx();

    // consumes `PA10`
    let pa10 = pins.PA10.as_usart1_rx();

    // consumes `USART1`, `pa9` and `pa10`
    let serial: Serial = Serial::new(p.USART1, (pa9, pa10), 115_200.bps());

    // consumes `serial`
    let (tx, rx): (Tx, Rx) = serial.split();

    // initializes the resources `TX` and `RX`
    init::LateResourceValues {
        TX: tx,
        RX: rx,
    }
}

fn idle(_t: &mut Threshold, r: idle::Resources) -> ! {
    let tx: &mut Tx = r.TX;

    loop {
        // do stuff with `tx`
    }
}

fn exti0(_t: &mut Threshold, r: EXTI0::Resources) {
    let rx: &mut Rx = r.RX;

    // do stuff with `rx`
}

With all the moves in init you are sure that:

• No task or crate (dependency) can reconfigure the pins through GPIOA, because it (GPIOA) was
  created (cf. the init::Peripherals argument of init) and consumed in init.

• No task or crate (dependency) can reconfigure or drive the pins PA9 and PA10, because both
  pins were created and consumed in init.

• No task or crate (dependency) can do serial I/O through USART1 because it (USART1) was created
  and consumed in init.

• No task or crate (dependency) other than idle can do serial writes because only idle has
  access to Tx.

• No task or crate (dependency) other than exti0 can do serial reads because only exti0 has
  access to Rx.

Without this RFC you can't have any of these guarantees because the svd2rust API lets you use
GPIOA, USART1 in any execution context so even a function foo with signature fn() (note:
no arguments) can reconfigure the pins or start a serial operation.

[jonas-schievink]

This is very nice! It fixes my main (and basically only) complaint about rtfm - the need to reconstruct peripherals in every task. Sadly, I haven't had any time to do more with rtfm, but I'll definitely check this out in more depth when I get the time.

[therealprof]

To transition the object from a state to another it needs to be moved so that the previous state
can no longer be used.

@japaric How do I "move" a peripheral? For some applications it is very important to being able to switch GPIO pins from input to output and back on-the-fly.

I do like the idea of the peripheral proxy quite a bit but I'm absolutely not hot about this:

static GPIOA: Mutex<RefCell<Option>> = Mutex::new(RefCell::new(None));

It would be great if there was a simple way to get an abstracted static peripheral proxy object (same for other fixed memory blocks as scratch space, but that is a different topic) as an alternative to obtaining the singleton.

[japaric]

How do I "move" a peripheral?

You write a method that takes self, not &self or &mut self, and that would be a move. But
maybe that's not the answer you wanted?.

For some applications it is very important to being able to switch GPIO pins from input to output and back on-the-fly.

You can continue use type state in straight line code to transition from input mode to output mode.
You just need an API to go from input to unconfigured and from output to unconfigured (the f3 repo
already has this, I think nvm I didn't check in that):

fn foo(pa0: PA0) -> PA0 {
    // `as_input` has signature `fn(self) -> Input<Self>`
    let input: Input<PA0> = pa0.as_input();

    // do input stuff

    // `unwrap` has signature `fn(self) -> PA0`
    let pa0 = input.unwrap();

    // `as_output` has signature `fn(self) -> Output<Self>`
    let output: Output<PA0> = pa0.as_output();

    // do output stuff

    output.unwrap()
}

Alternatively you could create an Io abstraction that automatically shifts from one mode to the
other:

enum Mode {
    Input,
    Output,
}

struct Io<P> {
    pin: P,
    mode: Mode,
}

let io: Io<PA0> = pa0.as_io(Mode::Input);

if io.is_high() {
     // ..

     // automatically changes the mode to `Output`
     io.set_low();

     // ..
}

If your pin is stored in a static variable and you still want to use type state then you'll have
to do a bit of "Option dancing" to get things to work:

// `pa0` is stored in a `static` variable (resource) so it can *not* be moved out.
// At best you can get a `&mut` reference to the `static`.
fn bar(resource: &mut Option<PA>) {
    // start of the dance
    if let Some(pa0) = resource.take() {
        // the definition of `foo` is above
        let pa0 = foo(pa0);

        // end of the dance
        *resource = Some(pa0);
    } else {
        // unreachable because the `resource` is always in the `Some` variant
        // we still need to `Option` to temporarily move `PA0` out of the resource
    }
}

It would be great if there was a simple way to get ..

There is: you use RTFM.

[japaric]

I have appended an example of how this proposal solves problem (a) to the RFC text (see the issue description).

I forgot to cc the embed-rs people. They have been using move semantics / ownership since long ago so maybe they have feedback on this proposal. cc @oli-obk @phil-opp please see the issue description.

[pftbest]

Do I understand correctly that each peripheral has a global flag to check if it was created or not? Is `pa0` in your last example a zero sized type?

…

On 22 Nov 2017 09:35, "Jorge Aparicio" ***@***.***> wrote: I have appended an example of how this proposal solves problem (a) to the RFC text (see the issue description). I forgot to cc the embed-rs people. They have been using move semantics / ownership since long ago so maybe they have feedback on this proposal. cc @oli-obk <https://github.com/oli-obk> @phil-opp <https://github.com/phil-opp> please see the issue description. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABgB3FHOhC4vMBo4pn-LiBof_GnfXx4Hks5s48CjgaJpZM4QmGtI> .

[oli-obk]

Implementation wise, the differences between your proposal and our impl are:

• No addresses returned by some function on the type, instead we generate a &'static mut SomeRegister from the address. &'static mut is equivalent to ownership semantics, because if you have a function taking a &'static mut, the original is borrowed indefinitely.
  • I think your method compiles down to less code though, should be trivial to convert our stuff to not use &self, but Self::ptr
• When not using RTFM, we still don't need statics, we have an interrupt system that works similar to crossbeam. There's three ways to give data to an interrupt
  • You can borrow data in an interrupt (if it impls Sync)
  • Or you can move it into the interrupt, never to see it again (move || some_data.action())
  • Or you can move it into the interrupt and get it back when you unregister the interrupt
• The interrupt system is initialized via the same ownership semantics that we use for all hardware

Some thoughts

• The ptr method should probably be an associated constant or an #[inline(always)] const fn to guarantee that rustc will optimize it even in debug mode. This will be a very common operation that could seriously penalize debug mode
• We've had issues with device drivers being locked into the board's config, even though technically the driver just wants to get a handle to some pins. Binding the type system more closely to the specific hardware will make this somewhat harder.

[oli-obk]

Do I understand correctly that each peripheral has a global flag to check if it was created or not?

No, there's a single global flag for whether you obtained the handle for the entire hardware. After that, you just zero-cost extract the hardware components that you want.

[japaric]

@pftbest

Do I understand correctly that each peripheral has a global flag to check if it was created or not?

Not in the main proposal. In the main proposal there's a single boolean that guards all the peripherals. I initially implemented this using a per peripheral boolean but didn't see much advantage in doing that so that became the alternative to the main proposal (it's under "unresolved questions" in the RFC text).

Also note that the boolean never gets cleared when the peripheral is dropped. The boolean is not some sort of lock.

(In the f3 implementation there's actually two booleans to guard the peripherals. One for core peripherals and one for device peripherals. It might be possible to just have one boolean but two booleans was easier to implement).

Is pa0 in your last example a zero sized type?

Yes.

@oli-obk

Thanks for the input.

I think your method compiles down to less code though

I think so too but haven't measured. In particular you are guaranteed that static FOO: GPIOA = .. will have a zero size memory representation since everything is a zero sized type. OTOH, with your approach USART{1,2,3} can all have the same type &'static mut USART whereas we would have to use traits to abstract over the different USART types.

When not using RTFM, we still don't need statics, we have an interrupt system that works similar to crossbeam.

I have yet to look at your interrupt system but I recall that you mentioned during rustfest that your approach doesn't support priority levels (all the interrupts had the same priority). Is that still the case?

I'm not 100% sure that reconfigurable interrupt handlers are compatible with RTFM (SRP); RTFM wants everything to be statically (compile time) defined.

The ptr method should probably be ...

Our code is already very bloated in debug mode due to the svd2rust API so I don't think this would help much 😅.

We've had issues with device drivers being locked into the board's config

I don't think I fully understand what's the issue here. Could you point me to an example?

[hannobraun]

I'm 100% on board with this concept. I'm currently working on a HAL for LPC82x, and what I'm doing is very much in line with what you're suggesting here, with the caveat that I the user must promise not to access the peripheral directly. This RFC would be a huge improvement for me.

Thank you, @japaric, this is great work!

I have a question about this piece of code from the original issue:

pub struct GPIOA { _marker: PhantomData<*const ()> }

Why the PhantomData field? If the purpose of the field is prevent construction of the struct from outside the module, a simple () should suffice. What am I missing?

Here the Input and Output newtypes are used to encode the type state.

Have you considered using type parameters instead of newtypes to encode the type state? Something along those lines:

struct PA0<State> {
    _state: State,
}

impl<State> PA0<State> {
    fn as_output(self) -> PA0<Output> { ... }
}

impl PA0<Output> {
    fn set_high(&mut self) { ... }
}

I don't know if that's a better solution in this specific case, but in general, I see the following advantages over newtypes:

• Methods that are available in every state are straight-forward to implement.
• Scales well to multiple types of state. Implementing methods that are available for any combination of states is straight-forward.
• You can use a default value for the type parameter to hide it, and make working with the API more convenient. This doesn't make much sense with pin input/output, but is nice if the type state encodes whether something is initialized or not (as depending on a type without qualification would always refer to the initialized version).

RTFM supports moving runtime initialized resources into tasks (interrupts) at zero, or very little,
cost but if you are not using RTFM then a static variable is required to share a peripheral
between e.g. main and an interrupt handler. Putting a peripheral singleton in a static variable
means making it globally visible which means you have global singletons again, and all their
disadvantages, but with worse performance (because an Option and a RefCell are needed, see
below) than what you get with today's API.

How does putting a peripheral in a static variable imply that the peripheral becomes globally visible? The static variable can live in a module and not be pub. Then you can use free or associated functions in that module to provide a limited API. You might not even need all that Mutex/RefCell business, if whatever your API does, doesn't happen to require it.

Breaking changes in cortex-m, svd2rust and cortex-m-rtfm are required to implement this.

I don't care. This is enough of an improvement to be worth the breaking changes.

Should we have finer grained access to peripherals as in a GPIOA::take() that returns
Option<GPIOA> (in addition to Peripherals::all() -- one would invalidate the other)?

I don't know the answer to this question, but at least I never had the need for this kind of fine-grained access.

The unsafe variant, Peripherals::_all, needs a better name.

Maybe all_unsafe? Or maybe Peripherals::take for the safe variant and force_take for the unsafe one?

I don't feel strongly about any of those, just throwing around some suggestions.

Note that the implementation is a bit crude at this point. Expect bugs. Still, I'm posting now to
get feedback and to allow others to experiment.

I haven't looked at any of the pull requests so far, and I'm busy enough right now that I might not get to it for the foreseeable futures. However, feel free to ping me if you need feedback on something. I'll gladly make time.

[oli-obk]

If the purpose of the field is prevent construction of the struct from outside the module, a simple () should suffice. What am I missing?

This clears all OIBITS like Send and Sync

I have yet to look at your interrupt system but I recall that you mentioned during rustfest that your approach doesn't support priority levels (all the interrupts had the same priority). Is that still the case?

Our mutexes don't support priority levels, so all critical sections block all interrupts. This can cause priority inversion. We just haven't gotten around to implementing Mutexes with priority level support into the interrupt system.

I don't think I fully understand what's the issue here. Could you point me to an example?

Take the simple example of a "Button driver" that's nothing more than a debouncer. We need to implement it to take a specific GPIO pin instead of just "something that we can read a bit from".

[hannobraun]

If the purpose of the field is prevent construction of the struct from outside the module, a simple () should suffice. What am I missing?

This clears all OIBITS like Send and Sync

Ah, makes sense. Thank you!

[japaric]

@hannobraun

Thanks for taking the time to comment.

Have you considered using type parameters instead ..

Short answer: no. But I'll explore that option at some point.

In general, you shouldn't take the examples in the RFC as "the right way to do it" but rather as a
"one way to do it" or "just to showcase that this is possible at all".

I'm using newtypes instead of the more standard Foo<State> because I originally thought of putting
the pin proxies and the pins() method in the device crate and that coherence might be a problem
(can't impl YourType<MyType>). But then I realized that since SVD files target device families
and that one size fits all is not appropriate for pins: for example, you want GPIOC to be split into
PC1{3,4,5} if the device has a few pins and into PC{0,...,15} if the device has many pins so you
need a different Pins struct per device.

How does putting a peripheral in a static variable imply that the peripheral becomes globally
visible? The static variable can live in a module and not be pub.

Right, it may not be global in the literal sense but it's still "global" to the module where it's
defined and any function can access the static without declaring that it will do so (i.e. the
static doesn't have to appear as an argument of the function). To me allowing this undeclared
dependency is as bad as global visibility.

You might not even need all that Mutex/RefCell business, if whatever your API does, doesn't happen
to require it.

If you are OK with using unsafe code / static mut then you can skip the Mutex / RefCell /
Option. But, with the current proposal, it's always the end user who has to "create" the singletons
so if you want to put a singleton in a static variable the end user would have to do it.

@oli-obk

Thanks for the pointers. I'll take a closer look at that example and the interrupt system you are
using during the week.

[therealprof]

@japaric Thanks for the explanation, seems like all bases are covered.

[hannobraun]

@japaric

In general, you shouldn't take the examples in the RFC as "the right way to do it" but rather as a
"one way to do it" or "just to showcase that this is possible at all".

Thanks, I realize that. I just wanted to make sure you're aware of the possibility, but I guess that whole discussion was a bit off-topic from the beginning. I think we all agree that this RFC allows us to build better abstractions. The details of how those abstractions will look like exactly are probably not that relevant at this point.

How does putting a peripheral in a static variable imply that the peripheral becomes globally visible? The static variable can live in a module and not be pub.

Right, it may not be global in the literal sense but it's still "global" to the module where it's
defined and any function can access the static without declaring that it will do so (i.e. the
static doesn't have to appear as an argument of the function). To me allowing this undeclared
dependency is as bad as global visibility.

I think we agree that this situation is not ideal, but I strongly disagree that it is "just as bad as global visibility". It might be close to as bad, if it's a huge module. But if only 2-3 functions have access to that static? I think that's acceptable.

If you are OK with using unsafe code / static mut then you can skip the Mutex / RefCell /
Option. But, with the current proposal, it's always the end user who has to "create" the singletons
so if you want to put a singleton in a static variable the end user would have to do it.

Makes sense. In any case, I think the trade-off is worth it.

[thejpster]

This looks like a really nice improvement. I vote for move fast and break things.

[japaric]

OK. There seems to consensus on this RFC so I'll land the pieces in all the repos in the coming days.

There are a few implementation details to iron out:

• Decide on better names for Peripherals::{all,_all}. If anyone has a suggestion please comment on the svd2rust PR (the link to the PR is in this issue description).

• Figure out whether we can have a single PERIPHERALS static boolean for both core peripherals and device peripherals. Right now the implementation uses two statics (which results in two peripheral sets so you end with p.core.ITM and p.device.TIM3 in application code) because that was easier to implement, but I think it should possible to only have a single static (and a single set of peripherals).

• This requires a new minor release of cortex-m-rtfm. I'd like to land the "safe &'static mut references" feature and drop the static_ref::Static wrapper before making a new minor release. The first change supersedes the static_ref::Static abstraction (which I think is still memory unsafe) and it's required to have memory safe DMA transfer APIs. The original RFC for the &'static mut feature is japaric/cortex-m-rtfm#47 but I think it's safer to go with the implementation in japaric/cortex-m-rtfm#50. I'll write a new RFC to discuss the trade-offs.

Finally I have been sketching APIs for more peripherals in this branch (clock configuration, pwm, capture, DMA, etc.) of the blue-pill crate.

[therealprof]

@japaric I've played around a bit with the singletons and it simplifies things quite a bit. I am a bit puzzled about the _all() function. You said it can only be called once but you seem to use debug_assert!() which means I can call it just fine multiple times since I'm using optimised builds only which seems a bit counter intuitive (code that works fine when optimised but breaks in debug mode). Is there a reason for that and wouldn't there be a better way to check this at compile-time rather than run-time?

[kjetilkjeka]

This looks like a nice improvement that will be very helpful today and to what I expect the long term embedded rust will be like. In case my heart emoji didn't express my feelings strongly enough, I'm all for breaking all kinds of things to land this!

I've been totally swamped with work the last week and probably will be the next week as well. I will take some time looking at the details as soon as i can.

[hannobraun]

@japaric

Figure out whether we can have a single PERIPHERALS static boolean for both core peripherals and device peripherals. Right now the implementation uses two statics (which results in two peripheral sets so you end with p.core.ITM and p.device.TIM3 in application code) because that was easier to implement, but I think it should possible to only have a single static (and a single set of peripherals).

For what it's worth, I don't mind having two peripheral sets. Actually, I think it might be a bit nicer, as it makes it more obvious how platform-dependent a given piece of code is.

If I remember correctly, the flags might be optimized away anyway. If that's true, I don't really see a reason to try to unify them, if that makes things more complicated.

Finally I have been sketching APIs for more peripherals in this branch (clock configuration, pwm, capture, DMA, etc.) of the blue-pill crate.

FYI, I've been working on APIs for clock stuff to, in my LPC82x HAL (which I've published this morning). There are also the beginnings of a generic interface in there, which I plan to submit to embedded-hal once it has matured a little.

@therealprof

I am a bit puzzled about the _all() function. You said it can only be called once but you seem to use debug_assert!() which means I can call it just fine multiple times since I'm using optimised builds only which seems a bit counter intuitive (code that works fine when optimised but breaks in debug mode). Is there a reason for that and wouldn't there be a better way to check this at compile-time rather than run-time?

I haven't looked at the code yet, but I supect the point is to minimize overhead in release mode, while making mistakes obvious in debug mode. This has precedent in Rust itself, as integer overflow will cause a panic in debug mode and do nothing in release mode, for the same reason.

It would be great if there was a way to guarantee at compile-time that the method if only called once. However, I don't know how we could do that reliable. My guess is that it's currently not practical.

[oli-obk]

It would be great if there was a way to guarantee at compile-time that the method if only called once. However, I don't know how we could do that reliable. My guess is that it's currently not practical.

That's actually easy to do as a clippy lint. You strap a #[clippy(once)] attribute on a function, and clippy guarantees that all #[clippy(once)] functions are only ever called once. There will be false positives where we'll tell you we can't prove it's only called once. E.g. if someone makes a function pointer out of the function, we won't be able to prove that noone's duplicating the pointer. So essentially #[clippy(once)] requires that the function is never turned into a function pointer and any function calling the function is also #[clippy(once)].

Open an issue on the clippy repo if you consider this solution practical

[therealprof]

@hannobraun

I haven't looked at the code yet, but I supect the point is to minimize overhead in release mode, while making mistakes obvious in debug mode. This has precedent in Rust itself, as integer overflow will cause a panic in debug mode and do nothing in release mode, for the same reason.

Well, with embedded projects debug code might not always fit in the flash so sometimes it's simply practical to use it. I never use debug code so I'm not too happy about catching this "problem" only in debug versions. Also if this is called so often that it would become a performance problem, you're very much doing it wrong anyway since this is only supposed to be called once so it shouldn't be a problem, no? ;)

[therealprof]

@oli-obk

That's actually easy to do as a clippy lint. You strap a #[clippy(once)] attribute on a function, and clippy guarantees that all #[clippy(once)] functions are only ever called once. There will be false positives where we'll tell you we can't prove it's only called once. E.g. if someone makes a function pointer out of the function, we won't be able to prove that noone's duplicating the pointer. So essentially #[clippy(once)] requires that the function is never turned into a function pointer and any function calling the function is also #[clippy(once)].

This is very cool, I like it. The only problem I see in this particular case is that code generated by svd2rust is very clippy unfriendly and requires a lot of adjusting and/or test gating to be usable at all. :(

[oli-obk]

The only problem I see in this particular case is that code generated by svd2rust is very clippy unfriendly and requires a lot of adjusting and/or test gating to be usable at all. :(

You won't be using it on svd2rust, you'd use it on your crate, and clippy would have to transitively prove the correctness of #[clippy(once)] stuff.

Also you can just turn off all lints except for the once lint ;)

[therealprof]

@oli-obk My own crate contains clippy unfriendly svd2rust generated code. I know I can do some stuff selectively in clippy, but I haven't figured out how to get rid of the specific warnings in the svd2rust code via configuration. Clippy is an incredible tool which I'm using extensively for any projects but the MCU stuff...

[hannobraun]

@oli-obk

That's actually easy to do as a clippy lint.

I didn't know this. This is awesome. Thanks!

Open an issue on the clippy repo if you consider this solution practical

I presume this is missing "don't"? Otherwise it's confusing :-)

@therealprof

Well, with embedded projects debug code might not always fit in the flash so sometimes it's simply practical to use it. I never use debug code so I'm not too happy about catching this "problem" only in debug versions.

You're right, but you don't have to do a full-blown debug build to get debug assertions. You can enable them separately. See Cargo documentation.

Also if this is called so often that it would become a performance problem, you're very much doing it wrong anyway since this is only supposed to be called once so it shouldn't be a problem, no? ;)

I don't think performance is the issue, but code size could be. Granted, a single assert! likely won't make a difference, but if we as a community decide that assert!s in release code are okay, this will quickly add up to the point where using Rust on very small microcontrollers will become very hard.

[oli-obk]

I presume this is missing "don't"? Otherwise it's confusing :-)

No. We don't have this lint. Open an issue if you want this lint implemented.

[therealprof]

@hannobraun I'm in general no big fan of panic! in MCU code but since this door was already opened and work is under way to improve it, we might as well use it for assert!s, too. I'd still prefer to have a compile time check and/or a lint for this.

[hannobraun]

@oli-obk Oh, I just remembered why I said I earlier, that I believe static checking might not be practical: The dependency graph could contain multiple versions of the same crate. Do you know how that affects the Clippy lint?

This might not be a big issue in pracice though. (And now that I think about it, unless the statics are #[no_mangle], which they might be, I don't know, a debug_assert! won't catch that case either. At least I don't think.)

Edit: Please disregard. While I wrote this, the latest answer by @oli-obk made this irrelevant.

[hannobraun]

@oli-obk

No. We don't have this lint. Open an issue if you want this lint implemented.

Oh, good thing I asked then. I've added it to my task list, so I can look into it later.

[nagisa]

The dependency graph could contain multiple versions of the same crate. Do you know how that affects the Clippy lint?

I believe multiple crates defining the same peripherals would result in the design laid out in this RFC to provide the guarantees it claims to provide. i.e. One would be able to run Peripherals::all() from one version of crate and Peripherals::all() from the other, essentially ending up in you having two sets of peripherals.

I feel like it would be desirable to make it impossible to link multiple versions of the crate defining peripherals in case this is implemented. (A simple way to do so would be to define a global symbol with a well-known name)

[japaric]

@nagisa

I feel like it would be desirable to make it impossible to link multiple versions of the crate defining peripherals in case this is implemented.

Yes, this what brought up in the pre-RFC (#151). The proposed solution was to add a #[no_mangle] to the static to make different minor versions of a device crate un-link-able. I had forgotten about it :-); I'll roll it later.

You can still break singletons if some implements an alternative singleton system for peripherals that uses different names for the static variables.

@therealprof

_all is unsafe. It doesn't matter if it has the debug_assert! or not -- that assert doesn't make the function safe. It's common, or I think it is, to check the preconditions of unsafe functions in dev mode to try to catch bugs.

@oli-obk

Can clippy trace the call graph then? Can it handle independent call graphs (an interrupt handler is the root of an independent call graph)? Could it get all these:

// defined in some other crate
fn call_me_once() { /* .. */ }

fn main() {
    call_me_once(); // assume this is the only intended call

    call_me_once(); // easy to catch

    foo(); // less (?) easy to catch
}

// could be defined in some other crate
fn foo() {
    call_me_once();
}

// could be defined in some other crate
// dead code
fn bar() {
    call_me_once();  // false positive
}

// could be defined in some other crate
// interrupt handler
#[no_mangle]
pub fn EXTI0() {
    call_me_once(); // hard to catch?
}

[oli-obk]

Could it get all these:

all the other functions would be required to also have #[clippy(once)], it's contageous.

For the interrupt handler, I'd just assume #[no_mangle] functions would also be forbidden to contain calls to #[clippy(once)] functions. We'll err on the safe side and forbid all calls that we can't trivially prove.

[therealprof]

@japaric

_all is unsafe. It doesn't matter if it has the debug_assert! or not -- that assert doesn't make the function safe. It's common, or I think it is, to check the preconditions of unsafe functions in dev mode to try to catch bugs.

That sounds rather weird to me: Either this tries to catch abuse or it doesn't, just doing this under certain circumstances sounds like a recipe for disaster.

As I said, I'm not at all a fan of panic!ing MCU code at all and I do think that keeping this as an unguarded unsafe function is perfectly fine.

[nagisa]

@japaric one alternative would be to use linkonce_odr linkage, but not sure how supported between linkers it is.

[japaric]

@oli-obk

all the other functions would be required to also have #[clippy(once)], it's contageous.

I see. You get a warning if a clippy(once) function is called from a function not marked as clippy(once). I think that might be enough for RTFM.

EDIT: enough for a sanity check. Since the lint won't be a part of the compiler we can't rely on it for safety (i.e. turn an unguarded unsafe function into a safe function).

@therealprof

If you, as an end user, want to construct Peripherals you should use the safe, guarded alls constructor. The unsafe, unguarded _alls constructor is not meant to be used by an end user but to cosntruct safe abstractions (e.g. rtfm::init) that the end user can then use.

[therealprof]

@japaric That is fully understood.

It is still insane if you develop code with release type builds (like I always do for MCUs for a number of reasons, e.g. because I like looking (and comparing) at the (dis-)assembly) and call _all() more than once and then someone else uses the code by accident (because it's the default build type) or on purpose with a debug build and all of a sudden it goes boom...

If it is enforced it should be always and properly enforced so there's no possible way to miss it. However to me it looks like just documenting why it is unsafe and removing the debug_assert! would be a better way to go.

[japaric]

All the pieces required to implement this RFC have landed in the master branches of cortex-m, cortex-m-rtfm and svd2rust. 🎉

However, there are quite a few things to do before releasing a new minor version of those crates (e.g. updating the documentation). I have created a milestone for each repo to track the things to do. If you think anything else should be done before the next minor version release now's your chance: open an issue or leave a comment on an existing issue requesting adding it to the milestone.

This requires a new minor release of cortex-m-rtfm. I'd like to land the "safe &'static mut references" feature and drop the static_ref::Static wrapper before making a new minor release.

BTW, the RFC for that change is in japaric/cortex-m-rtfm#59 if you have any thoughts / comments about it.

I have run out of embedded Rust time for this week; I'll be back (late) next week.

Thanks again everyone for your input.