Add local tasks

[usbalbin]

This PR adds the concept of "local tasks". Local tasks are tasks that may only be spawned from other tasks with the same priority, thus running on the same executor. Thanks to this, there is no requirement on the arguments to local tasks to be Send/Sync. Local tasks will therefore not have the usual my_task::spawn method available.

All tasks, local or not, that may spawn other local tasks will have a public field local_spawner in their Context. This type will then have methods for all local tasks that may be spawned. Tasks are made into local tasks by setting the task attribute local_task = true (default false).

#[app(device = pac, peripherals = false, dispatchers = [SPI1])]
mod app {
    use super::*;

    #[shared]
    struct Shared {}

    #[local]
    struct Local {}

    #[init]
    fn init(_cx: init::Context) -> (Shared, Local) {
        task1::spawn().ok();
        //task2::spawn(Default::default()).ok(); <--- This is rejected since it is a local task
        (Shared {}, Local {})
    }

    #[task(priority = 1)]
    async fn task1(cx: task1::Context) {
        defmt::info!("Hello from task1!");

        // This is the only way to spawn task2, by using the local spawner
        cx.local_spawner.task2(Default::default()).ok();
    }

    //    Note the same prio as above
    //     vvvvvvvv
    #[task(priority = 1, local_task = true)] // <-- This is a local task
    async fn task2(_cx: task2::Context, _nsns: NotSendNotSync) {
        defmt::info!("Hello from task1!");
    }
}

#[derive(Default)]
struct NotSendNotSync(PhantomData<*mut u8>);

[usbalbin]

CI seems quite angry, however I am not so sure if it is all my fault...

[datdenkikniet]

Is there a specific need/reason not to default to all tasks at the same priority to be locally spawnable? If the executor/performance characteristics are expected to be the same, and all of the non-local spawning is unaffected, is there any need for being able to turn off local tasks?

If one of the (few) reasons is that introducing new functionality like this introduces lots of unknowns, or there is some fixed-size overhead for this, could it make more sense to put this functionality behind a cargo feature in rtic/rtic-macros instead?

[datdenkikniet]

Woops, I see now that I left this as a single comment: I meant to put it up as part of a review.

I think the name change still applies, but the new question (why have it be a configuration field at all) is more important.

[datdenkikniet]

Ah OK, I've thought about this a little more and now realize that this isn't an additive change, making the task local-only also removes the global spawning mechanism. That doesn't really fit behind a feature, so I guess the name change (is_local_task -> local_task) and extra allowed form are actually relevant.

To save you from clicking through the history, this was the original comment:

To be consistent, I think we should rename this to local_task, and accept both of these forms:

#[task(local_task)]
#[task(local_task = <bool>)]

[usbalbin]

[...] this isn't an additive change [...]

Yeah exactly. Marking a task as local_task is quite a fundamental change with large limitations.

[datdenkikniet]

Is this an unrelated formatting change, or am I missing a functionality change here?

[usbalbin]

Is this an unrelated formatting change

I think so, yes

[usbalbin]

Would you like me to revert this?

[datdenkikniet]

Ideally, yes. If you submit it as a separate PR I can force-merge it, while getting rid of this thing bugging you

[usbalbin]

I cherry-picked your commits from #1115 to see if the CI is happy. Let me know if you want me to clean up the history and/or rebase.

[usbalbin]

Not sure what is wrong but CI seems to be stuck running forever :/

[datdenkikniet]

It seems that the examples in examples/lm3s6965 expect a debug::exit call to indicate that the example has finished when running in QEMU, which the example you added does not have.

I've cancelled the two jobs for now

[korken89]

A thought, why is the is_local_task = true needed? All tasks which are at the same prio should get the extended API in their context so that they can spawn other tasks at the same prio.
To me is_local_task = true is the same as checking if two tasks have the same priority.
To get the effect of supporting !Send spawns, we only need an API that enforces that the spawner and spawnee have the same priority.

Or is there something wrong in my conclusion?

[usbalbin]

My understanding is that we can not know in the macro if the type used in the arg to a task is Send/Sync or limit the task to only be spawnable on the same prio. Yet we have to decide in the macro whether to produce the assert Send/Sync code. The is_local_task(recently renamed to local_task) is to tell the macro that some args are not Send/Sync.

However if there is some clever way to get rid of it then I am happy to do so :)

[usbalbin]

One hack that would avoid the attribute would be to produce something like this:

mod task1 {
    use super::*;
    pub fn spawn<T0, T1>(x: T0, y: T1)
        where T0: Dummy<T=u8> + Send + Sync, T1: Dummy<T=NotSendSync> + Send + Sync
    {
        let x = x.to();
        let y = y.to();
        // the_real_spawn_here(x, y)
    }
}

pub trait Dummy {
    type T;
    fn to(self) -> Self::T;
}

impl<T> Dummy for T {
    type T = T;
    fn to(self) -> T {
        self
    }
}

This defers the errors to the call site:

   Compiling playground v0.0.1 (/playground)
error[E0277]: `*mut ()` cannot be sent between threads safely
   --> src/main.rs:2:23
    |
  2 |     task1::spawn(1u8, NotSendSync(core::marker::PhantomData));
    |     ------------      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut ()` cannot be sent between threads safely
    |     |
    |     required by a bound introduced by this call
    |
    = help: within `NotSendSync`, the trait `Send` is not implemented for `*mut ()`

Maybe there is a better way?

[korken89]

As I see it, the only optimization this does is that you can only generate the local spawn when is_local_task = true, and not generate the global one.
My thought is that instead: for all tasks generate local spawn to all other tasks of the same prio AND keep the current global spawn. Then you should not need the is_local_task parameter.

They fill different use-cases. Global spawn for example is so a driver can start something from the depths of a driver given only an function pointer, where local spawns are to support better interoperability with drivers that assume you run at the same priority.

[usbalbin]

@korken89

As I see it, the only optimization this does is that you can only generate the local spawn when is_local_task = true, and not generate the global one.
My thought is that instead: for all tasks generate local spawn to all other tasks of the same prio AND keep the current global spawn. Then you should not need the is_local_task parameter.

They fill different use-cases. Global spawn for example is so a driver can start something from the depths of a driver given only an function pointer, where local spawns are to support better interoperability with drivers that assume you run at the same priority.

As is now, we need the attribute to know if it is safe to skip the check for Send/Sync. The global spawn would be unsafe to call with !Send/!Sync args. But I get the feeling that I do not quite understand :)

[usbalbin]

As for always populating the local spawner with tasks of same prio, sure absolutely! Should the task also have itself in its local spawner?

[korken89]

The local spawn will have !Send, while the global spawn will always require Send - as global spawn can never know which one is calling it.
So the only ones that can alleviate the Send requirements will be the local spawns.

[usbalbin]

[...] the global spawn will always require Send [...]

Yes, and right now this is enforced by this assert which is emitted for all send_types from above, right?

This PR removes the assert and the global spawn only for the tasks marked with the attribute. If we remove the attribute then how will the macro know when it should skip generating the assert and global spawn?

We could emit where clauses on the global spawn function which checks that all the arguments impl Send + Sync like this:

fn spawn(x: u8, y: NotSendSync)
    where u8: Send + Sync, NotSendSync: Send + Sync { ... }

However that fails to compile with !Send/Sync even if not calling the types since we are using concrete types.

When creating dummy trait as done in my comment. This defers the compile error to when the function is called. This would, I think, do the same thing as the assert but only at the call site. This way we can replace the asserts with where clauses and then just always emit all the global spawn functions since it will be impossible to call them with !Send/!Sync types.

Or do you have something different in mind?