rtic-rs / rfcs

11 stars 6 forks source link

Goodby Exclusive, Welcome Symmetrical Lock and [mut/&resource] optimization. #17

Closed perlindgren closed 2 years ago

perlindgren commented 5 years ago

Currently RTFM has the notion of asymmetric access to shared resources. Tasks with highest priority accessing shared:T gets a &mut T API, while other tasks (if any) gets a resource proxy, with a lock API (former claim). The examples/lock.rs from the current upstream.

#[rtfm::app(device = lm3s6965)]
const APP: () = {
    struct Resources {
        #[init(0)]
        shared: u32,
    }

    #[init]
    fn init(_: init::Context) {
        rtfm::pend(Interrupt::GPIOA);
    }

    // when omitted priority is assumed to be `1`
    #[task(binds = GPIOA, resources = [shared])]
    fn gpioa(mut c: gpioa::Context) {
        hprintln!("A").unwrap();

        // the lower priority task requires a critical section to access the data
        c.resources.shared.lock(|shared| {
            // data can only be modified within this critical section (closure)
            *shared += 1;

            // GPIOB will *not* run right now due to the critical section
            rtfm::pend(Interrupt::GPIOB);

            hprintln!("B - shared = {}", *shared).unwrap();

            // GPIOC does not contend for `shared` so it's allowed to run now
            rtfm::pend(Interrupt::GPIOC);
        });

        // critical section is over: GPIOB can now start

        hprintln!("E").unwrap();

        debug::exit(debug::EXIT_SUCCESS);
    }

    #[task(binds = GPIOB, priority = 2, resources = [shared])]
    fn gpiob(c: gpiob::Context) {
        // the higher priority task does *not* need a critical section
        *c.resources.shared += 1;

        hprintln!("D - shared = {}", *c.resources.shared).unwrap();
    }

    #[task(binds = GPIOC, priority = 3)]
    fn gpioc(_: gpioc::Context) {
        hprintln!("C").unwrap();
    }
};

This is unfortunate, as

  1. Introducing another task in the application may break code. gpiob would need to be changed API if allowing gpioc access to shared.
  2. If we want gpioa to pass the resource access to another (external) function (like a HAL), we would need to wrap shared by Exclusive (which gives a lock API to inner).
  3. Context cannot have 'static lifetime if allowed to have &mut T in resources. This will prevent from captures in static stored generators (which requires 'static lifetime). Generators can be useful as is (initial experiments confirm the possibility), and are used under the hood for async/await, so if we want to keep this door open, we will eventually run into this problem.

Proposal.

Offer only a single API (always resource proxy) for all shared resources. Pros:

Cons:

Performance:

[resource] vs [mut resource] (or [&resource] vs [resource]) In a related RFC, Jorge proposed to distinguish between mutable and immutable access. With the above proposal, this will amount in a slight syntax extension. (Following Rust I would think that [mut resource] would be appropriate, but the &syntax would be backwards compatible, so I don't have a strong opinion).

Pros:

Cons:

Under the "Goodby Exclusive" proposal, the optimization can be done fully transparent to the API. lock will lend out &T/&mut T, for immutable/mutable access respectively and the compiler will block misuse.

I might have missed something, looking forward to input. Best regards Per

MabezDev commented 4 years ago

The non symetrical API also introduces issues when using the resource proxy concrete types because the higher prio task doesn't get a resource proxy, instead it gets the resource directly. Example:

#[rtfm::app(device = lm3s6965)]
const APP: () = {
    struct Resources {
        // A resource
        #[init(0)]
        shared: u32,
    }

    #[task(resources = [shared])
    fn normal_prio(c: normal_prio::Context) {
        takes_proxy(c.resources.shared); // works
    }

    #[task(resources = [shared], priority = 5)
    fn high_prio(c: high_prio::Context) {
        // fails to compile, expected type `resources::shared` got `&mut u32`
        takes_proxy(c.resources.shared); 
    }

};

fn takes_proxy(shared: resources::shared) {
    shared.lock(|_s| {});
}
perlindgren commented 2 years ago

Close as solved.