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.

Sign up for GitHub

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

Jump to bottom

Determine an idiomatic way of sharing or transferring resources between Interrupt and User contexts #294

Closed
jamesmunns opened this issue Jan 15, 2019 · 41 comments
Closed

Determine an idiomatic way of sharing or transferring resources between Interrupt and User contexts #294

jamesmunns opened this issue Jan 15, 2019 · 41 comments

Comments

@jamesmunns
Copy link
Member

jamesmunns commented Jan 15, 2019 •
edited
Loading

See discussion on this thread: rust-embedded/bare-metal#15

Edit: This may end up being more than one solution, particularly "moving" vs "sharing", and for different levels of "sharing" guarantees".
@jamesmunns jamesmunns mentioned this issue Jan 15, 2019
Closed
@adamgreig
Copy link
Member

I can see at least a few different use cases:

Sharing a variable between the main thread and only one interrupt handler

This is for something like a semaphore the ISR can signal to tell the main thread to perform an action, or some data received by the ISR that it wants to send to the main thread, or the main thread and the ISR sharing a buffer that the main thread fills and the ISR drains. The main thread can never pre-empt the ISR, and since no other ISR shares the variable, it should be possible for the ISR to get direct exclusive non-panicking access. The main thread would need to disable the ISR (but only that one ISR) to get safe exclusive access.

The variable might be statically initialised (in the simplest case), or might have to be early-runtime initialised. We might be able to support statically initialised variables first and only later have a good solution for runtime-initialised: you can always use an Option<T> to bridge the gap.

Sharing a variable between the main thread and one or more interrupt handlers

This is the much harder case where any ISR might access the variable and might pre-empt another ISR, which seems like it leaves us with either whole-program analysis a la RTFM or critical sections a la the current Mutex dance.

Moving a variable from the main thread to one interrupt handler

Main needs to initialise or obtain some variable (typically a peripheral instance) and only the ISR needs to access it. By definition this is not a statically initialised variable but rather something initialised at runtime then moved. Once moved you'd want the ISR to have direct and non-panicking access.

Anything else? I think even just addressing the simplest version of the first use case (statically initialised, shared between main and one ISR only) would be a huge win.

@jamesmunns
Copy link
Member Author

Re: Moving a variable; it would be nice to have a way to move the variable back if we shut down the interrupt. This could be useful when swapping different interrupts throughout the run time, but probably not strictly necessary for a first useful approach. Additionally this approach is useful if you are using something like BBQueue where you have SPSC guarantees already, and you just need to give one or more producers/consumers to the interrupt handler so it can fill/drain events/data as necessary (without a mutex or semaphore).

Re: one or more interrupt handlers; yeah, I think we need the RTFM approach to avoid deadlock guarantees, but it might be useful to have something like Shared for this usecase, with a big red "you must BYO guarantees about deadlock avoidance", or somehow never allow more than one resource handle to be held at any time.

Re main thread and only one interrupt handler; I think you nailed it. I would say one possible interesting item that this point would allow would be something like ping-pong/double buffers, where the main thread can move the buffer from 1->2, and the interrupt can move the buffer from 2->1 in a safe way with shared memory.

@adamgreig
Copy link
Member

it might be useful to have something like Shared for this usecase, with a big red "you must BYO guarantees about deadlock avoidance"

Sure. It's already deadlock-free on Cortex-M since Mutex requires a CS which can't be interfered with. Honestly at that point we're not far off just suggesting using static mut and pretending you're dealing with C and people can do their own analysis re pre-emption or deadlocking; no different to what you'd have to do in C. I'd really like to sort out the simpler cases before worrying about replacing what RTFM already does well for large/complicated scenarios.

@jamesmunns
Copy link
Member Author

@adamgreig Ah sorry, you are correct, in the other thread we talked about the case where only one interrupt would be disabled, and I wasn't thinking of the "total critical section" case we currently have.

Honestly I think having something like an arc_singleton!() would handle the first two cases, and should be possible once MaybeUnInit lands in 1.32. (Essentially a Mutex<T> that is statically allocated similarly to the singleton!() macro or the bbq!() macro in BBQueue).

@therealprof
Copy link
Contributor

Actually the current Mutex dance also allows moving stuff out of the Mutex and into a "static" variable in the interrupt handler allowing to protect it from external access without having to lock the resource.

@eddyp
Copy link

eddyp commented Jan 15, 2019

Honestly I think having something like an arc_singleton!() would handle the first two cases

What would be the underlying hw mechanism on which it should be implemented?

Because if we're talking about single core and there is an interrupt involved, anything else but a way to mask/disable the interrupt doesn't seem to cut it.

/confused

Still, is this supposed to be a mechanism only for single core?

@perlindgren
Copy link

Adding to the discussion. We are working on a multicore extension of RTFM (prototype has been up and running and under evaluation for some months already).

The most straightforward approach is to have Tasks and Resources associated to cores by the programmer, allowing shared resources only within a single domain. This might sound too restrictive, but given that message passing is implemented using lock free queues, we only need to ensure that atomicity across cores are enforced. In practice this allows zero cost data propagation between cores. Not exactly sure haw this arc_singleton would fit into this picture though....

As mentioned in other issue (#15), we would be happy to see actual examples where RTFM does not fit Your bill of embedded programming, and from there suggest and/or develop suitable patterns. (And I don't agree that hiding a potentially panicking Mutex behind a newtype would be a better abstraction than the guaranteed race- and deadlock free access you get from RTFM).

Best regards/
Per

@HarkonenBade
Copy link

@perlindgren Is there any meaningful way to make RTFM not require as much macro magic? Even if it means driving some upstream development? As the thing that presents me with the most issue from RTFM is the use of macros to generate a new DSL within rust, I would favour something that was more purely expressed in regular rust syntax.

@jamesmunns
Copy link
Member Author

@perlindgren I think the rub here is that I would like to support users who do, and do not use RTFM. I could believe that use of RTFM, or a similar tool which has whole-program visibility, is perhaps to only way to guarantee zero cost overhead towards safe code.

However, I think there is still value in a low-cost, yet safe set of abstractions that could be used outside of the context of RTFM.

If you believe it is only valuable to develop Embedded Rust in the context of RTFM, I might suggest that you submit an RFC making that a primary/official goal of the Working Group.

@jamesmunns
Copy link
Member Author

@eddyp Yeah, my suggestion for arc_singleton!() was perhaps overly bold. What I had in mind was a statically allocated mutex that would be initialized on first access. Sort of like lazy_static!() + the cortex-m::Mutex.

This likely would only be safe across a single core.

@japaric
Copy link
Member

japaric commented Jan 22, 2019

Solutions that don't depend on procedural macros, off the top of my head:

A. Signaling from ISR to main

  • static _: AtomicBool

    • Upside: multi-core safe (if correct Ordering is used)
    • Downside(correctness): global static variable, anyone can modify it
    • Downside(portability): doesn't work on ARMv6-M, a CAS loop is needed in main

  • Signal

    • What: Use bit-banding to pack 32 AtomicBools in a single word.
    • Unclear: is bit-banding multi-core safe? (IDK)
    • Downside(portability): doesn't work on ARMv6-M for the same reasons as AtomicBool

B. Sharing between ISR and main

  • static _: spin::RwLock<T>

    • Upside: multi-core safe
    • Upside: doesn't block all interrupts
    • Downside(overhead): unnecessary overhead when used from ISR
    • Downside(portability): doesn't work on ARMv6-M for the same reasons as AtomicBool
    • Downside(correctness): may deadlock
    • Downside(correctness): global static variable

  • static _: bare_metal::Mutex<RefCell<T>>

    • Downside(safety): not multi-core safe (it should not even implement Sync)
    • Downside(overhead): unnecessary overhead when used from ISR
    • Downside(correctness): always blocks all interrupts
    • Downside(overhead+correctness): can panic
    • Downside(correctness): global static variable

C. Moving from main to ISR

  • Dynamic interrupt handlers

    • What: Move semantics, _: Send requirement
    • Upside: compiler enforced access control / no global access
    • Downside(overhead): Extra memory overhead per handler, plus if let branch
      and dynamic dispatch on each ISR
    • Downside(ergonomics): Need some sort of allocator to move the captures into
      "leaked" memory at runtime (this is equivalent to creating static mut
      variables at runtime)
    • Downside(correctness): allocator can run out of memory (this is a bug
      (programmer error) that should be easy to fix though)

  • static _: spin::RwLock<Option<T>>, static _: bare_metal::Mutex<RefCell<Option<T>>>

    • These don't have proper move semantics; they are just runtime initialized
      versions of (B)
    • Extra downside: will panic if one tries to use the static before initializing it

@japaric
Copy link
Member

japaric commented Jan 22, 2019

@HarkonenBade

I would favour something that was more purely expressed in regular rust syntax.

RTFM is expressed in regular Rust syntax; attributes, which cortex-m-rt also uses
plenty of, are regular Rust syntax; if you can rustfmt something then it's
regular Rust syntax.

With crate level attributes you can reduce the number of required annotations by
introducing inference; though, that's more macro magic, not less. For example,
moving from main to ISR could look like this:

#![app]

// runtime initialized static
// (you can't get rid of this because there's no static-level type interference)
static mut SERIAL: Serial = (); // or `= UNINIT` (w/e syntax you prefer)

fn init() {
    // ..

    // initialize the static
    SERIAL = Serial::new();
}

fn main() -> ! {
    loop {
        // ..
    }
}

#[interrupt]
fn USART0() {
    // "move" into this ISR (you only get `&mut Serial`, though)
    let serial = SERIAL;

    // do stuff with `serial`
}

Is there any meaningful way to make RTFM not require as much macro magic?

Even if it means driving some upstream development?

The DSL is used to express "before, after" constraints and ownership / sharing
of static variables. These only make sense in the bare metal domain (where
non-reentrant interrupt handlers exist), so a DSL is the right way to express
this, IMO.

As the syntax / features are not general purpose I don't see them ever being
integrated into the language. Unless we are talking about adding some --dsl
flag to rustc; though I don't really see that ever happening either.

@therealprof
Copy link
Contributor

Downside(correctness): always blocks all interrupts

That is not necessarily a downside and certainly not a correctness problem. Also one could move the protected resource into the ISR upon first use if the ISR is supposed to be the exclusive owner to get rid of the critical section if performance really is an issue.

Extra downside: will panic if one tries to use the static before initializing it

I don't have any problems with deterministic panics. Even better would be if the compiler could figure it out and warn about it.

@HarkonenBade
Copy link

@HarkonenBade

I would favour something that was more purely expressed in regular rust syntax.

RTFM is expressed in regular Rust syntax; attributes, which cortex-m-rt also uses
plenty of, are regular Rust syntax; if you can rustfmt something then it's
regular Rust syntax.

With crate level attributes you can reduce the number of required annotations by
introducing inference; though, that's more macro magic, not less. For example,
moving from main to ISR could look like this:

#![app]

// runtime initialized static
// (you can't get rid of this because there's no static-level type interference)
static mut SERIAL: Serial = (); // or `= UNINIT` (w/e syntax you prefer)

fn init() {
    // ..

    // initialize the static
    SERIAL = Serial::new();
}

fn main() -> ! {
    loop {
        // ..
    }
}

#[interrupt]
fn USART0() {
    // "move" into this ISR (you only get `&mut Serial`, though)
    let serial = SERIAL;

    // do stuff with `serial`
}

Is there any meaningful way to make RTFM not require as much macro magic?

Even if it means driving some upstream development?

The DSL is used to express "before, after" constraints and ownership / sharing
of static variables. These only make sense in the bare metal domain (where
non-reentrant interrupt handlers exist), so a DSL is the right way to express
this, IMO.

As the syntax / features are not general purpose I don't see them ever being
integrated into the language. Unless we are talking about adding some --dsl
flag to rustc; though I don't really see that ever happening either.

Ok, that syntax is something I feel much more comfortable with, I think i was mostly being thrown off by the weird const stuff in the current version of RTFM.

@therealprof
Copy link
Contributor

@japaric Can't we have:

#![init]
fn init() {
    static mut SERIAL: Serial = ();
    
    // ..

    // initialize the static
    SERIAL = Serial::new();
}

#![loop]
fn main() -> ! {
    // ..
}

#[interrupt]
fn USART0() {
    // "move" into this ISR (you only get `&mut Serial`, though)
    let serial = SERIAL;

    // do stuff with `serial`
}

@HarkonenBade
Copy link

HarkonenBade commented Jan 22, 2019 •
edited
Loading

  • static _: bare_metal::Mutex<RefCell<T>>

    • Downside(safety): not multi-core safe (it should not even implement Sync)
    • Downside(overhead): unnecessary overhead when used from ISR
    • Downside(correctness): always blocks all interrupts
    • Downside(overhead+correctness): can panic
    • Downside(correctness): global static variable

Out of interest, in what situations does this panic? As I'm pretty sure i'd made my implementation of the shared wrapper non-panicking.

C. Moving from main to ISR

  • Dynamic interrupt handlers

    • What: Move semantics, _: Send requirement
    • Upside: compiler enforced access control / no global access
    • Downside(overhead): Extra memory overhead per handler, plus if let branch
      and dynamic dispatch on each ISR
    • Downside(ergonomics): Need some sort of allocator to move the captures into
      "leaked" memory at runtime (this is equivalent to creating static mut
      variables at runtime)
    • Downside(correctness): allocator can run out of memory (this is a bug
      (programmer error) that should be easy to fix though)

Can we not avoid using an allocator by having the interrupt handler controller pre-allocate a static block of memory for all the handlers? (possibly with ways to reduce that allocation if you can just choose specific handlers you are expecting to use). As I personally find this method very very attractive because of its similarity to how similar patterns work in full fat systems with sharing data to threads and such.

@perlindgren
Copy link

perlindgren commented Jan 22, 2019 via email

@jamesmunns
Copy link
Member Author

@perlindgren There is a lot to unpack there, and I hope to be able to give a longer answer later. I do appreciate the history and discussion of the design constraints you have worked with.

However, I want to reiterate that I am not against usage of proc macros, nor even the current syntax of RTFM4. I think that some of the module-level proc macro awareness will help with logically structuring code (if people prefer that). I'm actually a fan of "magic", as long as the cognitive overhead involved is acknowledged and minimized, as much as possible. That being said, this is my opinion, and others may disagree.

I want to restate my goal as supporting the use cases listed in #294 (comment), for:

  • End users/applications using RTFM
  • End users/applications NOT using RTFM
  • Library crate developers, supporting applications which may or may not use RTFM

In particular, the last item, library crate developers, are not a use case I have seen you address yet (though Jorge did hit on that topic in his response). It is likely that libraries will need to interact with thread-safe components, and having a way to "give" them those components at runtime, either through dependency injection, or other means. In particular, HAL crate developers may also have a want or need to provide interrupt handler routines, in essence "taking" or "borrowing" the interrupt resource and related data, in order to improve ergonomics for users of these crates.

Again, I am very thankful for the existence of RTFM, and I don't aim to detract from what you have achieved. However as Rust is much more package based than C or C++, the crates in Rust need to "stand on their own", and be correct without depending on RTFM. This is the problem that I have faced as a maintainer of nrf52-hal, and trying to provide convenient and correct abstractions for all users of this library.

@perlindgren
Copy link

perlindgren commented Jan 22, 2019 via email

@japaric
Copy link
Member

japaric commented Jan 23, 2019

@therealprof

That is not necessarily a downside and certainly not a correctness problem

In general, interrupts can have different priorities. Setting them all to the
same priority is just one of the hundreds or thousands of different
possibilities. In general, the critical section will prevent higher priority
interrupts from starting and that's a downside; it also affects correctness
because a higher priority was given for a reason and the critical section is
nullifying that setting (goes against the specification).

Also one could move the protected resource into the ISR upon first use

Sure, but your comment refers to a solution to the 'share between main and ISR'
problem so it doesn't apply.

Can't we have:

You can put the static mut declaration wherever, yes. But note that you need
access to the whole program (and thus a crate level attribute) to prevent code
like this:

// same as before

#[interrupt]
fn USART0() {
    let serial = SERIAL;

    // do stuff with `serial`
}

#[interrupt] // this could be running a different priority (that would be UB)
fn USART1() {
    let serial = SERIAL; // <- this should be a compile time error

    // do stuff with `serial`
}

Unless you (a) equalize all interrupt priorities after init returns and
before main starts and (b) force the programmer to give up ownership of NVIC
by the end of init. Those two are required to keep the priorities static and
the static (compile time) analysis correct. Then you can accept the above
program.

@HarkonenBade

Out of interest, in what situations does this panic?

RefCell is panicky. Its runtime check can not be optimized away (when you put
it in a static) and the panicking branch will be kept in the final binary.
Some examples where the RefCell will / may panic:

static FOO: Mutex<RefCell<u64>> = Mutex::new(RefCell::new(0));

#[interrupt]
fn USART0() {
    interrupt::free(|cs| {
        let foo = FOO.borrow(cs);
        let x = foo.borrow_mut();
        bar();
        let y = foo.borrow_mut(); // this panics
    });
}

// "nobody writes code like that!", right?
// your collegue may write this in some other file / module though
fn bar() {
    interrupt::free(|cs| {
        let foo = FOO.borrow(cs);
        let x = foo.borrow_mut(); // may panic
        // ..
    });
}

// Or yet another possibility
#[exception] // this won't be stopped by the critical section and can preempt USART0
fn NMI() { // this could be HardFault; same problem
    interrupt::free(|cs| {
        let foo = FOO.borrow(cs);
        let x = foo.borrow_mut(); // this will panic if it preempts USART0
    });
}

Again, the root of the problem is the global static; it makes it hard to write
correct code. Replacing Mutex<RefCell<T>> with spin::RwLock<T> gives you
deadlocks instead of panics. The solution is not a "better Mutex"; the solution
is to stop using global statics.

Can we not avoid using an allocator by having the interrupt handler controller
pre-allocate a static block of memory for all the handlers?

That's possible. You could either pre-allocate in excess for all handlers
(wastes RAM) or provide fine grained control over each handler's static block
(tedious and error prone). (Both options remind of reserving stack space for
threads.)

@HarkonenBade
Copy link

@japaric Ah ok, that makes sense in terms of the panics. With my wrapper I had equated both the 'this value hasn't been initialised' and 'you cannot get a borrow on this value at this time' to both return None with the intent that it would be used like:

static FOO: Shared<u64> = Shared::new();

#[interrupt]
fn USART0() {
    interrupt::free(|cs| {
        if let Some(foo) = FOO.get(cs) {
            /* do stuff with foo */
        }
    });
}

@adamgreig adamgreig mentioned this issue Jan 23, 2019
Closed
@eddyp
Copy link

eddyp commented Jan 23, 2019

I am a little confused, are we talking about an embedded generic solution, or are we talking about RTFM?

In general, interrupts can have different priorities. Setting them all to the
same priority is just one of the hundreds or thousands of different
possibilities.

The OSEK/AUTOSAR OS solution for this is using priority ceiling, i.e. temporary raising the priority of the task/code accessing the shared resource to the highest level of the tasks/ISRs sharing that particular resource.

Not sure how we can translate this to code without an OS and how we can make some Rustic implementations of GetResource/ReleaseResource which could actually be implemented once and reused to implement the priority ceiling protocol for an OS. My gut feeling is we should be able to use the type system somehow, but I think we will need to use some kind of locking mechanism (e.g. spinlock) to achieve run-time panic-free code.

@eldruin eldruin mentioned this issue Jan 23, 2019
Closed
@therealprof
Copy link
Contributor

it also affects correctness because a higher priority was given for a reason and the critical section is
nullifying that setting (goes against the specification).

I disagree. A critical section is a reasonable way to ensure exclusive access to shared resources. It may not be the ideal way but that is a different topic.

Sure, but your comment refers to a solution to the 'share between main and ISR'
problem so it doesn't apply.

Fair.

Unless you (a) equalize all interrupt priorities after init returns and
before main starts and (b) force the programmer to give up ownership of NVIC
by the end of init. Those two are required to keep the priorities static and
the static (compile time) analysis correct. Then you can accept the above
program.

Absolutely. We already do this in the e.g. #[interrupt] and #[entry] macros, right? The main concern here to keep a familiar program structure.

Replacing Mutex<RefCell> with spin::RwLock gives you deadlocks instead of panics.

There's nothing worse than deadlocks in embedded programming. Trading a panic for a deadlock is a horrible idea.

The solution is not a "better Mutex"; the solution is to stop using global statics.

Agreed.

@japaric
Copy link
Member

japaric commented Jan 24, 2019

@eddyp

I am a little confused, are we talking about an embedded generic solution, or are we talking about RTFM?

All kind of solutions.

Not sure how we can translate this to code without an OS and how we can make some Rustic implementations of GetResource/ReleaseResource which could actually be implemented once and reused to implement the priority ceiling protocol for an OS

I think a safe API like raise(to_priority, || { /* critical section */}) would be a reasonable addition. But I don't see how a safe PcpResource<T> { data: UnsafeCell<T>, ceiling: u8 } API could be implemented as it would rely on external invariants like (a) priorities must be kept static and (b) must not be used from interrupt handler with priority greater than ceiling. As soon so you put such PcpResource in a (global) static variable it becomes impossible to prevent (b).

RTFM uses the priority ceiling protocol (PCP) and exposes a safe API to access the underlying data, but this is only possible because the DSL enforces the (a) and (b) invariants at compile time.

My gut feeling is we should be able to use the type system somehow

I refer you to RTFMv1 for an old version that used the type system to track interrupt priorities and ceilings (i.e. BASEPRI). Not only was the API super unergonomic to use, but there were also problems with Rust aliasing model / borrow checker that forced you to use Cell / RefCell everywhere. Again, the root of all problems were the global static variables.

@therealprof

A critical section is a reasonable way to ensure exclusive access to shared resources

I agree with this. My comment was specifically about disabling all interrupts to create a critical section. That mechanism also blocks higher priority task that don't share memory with the context that needs to access the shared memory -- that's what I was referring to as "a correctness issue". Other mechanisms to create critical sections like masking interrupts and raising the priority (see BASEPRI) don't have this issue (or minimize the issue).

@japaric
Copy link
Member

japaric commented Jan 24, 2019

dynamic interrupt handlers

@HarkonenBade and I were talking a bit about this yesterday on IRC and came up
with lower cost implementations that don't need an allocator.

struct + trait instead of a closure

A closure is just a struct that implements the (or one of) Fn* traits. So
one idea is to use a named struct and some trait instead of an anonymous
closure.

The API could look like this

// use cortex_m_rt::Interrupt;

// This is a named closure struct
// NOTE: struct name must match a device interrupt
// NOTE: fields must be `Send`
// NOTE: all fields that are references must have `'static` lifetime
#[derive(Interrupt)]
struct USART0 {
    // captures
    counter: u32,
}

#[entry]
fn main() -> ! {
    let my_counter = 1;

    // register an interrupt handler
    USART0 {
        // capture stack variable (move it into the closure)
        counter: my_counter,
    }
    .register(|data| {
        data.counter += 1;
        println!("{}", data.counter);
    });

    loop {
        // other stuff
    }
}

Implementation details.

A named closure struct lets us store it in a static mut variable removing the
need for trait objects and an allocator.

static mut _: impl Trait

When the static mut _: impl Trait feature becomes available (and depending on
what you are allowed to do with it) it should become possible to use the closure
syntax to register an interrupt handler but the API would need to be a 1.0
macro.

The API could look like this:

#[entry]
fn main() -> ! {
    let my_counter = 1;

    register!(USART0, move || {
        // captured stack variable
        my_counter += 1;

        println!("{}", my_counter);
    });

    loop {}
}

Implementation details:

// expansion of `register!`
unsafe {
    //  start of user input
    let handler = move || {
        my_counter += 1;

        println!("{}", my_counter);
    };
    // end of user input

    static mut HANDLER: Option<impl FnMut() + Send> = None;

    // FIXME this needs to be interrupt safe
    HANDLER = Some(handler);

    #[interrupt]
    unsafe fn USART0() {
        if let Some(mut handler) = HANDLER {
            handler();
        } else {
            // default handler
            intrinsics::abort() // or w/e makes sense
        }
    }
}

@therealprof
Copy link
Contributor

@japaric That looks great for the moving of resources into interrupt handlers. How would the sharing work?

@HarkonenBade
Copy link

@japaric That looks great for the moving of resources into interrupt handlers. How would the sharing work?

Currently it would use reference semantics, so things that only require & references can be passed to multiple interrupt closures, things that require &mut references can only be used in a single interrupt. At this point we would then want a proper implementation of Mutex or similar to allow safe and structured upgrading from a & ref to a &mut ref while maintaining exclusivity and safety.

@HarkonenBade
Copy link

HarkonenBade commented Jan 24, 2019 •
edited
Loading

dynamic interrupt handlers

static mut _: impl Trait

I have high hopes for this as it feels like a very elegant syntax for doing bare bones interrupt interfacing in places where the rust compilers reference semantics are sufficient to solve any sharing concerns.

@japaric
Copy link
Member

japaric commented Jan 24, 2019

@therealprof

How would the sharing work?

Sharing (references) doesn't really work. That's why this is listed under 'moving from main to ISR'. You can use channels, though.

@HarkonenBade

It's more nuanced than that. Since we are placing the closure in a static there's an implicit 'static bound so you can only send &'static and &'static mut references. Also note that there's a Send bound because this is equivalent to thread::spawn so you can only send &'static T if T: Sync, meaning that T can't be Cell or RefCell, or anything else that has unsychronized interior mutability.

@chrysn
Copy link

chrysn commented Jan 25, 2019

For non-RTFM use, when I started reading this thread I had hoped to find something like

#[interrupt]
fn USART0(serial: Serial) {
    // do stuff with `serial`
}

fn main() {
    let serial = ...;
    USART0.enable(serial);
    // to take it back later:
    let (serial, ) = USART0.disable().expect("Interrupt was not active");
}

that could have no overhead at all in the interrupt, but that'd only be achievable if we could make sure that the interrupt never ever gets enabled without setting the static mut that's somewhere in the expansion (eg. via nvic.enable()), and in non-RTFM land I don't see a way to prevent that.

(This case seems to be the most important of the use cases, as the data-flow cases seem to come naturally by passing one end of an SPSC into the interrupt).

@eddyp
Copy link

eddyp commented Jan 25, 2019

   #[interrupt]
    unsafe fn USART0() {
        if let Some(mut handler) = HANDLER {
            handler();
        } else {
            // default handler
            intrinsics::abort() // or w/e makes sense
        }
    }

I assume you can put the entry of unsafe fn USART0() in the vector table, right?

@eddyp
Copy link

eddyp commented Jan 25, 2019 •
edited
Loading

   let serial = ...;
    USART0.enable(serial);
    // to take it back later:
    let (serial, ) = USART0.disable().expect("Interrupt was not active");

If I understand your idea correctly, that's not quite idiomatic and you are still in a situation where the developer might forget to enable the interrupt.

It's better to have the enable be implicitly done at scope end, only make explicit the entry in the critical section; this would be similar in felling to how drop() happens.

Also I don't consider the panic an option, better have a 0 cost abstraction or stick with C if we don't 🤪

@chrysn
Copy link

chrysn commented Jan 25, 2019

might forget to enable the interrupt

I'd consider this a good thing: The function does not get called until something explicitly requests it to. (And otherwise, how can one hope to have as little error handling as possible run in the interrupt?) IMO the "interrupt local" variables should be valid whenn the interrupt is enabled. (Conversely, disabling the interrupt would (move out and) drop them, and never disabling keeps them forever owned by it).

It's better to have the enable be implicitly done at scope end

I don't understand what you mean there; the intended workings of the .enable() functon were "assert that the interrupt is not enabled; set the data; enable the interrupt" (needs a critical section only if there can be shared access to the USART0 object, which we might not need if the interrupt handler gets placed inside main); the .disable() would "assert the interrupt is enabled, disable it, and return any data set to it".

I see, though, (from the "critical section if" part) that this is getting so close the "static mut _: impl Trait" version it (when thought through) probably winds up being the same, plus/minus whether there is a mutable closure or a function with its syntactic arguments turned into global statics by similar macros to what treats their statics now.

@japaric
Copy link
Member

japaric commented Jan 25, 2019

@eddyp

I assume you can put the entry of unsafe fn USART0() in the vector table, right?

That's what the #[interrupt] attribute does: it statically (i.e. at compile
time) installs a function in the vector table. It works with both fn and
unsafe fn.

@chrysn

That looks like a reasonable API, but its behavior in edge cases needs to
specified. Consider these scenarios

  1. Pending the interrupt before enable-ing it.
#[interrupt]
fn USART0(serial: &mut Serial) { // (argument needs to be `&mut _` to prevent cloning singletons)
    // ..
}

#[entry]
fn main() -> ! {
    let p = cortex_m::Peripherals::take().unwrap();

    p.NVIC.enable(Interrupt::USART0);
    // triggers `USART0`; UB since its state is not initialized
    p.NVIC.pend(Interrupt::USART0);

    let serial = ..;
    USART0::enable(serial);
}
  1. Rewriting the interrupt state from the handler itself
#[interrupt]
fn USART0(x: &mut u64) {
    // ..

    // UB if unchecked
    USART::enable(..);

    // ..
}
  1. Rewriting the interrupt state from a higher priority interrupt
#[interrupt]
fn USART0(x: &mut u64) {
    // ..

    // preempted by EXTI0 at this point

    // ..
}

// higher priority than `USART0`
#[interrupt]
fn EXTI0(nvic: &mut NVIC) {
    NVIC.disable(Interrupt::USART0);

    // UB if unchecked
    USART::enable(..);
}

@chrysn
Copy link

chrysn commented Jan 25, 2019

The "What if it gets pended" would require all code paths that can lead to an enabled pending interrupt to be &mut-protected. That might be have been feasible with enabling, but having (prompted by your example) found that an interrupt can be pending without being enabled, and ::pend() has become globally available – nevermind. The function syntax could be salvaged if all arguments were demanded to implement Default, but I'm not sure that's really the way we want it to be (after all, now that they can have already used their default arguments, setting them in enable might need to drop the old default values, and things get awkward).

I thought the "rewriting from handler itself" and "rewriting from higher priority interrupt" could go away if enable took a &mut self of the interrupt handler – but there's nothing to keep a user from getting such a mut references into one of the higher interrupts where they could still no it – so setting or recovering data would need to be fallible on the interrupt being active right now. They'd need to, in a critical section, compare the current priority with the interrupt's.

The updated example doesn't look half as nice as my original one (as the interrupt author can't differentiate between interface arguments and statics any more), but may be still worth considering:

#[interrupt]
fn USART0() {
    static serial: Option<Serial> = None;
    static count: i32 = 0;
    // ... as currently
}

// Not doing this in main to demonstrate it can return
fn setup(some_peripherals) {
    let serial = some_peripherals.serial.take();
    USART0::set(|s| s.serial(serial).count(99));
    USART0::enable();
}

(Whether the setters are grouped or not, or the enable is in there as well, is probably a matter of taste; some grouping does make sense as all those accesses incur the run-time criticial-section-plus-priority-checking cost that RTFM gives for free; the grouping reduces that and may be nice to read too.)

The interrupt macro could stay quite similar to how it is now, and'd "just" give runtime protected access to its statics.

@HarkonenBade
Copy link

The other option is that we could restrict access to the NVIC if you were using this method of interrupt orchestration. As it would probably need to own at least a pointer to the NVIC anyway to handle enabling and disabling bits.

@chrysn
Copy link

chrysn commented Jan 25, 2019

ad "restrict access to the NVIC" That restriction would need to be build-time (eg. from a feature in cortex-m that disables access to it), as the vectors are in the global table from the beginning of execution, and saying "If you use this pretty please call the thing that consumes the NVIC before doing anything else" won't be enough to claim safety.

Might be feasible, but I'd be unsure whether that'd composes well with HALs that just started migrating from nvic.set_pending() to NVIC::pend(), and it sounds a bit like building an RTFM-light (that might easily end up being RTFM which already doesn't have those issues AFAIU).

@HarkonenBade
Copy link

More I'd be tempted to have some 'InterrruptController' that takes posession of the NVIC peripheral and then is used to enable/disable/pend interrupts.

e.g.

fn USART0(serial: &mut Serial) { // (argument needs to be `&mut _` to prevent cloning singletons)
    // ..
}

#[entry]
fn main() -> ! {
    let p = cortex_m::Peripherals::take().unwrap();

    let ic = InterruptController::new(p.NVIC);

    ic.pend(USART0); // Will return a failure as USART0 is not enabled.

    let serial = ..;
    ic.enable(USART0, (serial,));

    ic.pend(USART0);
}

@jamesmunns
Copy link
Member Author

@HarkonenBade I was actually thinking something similar. I think for safety it might require that we enforce ownership of the NVIC (so no NVIC::pend(), or at least make that unsafe), and maaaaaybe ownership of the Interrupts, though I'm not 100% on that.

It would be nice to have InterruptController::enable() be failable, for the reasons you listed.

I've also been thinking about how to reasonably statically-allocate space for the resources used by each interrupt, and how we could possibly avoid dynamic dispatch (e.g. every interrupt hits the Interrupt Controller, then it dispatches with the correct context info).

@japaric japaric mentioned this issue Jan 29, 2019
Closed
@jamesmunns jamesmunns mentioned this issue Mar 7, 2019
Merged
7 tasks
@jamesmunns
Copy link
Member Author

This is now listed at https://github.com/rust-embedded/not-yet-awesome-embedded-rust#sharing-data-with-interrupts, I think we can close this issue here.

@jonas-schievink
Copy link
Contributor

I've recently published the irq crate to help with this. AFAICT it addresses all success criteria listed in https://github.com/rust-embedded/not-yet-awesome-embedded-rust#sharing-data-with-interrupts, but I've not followed this thread for any other patterns that aren't currently possible.

@tstellanova tstellanova mentioned this issue Apr 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@adamgreig @eddyp @chrysn @HarkonenBade @jonas-schievink @jamesmunns @therealprof @perlindgren @japaric