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

[RFC] "rooting" memory (&'static mut- references in init) #47

Closed
japaric opened this issue Sep 27, 2017 · 4 comments
Closed

[RFC] "rooting" memory (&'static mut- references in init) #47

japaric opened this issue Sep 27, 2017 · 4 comments

Comments

@japaric
Copy link
Collaborator

japaric commented Sep 27, 2017

Background

I was revisiting the original use case that @pftbest once told me about -- a lock free single
producer single consumer ring buffer -- and that led me to submit #37 and noticed that RTFM still
can't fulfill that use case in 100% safe code. (I hope I'm wrong)

Below is a program that explains the use case:

#![deny(warnings)]
#![feature(proc_macro)]
#![no_std]

extern crate blue_pill;
extern crate cortex_m_rtfm as rtfm;
// https://github.com/japaric/spscrb
extern crate spscrb;

use blue_pill::stm32f103xx::{self, Interrupt};
use rtfm::{app, Threshold};
use spscrb::{Consumer, Producer, RingBuffer};

app! {
    device: stm32f103xx,

    resources: {
        static PRODUCER: Producer<u32, [u32; 32]>;
        static CONSUMER: Consumer<u32, [u32; 32]>;
    },

    tasks: {
        EXTI0: {
            path: exti0,
            resources: [PRODUCER],
            priority: 1,
        },

        EXTI1: {
            path: exti1,
            resources: [CONSUMER],
            priority: 2,
        },
    },
}

fn init(_p: init::Peripherals) -> init::LateResourceValues {
    let rb: &'static mut RingBuffer<u32, [u32; 32]>;

    // this part needs to be hidden from the user, or rather the user shouldn't need to do this
    rb = {
        static mut RB: RingBuffer<u32, [u32; 32]> = RingBuffer::new();
        unsafe { &mut RB }
    };

    // NOTE this method *consumes* a `&'static mut` reference
    let (p, c) = rb.spsc();
    // let (p2, c2) = rb.spsc();
    // ^ this would be an error

    init::LateResourceValues { PRODUCER: p, CONSUMER: c }
}

fn idle() -> ! {
    rtfm::set_pending(Interrupt::EXTI0);
    rtfm::set_pending(Interrupt::EXTI1);

    loop {
        rtfm::wfi();
    }
}

fn exti0(_t: &mut Threshold, r: EXTI0::Resources) {
    // lock-free operation
    r.PRODUCER.enqueue(0xdead_beef).unwrap();
}

fn exti1(_t: &mut Threshold, r: EXTI1::Resources) {
    // lock-free operation
    r.CONSUMER.dequeue().unwrap();
}

Basically you have a statically allocated ring buffer which is hidden from the user. To enqueue and
dequeue elements into the buffer you have to use the "producer" and "consumer" end points,
respectively. These end points can be used from different execution contexts, which can run at
different priorities, in a lock free manner. This is memory safe, without locking, because the
producer and the consumer, each, own different cursors into the ring buffer and because the cursors
are word sized (they can be atomically read) -- check the implementation for details.

To construct the producer and consumer there's this spsc method with signature fn(&'static mut RingBuffer<_, _>) -> ... This signature ensures that (a) the ring buffer is statically allocated,
and thus it will never be destroyed, and (b) that once you have called this method the reference to
the ring buffer becomes invalidated (required to avoid mutable aliasing).

Thanks to #43 (thanks @jonas-schievink!) we can defer initialization of resources and initialize the
producer and consumer end points in init (see example above). However, one problem remains:
there's no way to safely get / create a &'static mut RingBuffer<_, _> in init. The only place
where &'static mut - references are available right now is in idle but that's too late for
initialization of resources (without wrapping them in Option).

Here's my proposal for solving this:

Proposal

Add a roots field to app!. This field contains a list of static variables with initial values.
The syntax of its contents is similar to the contents of resources.

app! {
    roots: {
        static RB: RingBuffer<u32, [u32; 32]> = RingBuffer::new();
    },
}

The user will get a list of &'static mut- references to these roots in init:

// auto-generated: struct Roots { RB: &'static mut RingBuffer<u32, [u32; 32]>, }
fn init(roots: init::Roots, ..) {
    let (p, c) = roots.RB.spsc();
    // ..
}

Implementation

This can be implemented by creating a static mut variable for each root.

// auto-generated
fn main() {
    interrupt::free(|_| {
        static mut RB: RingBuffer<u32, [u32; 32]> = RingBuffer::new();

        init(init::Roots { RB: &mut RB }, ..);
    });
}

Alternatively, the roots could be allocated in the lowest / first stack frame (hence "rooting" in
the proposal name), which is known to never be deallocated: (I haven't tested if this works)

// auto-generated
fn main() {
    interrupt::free(|_| {
        let mut RB: RingBuffer<u32, [u32; 32]> = RingBuffer::new();
        let roots = init::Roots { RB: &mut *(&mut RB as *mut _) };
        mem::forget(RB); // ???

        init(roots, ..);

        // ..
    });

    // idle is a divergent function
    idle();
}

Other use cases

Another use case for having &'static mut- references in init is being able to set up periodic
DMA transfers in init. An example of this use case is setting up a circular DMA transfer that
reads ADC or Serial data:

app! {
    roots: {
        static BUFFER: [[u16; 64]; 2] = [[0; 64]; 2];
    },

    resources: {
        // cf. japaric/embedded-hal#14 and the blue-pill `dma` module
        static BUFFER: dma::CircBuffer<[u16; 64]>;
    }
}

fn init(roots: init::Roots, ..) -> init::LateResourceValues {
    let buffer = adc1.start(dma1, roots.BUFFER);

    init::LateResourceValues {
        BUFFER: buffer,
    }
}

Thoughts? Can these use cases be safely achieved through some other means?

japaric added a commit that referenced this issue Nov 9, 2017
This implements the "rooting" mechanism proposed in #47. However, it implements a `root` constructor
function instead of list of `roots` values as originally proposed.

In a nutshell:

- There's a new field, `root`, which takes a path to the "root" constructor function.
- This constructor has signature `fn() -> T`
- When the `root` field is used the signature of `init` changes to accommodate a `&'static mut T`
  argument at the end. The `T` in that argument type matches the type returned by the "root"
  constructor.
- The "root"-ed value is stack allocated.

This enables the safe creation of `&'static mut` references. Example below:

``` rust
//#![feature(proc_macro)]
//#![no_std]

extern crate blue_pill;
extern crate cortex_m_rt;
extern crate cortex_m_rtfm as rtfm;
extern crate heapless;

use blue_pill::stm32f103xx;
use heapless::RingBuffer;
use heapless::ring_buffer::{Consumer, Producer};
use rtfm::{app, Threshold};
use stm32f103xx::Interrupt;

app! {
    device: stm32f103xx,

    resources: {
        static CONSUMER: Consumer<'static, u32, [u32; 8]>;
        static PRODUCER: Producer<'static, u32, [u32; 8]>;
    },

    root: root,

    idle: {
        resources: [CONSUMER],
    },

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

struct Root {
    rb: RingBuffer<u32, [u32; 8]>,
}

fn root() -> Root {
    Root {
        rb: RingBuffer::new(),
    }
}

fn init(_p: init::Peripherals, root: &'static mut Root) -> init::LateResourceValues {
    let (p, c) = root.rb.split();

    init::LateResourceValues {
        CONSUMER: c,
        PRODUCER: p,
    }
}

fn idle(_t: &mut Threshold, r: idle::Resources) -> ! {
    rtfm::set_pending(Interrupt::EXTI0);

    loop {
        if r.CONSUMER.dequeue().is_some() {
            rtfm::bkpt();
        } else {
            rtfm::wfi();
        }
    }
}

fn exti0(_t: &mut Threshold, r: EXTI0::Resources) {
    r.PRODUCER.enqueue(42).ok();

    rtfm::bkpt();
}
```

This produces the following machine code:

``` armasm
0800019c <EXTI0>:
 800019c:       f240 0000       movw    r0, #0
 80001a0:       f2c2 0000       movt    r0, #8192       ; 0x2000
 80001a4:       6800            ldr     r0, [r0, #0]
 80001a6:       6803            ldr     r3, [r0, #0]
 80001a8:       6842            ldr     r2, [r0, #4]
 80001aa:       1c51            adds    r1, r2, #1
 80001ac:       f001 0107       and.w   r1, r1, #7
 80001b0:       4299            cmp     r1, r3
 80001b2:       d006            beq.n   80001c2 <EXTI0+0x26>
 80001b4:       eb00 0282       add.w   r2, r0, r2, lsl #2
 80001b8:       232a            movs    r3, #42 ; 0x2a
 80001ba:       6093            str     r3, [r2, #8]
 80001bc:       f3bf 8f5f       dmb     sy
 80001c0:       6041            str     r1, [r0, #4]
 80001c2:       be00            bkpt    0x0000
 80001c4:       4770            bx      lr

080001c6 <main>:
 80001c6:       b08a            sub     sp, #40 ; 0x28  ; Root allocation
 80001c8:       f240 1030       movw    r0, #304        ; 0x130
 80001cc:       4669            mov     r1, sp
 80001ce:       22f0            movs    r2, #240        ; 0xf0
 80001d0:       f6c0 0000       movt    r0, #2048       ; 0x800
 80001d4:       7800            ldrb    r0, [r0, #0]
 80001d6:       2000            movs    r0, #0
 80001d8:       e9cd 0000       strd    r0, r0, [sp]
 80001dc:       f240 0000       movw    r0, #0
 80001e0:       f2c2 0000       movt    r0, #8192       ; 0x2000
 80001e4:       b672            cpsid   i
 80001e6:       6001            str     r1, [r0, #0]    ; PRODUCER = ..
 80001e8:       f240 0004       movw    r0, #4
 80001ec:       f2c2 0000       movt    r0, #8192       ; 0x2000
 80001f0:       6001            str     r1, [r0, #0]    ; CONSUMER = ..
 80001f2:       f24e 4106       movw    r1, #58374      ; 0xe406
 80001f6:       f2ce 0100       movt    r1, #57344      ; 0xe000
 80001fa:       700a            strb    r2, [r1, #0]
 80001fc:       f24e 1100       movw    r1, #57600      ; 0xe100
 8000200:       2240            movs    r2, #64 ; 0x40
 8000202:       f2ce 0100       movt    r1, #57344      ; 0xe000
 8000206:       600a            str     r2, [r1, #0]
 8000208:       b662            cpsie   i
 800020a:       f8c1 2100       str.w   r2, [r1, #256]  ; 0x100
 800020e:       e006            b.n     800021e <main+0x58>
 8000210:       f3bf 8f5f       dmb     sy
 8000214:       3201            adds    r2, #1
 8000216:       f002 0207       and.w   r2, r2, #7
 800021a:       600a            str     r2, [r1, #0]
 800021c:       be00            bkpt    0x0000
 800021e:       6801            ldr     r1, [r0, #0]
 8000220:       684b            ldr     r3, [r1, #4]
 8000222:       680a            ldr     r2, [r1, #0]
 8000224:       429a            cmp     r2, r3
 8000226:       d1f3            bne.n   8000210 <main+0x4a>
 8000228:       bf30            wfi
 800022a:       e7f8            b.n     800021e <main+0x58>
```

Unresolved questions:

- Is this mechanism memory safe in presence of `panic!` unwinding?
  - If not, can we generate a compile error if `panic = abort` is *not* used?
- How does this affect the DMA API proposed in rust-embedded/embedded-hal#14

cc @pftbest
@therealprof
Copy link

Not sure what rooting means but effectively you're creating a global shared state but rather than letting it hang freely in the breeze you're attaching it to a context which is nice. There're many use-cases for this and it'd be really great if a sane method was also available in cortex-rt. I fully understand why static mut is unsafe and discouraged in regular applications but in case of MCUs the case is a little different since due to interrupts there's really no single point of control where you could pass the necessary handles from.

The only halfway sane way I can handle this it at the moment is something like this:

pub struct PWMCache {
    bitmask: [u32; 256],
}

pub fn pwmcache() -> &'static mut PWMCache {
    static mut SINGLETON: PWMCache = PWMCache::new();
    unsafe { &mut SINGLETON }
}

That way one can also control access to the data but it would be nice to have a generic way of declaring global resources (also for cortex-m-rt 😉).

@japaric
Copy link
Collaborator Author

japaric commented Nov 9, 2017

Not sure what rooting means

Ah, it's just that in this proposal I suggest placing the data that will never be freed in one of the first stack frames or near the "root" of the call stack. It may not be the best name though.

effectively you're creating a global shared state but rather than letting it hang freely in the breeze you're attaching it to a context which is nice.

That's not the intention of this RFC though. The goal here is being able to safely create &'static mut references -- it doesn't really matter if they are allocated on the stack or on .bss / .data (static variables).

Safe creation of &'static muts opens a new possibility in APIs because &'static mut is very close to owning the value minus worrying about destructors ever running. Also you can store these references in static variables.

The idea of "scoping" static variables or constraining them to certain execution contexts is a core idea of the RTFM framework (it was one of the hard lessons learned between v1 and v2: global visibility is bad).

There're many use-cases for this and it'd be really great if a sane method was also available in cortex-rt.

But the result would be RTFM, no? If you need to share state between execution contexts then you may need to locking -- whether or where locking is required has to be computed from the priorities and that's what RTFM does. If you don't need sharing then you already have interrupt / exception local variables (which effectively add state to your handler) via the interrupt! and exception! macros of the cortex-m-rt device crate. Or is the problem with interrupt local variables due to const eval (const fn)? If that's the case then some runtime initialization like what's proposed here would make sense in cortex-m-rt / svd2rust.

pub fn pwmcache() -> &'static mut PWMCache {

This is still very dangerous. If you ever call pwmcache more than once then you risk running into UB due to mutable aliasing -- the problem may not be so obvious as calling pwmcache twice within a function foo; you'll also run into problems if foo calls pwmcache and, for example, a function bar calls foo twice.

@therealprof
Copy link

Safe creation of &'static muts opens a new possibility in APIs because &'static mut is very close to owning the value minus worrying about destructors ever running. Also you can store these references in static variables.

Right. ;)

If you need to share state between execution contexts then you may need to locking -- whether or where locking is required has to be computed from the priorities and that's what RTFM does.

No, I don't need any locking; There's exactly one reader and one writer and the storage is a simple array of integral types. The reason I want to share it is that the values are recalculated on much slower cadence (and only after an update of the value they're calculated from) than I need to output them. I could probably use rtfm but that complicates a lot of things without any obvious benefit.

Or is the problem with interrupt local variables due to const eval (const fn)? If that's the case then some runtime initialization like what's proposed here would make sense in cortex-m-rt / svd2rust.

There're many problems I have with that. I find the macros rather unflexible since I can't use them in in my initialisation code, can't do any any conditional (non-constant) initialisation (e.g. a random value) and can't use fancy datatypes e.g. arrays.

This is still very dangerous. If you ever call pwmcache more than once then you risk running into UB due to mutable aliasing -- the problem may not be so obvious as calling pwmcache twice within a function foo; you'll also run into problems if foo calls pwmcache and, for example, a function bar calls foo twice.

I'm afraid I don't understand how that could be a problem since this is really only an abstraction to get rid of the nasty unsafe {}s everywhere in contrast to simply defining a global static mut. The generated code not only looks fine and does exactly what it's supposed to. But if there's a better way to get a fixed chunk of shared RAM I'd be certainly happy to change that. 😉

japaric pushed a commit that referenced this issue Dec 9, 2017
Peripherals as scoped singletons

See this RFC for details: rust-embedded/svd2rust#157

- The first commit adapts this crate to the changes in rust-embedded/cortex-m#65 and rust-embedded/svd2rust#158
- ~~The second commit is an alternative implementation of RFC #47 (there's another implementation in #49. This second commit is not required for RFC157 but let us experiment with safe DMA abstractions.~~ postponed

### TODO

- [x] un-bless peripherals as resources. Peripherals as resources were special cased: if resource listed in e.g. `app.tasks.FOO.resources` didn't appear in `app.resources` then it was assumed to be a peripheral and special code was generated for it. This is no longer required under RFC157.

~~This depends on PR rtic-rs/rtic-syntax#2~~ postponed
@japaric
Copy link
Collaborator Author

japaric commented Dec 9, 2017

Closing in favor of #59

@japaric japaric closed this as completed Dec 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants