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] explicit Context parameter #155

Closed
japaric opened this issue Feb 19, 2019 · 11 comments · Fixed by #176
Closed

[RFC] explicit Context parameter #155

japaric opened this issue Feb 19, 2019 · 11 comments · Fixed by #176
Labels
RFC This issue needs you input! S-accepted This RFC has been accepted but not yet implemented
Milestone

Comments

@japaric
Copy link
Collaborator

japaric commented Feb 19, 2019

Summary

Have all handlers take a Context argument as first parameter.

Motivation

Be less magical. Currently values like resources, spawn and schedule are
directly injected into the value namespace of each handler. This RFC proposes
packing those values in a Context struct which appears in the signature of
each handler. See below:

// today
#[task(resources = [..], spawn = [..], schedule = [..])]
fn foo() {
    let foo = resources.FOO;
    spawn.bar().ok();
    schedule.baz().ok();
}

// with this RFC
#[task(resources = [..], spawn = [..], schedule = [..])]
fn foo(c: foo::Context) {
    let foo = c.resources.FOO;
    c.spawn.bar().ok();
    c.schedule.baz().ok();

    // spawn.bar().ok(); //~ ERROR value `spawn` is not in scope
}

This also makes writing trampolines much easier:

#[task(resources = [..], spawn = [..], schedule = [..])]
fn foo(c: foo::Context) {
    some::module::function(c)
}

Of course, the layout of Context is still auto-generated so which fields are
available in this particular Context remains more or less a mystery -- this
RFC doesn't help with that.

Design

All handlers now require that their first parameter is $handler_name::Context.
Tasks that take inputs will have their inputs appear as second, third, etc.
parameters.

#[app]
const APP: () = {
    // ..

    #[init(resources = [..], spawn = [..], scheduled = [..])]
    fn init(c: init::Context) -> init::LateResources {
        // ..
    }

    #[idle(resources = [..], spawn = [..], scheduled = [..])]
    fn idle(c: idle::Context) -> ! {
        // ..
    }

    // task with no input
    #[task(resources = [..], spawn = [..], schedule = [..])]
    fn foo(c: foo::Context) {
        // ..
    }

    // task with one input
    #[task(resources = [..], spawn = [..], schedule = [..])]
    fn bar(c: bar::Context, input: i32) {
        // ..
    }

    // task with two inputs
    #[task(resources = [..], spawn = [..], schedule = [..])]
    fn baz(c: baz::Context, first: i32, second: i64) {
        // ..
    }

    // task with empty context
    #[task]
    fn quux(c: quux::Context) {
         // ^ parameter is still required in this case
    }
};

Alternatives

In addition to this change we can also flatten resources into
$handler::Context , effectively removing the need for the resources. prefix.

// the main proposal
#[task(resources = [..])]
fn foo(c: foo::Context) {
    c.resources.FOO.lock(|foo| /* .. */);
}

// with this extension
#[task(resources = [..])]
fn foo(c: foo::Context) {
    c.FOO.lock(|foo| /* .. */);
}

Note that this extension does not run into the drawbacks that plague RFC #146.

Drawbacks

Application authors would have to write more code: the signature of handlers are
longer and accessing resources and spawning / scheduling tasks requires a
$context. prefix per access / invocation (though all those can be replaced
with some de-structuring at the beginning of the handler).

@japaric japaric added the RFC This issue needs you input! label Feb 19, 2019
@japaric japaric added this to the v0.5.0 milestone Feb 19, 2019
@perlindgren
Copy link
Collaborator

perlindgren commented Feb 19, 2019 via email

@TeXitoi
Copy link
Collaborator

TeXitoi commented Feb 19, 2019

Another alternative would be to have 1 parameter per "context" attribute, i.e.:

#[app]
const APP: () = {
    // ..

    #[init(resources = [..], spawn = [..], scheduled = [..])]
    fn init(r: init::Resources, s: init::Spawn, sch: init::Scheduled) -> init::LateResources {
        // ..
    }

    #[idle(resources = [..], spawn = [..])]
    fn idle(r: idle::Resources, s: idle::Spawn) -> ! {
        // ..
    }

    // task with no input
    #[task(resources = [..])]
    fn foo(r: foo::Resources) {
        // ..
    }

    // task with one input
    #[task(resources = [..], spawn = [..], schedule = [..])]
    fn bar(c: bar::Resources, s: bar::Spawn, sch: bar::Schedule, input: i32) {
        // ..
    }

    // task with two inputs
    #[task(resources = [..])]
    fn baz(r: baz::Resources, first: i32, second: i64) {
        // ..
    }

    // task with empty context
    #[task]
    fn quux() {
    }
};

@TeXitoi
Copy link
Collaborator

TeXitoi commented Feb 19, 2019

I'm in favor of this. Not a very strong preference.

My preferences would be: RFC > as now > my alternative > RFC alternative

@MabezDev
Copy link
Contributor

I like this. As much I like the ease of the magicness, I find it quite hard to explain to other people new to Rust where these values come from without having to go down a rabit hole about proc_macro's.

@korken89
Copy link
Collaborator

I'd say the presented interface is good, the one shown by @TeXitoi is also good, but this would need changes in the interface every time one changes the task definition.
IMO the first one presented is preferred.

@davidlenfesty
Copy link

As someone very new to rust and rust-embedded, this looks very nice to me.

  1. it clears up the mystery (slightly, coming from a C embedded background it still kind of seems like magic, given that interrupt vectors don't actually pass context) about where the peripherals structures come from.
  2. It (hopefully) gives a useful compile time error when you pass context and it's not explicitly included.

I don't think more code is necessarily always a problem, given that this is an embedded context more specificity is generally nice.

As well, I would say the first interface is better, since adding more code for each piece of context passed seems to be a little redundant. It could be argued that adding the context bit in the first place is redundant but I feel it would be worth it for the compile time error.

@japaric
Copy link
Collaborator Author

japaric commented Mar 26, 2019

It seems everyone who has commented is in favor of the main proposal so I'm going to call for a vote to accept the main proposal of:

#[task(resources = [..])]
fn foo(c: foo::Context) {
    c.resources.FOO.lock(|foo| /* .. */);
}

where resources are grouped in a resources field.

Votes:

@japaric
Copy link
Collaborator Author

japaric commented Apr 16, 2019

This has enough approvals and no concerns have been raised over the course of several weeks so this RFC is now officially accepted. 🎉

@japaric japaric added S-accepted This RFC has been accepted but not yet implemented and removed disposition-merge labels Apr 16, 2019
japaric added a commit that referenced this issue Apr 21, 2019
note that this assumes that RFC #155 has been implemented
bors bot added a commit that referenced this issue May 1, 2019
176:  implement RFCs 147 and 155, fix #141, etc. r=japaric a=japaric

This PR:

- Implements RFC 147: "all functions must be safe"

- Implements RFC 155: "explicit Context parameter"

- Implements the pending breaking change #141: reject assign syntax in `init`
  (which was used to initialize late resources)

- Refactors code generation to make it more readable -- there are no more random
  identifiers in the output -- and align it with the book description of RTFM
  internals (see PR #175).

- Makes the framework hard depend on `core::mem::MaybeUninit` and thus will
  require nightly until that API is stabilized.

- Fixes a ceiling analysis bug where the priority of the system timer was not
  considered in the analysis (TODO backport this into the v0.4.x branch).

- Shrinks the size of all the internal queues by turning `AtomicUsize` indices
  into `AtomicU8`s.

- Removes the integration with `owned_singleton`.

closes #141
closes #147
closes #155

Additionally:

- This changes CI to push v0.5.x docs to
  https://japaric.github.io/rtfm5/book/en/ -- we need to do this because our
  official docs are hosted on https://japaric.github.io/cortex-m-rtfm and we
  need to keep them on v0.4.x until we release v0.5.0

- I propose that we use the master branch to develop the upcoming v0.5.0.

- I have created a branch v0.4.x for backports; new v0.4.x releases will come
  from that branch.

r? @korken89 @TeXitoi, sorry for doing all the impl work in a single commit --
I know that makes things harder to review for you.

Suggestions for compile-pass and compile-fail tests are welcome


Co-authored-by: Jorge Aparicio <jorge@japaric.io>
@bors bors bot closed this as completed in #176 May 1, 2019
japaric added a commit that referenced this issue May 8, 2019
note that this assumes that RFC #155 has been implemented
bors bot added a commit that referenced this issue May 9, 2019
175: document internals r=japaric a=japaric

note that this assumes that RFC #155 has been implemented

[Rendered text](https://japaric.github.io/rtfm5/book/en/internals.html)

Do not merge this before PR #176

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
@placrosse
Copy link

placrosse commented Jun 3, 2019

I know this comment is "late in the game", but, since 0.5.0 is not yet released, here goes.

Try to convert a relatively small RTFM app, such as
https://github.com/jonas-schievink/rubble/tree/master/rubble-demo

Take a look at the "before and after" and answer honestly if, in practice, this made an improvement? If you enjoy the verbosity of (original) Java, with its duplicate type information specified everywhere, perhaps. The new Context really, in most cases, seems to be repetitive information from the required annotations.

More noise, without more expressiveness. It made RTFM "less beautiful", IMHO

@perlindgren
Copy link
Collaborator

Hi placrosse

The main reason is to make the task signature follow the Rust lang/syntax. If you want to modularise the code, e.g., use the task merely as a trampoline while doing the work somewhere else (like a lib), then its easier to get the signature of the library function right, rather than to guess what the signature should be.

We have experimented back and forth with the API/syntax of the DSL and I/we believe the extra "noise" is well worth it. Rust is extremely noisy in comparison to e.g., OCaml, but the advantage with Rust that is that the explicit signatures makes it clear under which context a function evaluates, and we just pick up on that. (In an older version of RTFM we had the explicit signatures, we removed them in v4 (I even argued for further simplifications, but that would have implied potential name clashes) , and now they are back in v5 as we found the extra clarity is overall beneficial... you just write code once, but read it many times).

Maybe some smart "templating" in an IDE could fill out the signatures, given the task specification?

/Per

@placrosse
Copy link

Thank you for your response, Per. I understand what you are saying, and truly appreciate the dialog!

I compared RTFM 0.3 and 0.4, along with 0.5. The difference is the addition of the annotations immediately above the function signature. This was not present in 0.3, with the information about resources, spawns, etc., away from the function, further up in the app macro. In 0.4, this specification moved to be immediately by the function. While reading, the annotated function signature (at least to me) indicates exactly the information required. My "rant" above was from the experience of converting the Rubble demo app to RTFM 0.5. It felt very "boilerplate". In the case of convenience for trampoline use, though, I agree fully that the experience is the exact opposite, with everything automatically packaged together for you. As I make further use of RTFM, I'm certain I'll appreciate this.

Thank you for your work on this framework; it's exciting to see Rust start to mature in the embedded space.

bors bot added a commit to quartiq/stabilizer that referenced this issue Nov 15, 2019
53: build(deps): bump cortex-m-rtfm from v0.5.0-beta.1 to v0.5.0 r=jordens a=dependabot-preview[bot]

Bumps [cortex-m-rtfm](https://github.com/rtfm-rs/cortex-m-rtfm) from v0.5.0-beta.1 to v0.5.0.
<details>
<summary>Changelog</summary>

*Sourced from [cortex-m-rtfm's changelog](https://github.com/rtfm-rs/cortex-m-rtfm/blob/master/CHANGELOG.md).*

> ## v0.5.0 - 2019-11-14
> 
> ### Added
> 
> - Experimental support for homogeneous and heterogeneous multi-core
>   microcontrollers has been added. Support is gated behind the `homogeneous` and
>   `heterogeneous` Cargo features.
> 
> ### Changed
> 
> - [breaking-change][] [RFC 155] "explicit `Context` parameter" has been
>   implemented.
> 
> [RFC 155]: [rtic-rs/rtic#155](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/155)
> 
> - [breaking-change][] [RFC 147] "all functions must be safe" has been
>   implemented.
> 
> [RFC 147]: [rtic-rs/rtic#147](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/147)
> 
> - All the queues internally used by the framework now use `AtomicU8` indices
>   instead of `AtomicUsize`; this reduces the static memory used by the
>   framework.
> 
> - [breaking-change][] when the `capacity` argument is omitted, the capacity of
>   the task is assumed to be `1`. Before, a reasonable (but hard to predict)
>   capacity was computed based on the number of `spawn` references the task had.
> 
> - [breaking-change][] resources that are appear as exclusive references
>   (`&mut-`) no longer appear behind the `Exclusive` newtype.
> 
> - [breaking-change][] the `timer-queue` Cargo feature has been removed. The
>   `schedule` API can be used without enabling any Cargo feature.
> 
> - [breaking-change][] when the `schedule` API is used the type of
>   `init::Context.core` changes from `cortex_m::Peripherals` to
>   `rtfm::Peripherals`. The fields of `rtfm::Peripherals` do not change when
>   Cargo features are enabled.
> 
> - [breaking-change][] the monotonic timer used to implement the `schedule` API
>   is now user configurable via the `#[app(monotonic = ..)]` argument. IMPORTANT:
>   it is now the responsibility of the application author to configure and
>   initialize the chosen `monotonic` timer during the `#[init]` phase.
> 
> - [breaking-change][] the `peripherals` field is not include in `init::Context`
>   by default. One must opt-in using the `#[app(peripherals = ..)]` argument.
> 
> - [breaking-change][] the `#[exception]` and `#[interrupt]` attributes have been
>   removed. Hardware tasks are now declared using the `#[task(binds = ..)]`
>   attribute.
></tr></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`6b0a2df`](rtic-rs/rtic@6b0a2df) Merge [#272](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/272)
- [`4fcb6ab`](rtic-rs/rtic@4fcb6ab) v0.5.0 final release
- [`e28294b`](rtic-rs/rtic@e28294b) Merge [#271](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/271)
- [`2441b7e`](rtic-rs/rtic@2441b7e) Minor docs update to monotonic
- [`85463ed`](rtic-rs/rtic@85463ed) Merge [#268](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/268) [#270](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/270)
- [`76e2345`](rtic-rs/rtic@76e2345) Added struct de-structure-ing example in tips & tricks
- [`e9a8394`](rtic-rs/rtic@e9a8394) fix bash comparison
- [`da9c6a7`](rtic-rs/rtic@da9c6a7) run cfail tests only when rustc --version == $MSRV
- [`31b392f`](rtic-rs/rtic@31b392f) CI: replace compiletest-rs with trybuild
- [`72e84cb`](rtic-rs/rtic@72e84cb) Merge [#266](https://github-redirect.dependabot.com/rtfm-rs/cortex-m-rtfm/issues/266)
- Additional commits viewable in [compare view](rtic-rs/rtic@1fe9767...6b0a2df)
</details>
<br />

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
andrewgazelka pushed a commit to andrewgazelka/cortex-m-rtic that referenced this issue Nov 3, 2021
155: ci: test on stable r=therealprof a=japaric

now that 1.31 is out

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC This issue needs you input! S-accepted This RFC has been accepted but not yet implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants