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] opt-in Peripherals steal #201

Closed
japaric opened this issue Jun 14, 2019 · 10 comments
Closed

[RFC] opt-in Peripherals steal #201

japaric opened this issue Jun 14, 2019 · 10 comments
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 Jun 14, 2019

Current behavior

The init::Context struct includes a device field whose type is
#device::Peripherals, a singleton that's a collection of all the device
peripherals.

Proposal

Don't include this device field by default. Add a peripheral argument to the
#[rtfm::app] attribute that lets the user opt into today's behavior.

Rationale

#device::Peripherals is a singleton; in multi-core mode only one core can
take all the peripherals so it's necessary to specify which core will take the
singleton.

Detailed design

The #[rtfm::app] attribute will gain a peripherals argument whose semantics
varies between single-core and multi-core mode.

Single-core mode

In single core mode the argument takes a boolean that indicates whether the
device peripherals will be made available during the init function. When the
peripherals argument is omitted the peripherals will not be made available.

// NOTE omitting `peripherals` also means `peripherals = false`
#[rtfm::app(device = pac, peripherals = false)]
const APP: () = {
    #[init]
    fn init(c: init::Context) {
        // let device: pac::Peripherals  = c.device;
        //~^ error: no field named `device` in this struct
    }
};
#[rtfm::app(device = pac, peripherals = true)]
const APP: () = {
    #[init]
    fn init(c: init::Context) {
        // OK
        let device: pac::Peripherals  = c.device;
    }
};

Multi-core mode

In multi-core mode the peripherals argument takes a integer that indicates
which core will take the device peripherals during init. If the argument is
omitted then the peripherals will not be taken by the framework.

#[rtfm::app(cores = 2, device = pac, peripherals = 1)]
const APP: () = {
    #[init(core = 0)]
    fn init(c: init::Context) {
        // let device: pac::Peripherals  = c.device;
        //~^ error: no field named `device` in this struct
    }

    #[init(core = 1)]
    fn init(c: init::Context) {
        // OK -- this core takes all peripherals
        let device: pac::Peripherals  = c.device;
    }
};

Alternatives

In single-core mode, should the default be to have RTFM take the peripherals?
That is peripherals would default to true matching today's behavior and one
would have to opt-out by writing peripherals = false.

One could design a syntax to split the peripherals between the many init
functions in multi-core mode but this can't be implemented today's without extra
help from svd2rust so this proposal doesn't delve into that question.

cc @hannobraun #163

@japaric japaric added the RFC This issue needs you input! label Jun 14, 2019
@japaric japaric added this to the v0.5.0 milestone Jun 14, 2019
@hannobraun
Copy link

Thanks for the heads-up! This looks sensible to me, and would solve the problem I reported in #163.

@TeXitoi
Copy link
Collaborator

TeXitoi commented Jun 14, 2019

Is device = pac needed for other thing that peripherals?

@japaric
Copy link
Collaborator Author

japaric commented Jun 19, 2019

@TeXitoi the framework internally uses ${device}::Interrupt::${binds} as an argument to NVIC::{pend,enable,set_priority} calls and to check that the ${binds} interrupt actually exists on the target device.

@korken89
Copy link
Collaborator

I agree with the addition, but not with the default.
I'd argue to keep the old default (peripherals = true) to maximize backwards compatibility and keep it as users expect it.

@japaric
Copy link
Collaborator Author

japaric commented Jun 29, 2019

@korken89 the multi-core default is to not take the peripherals, though, so in a way the two defaults wouldn't match if we go with peripherals = true on single-core. Alternatively, we could default to peripherals = 0 on multi-core and use peripherals = false to not take the peripherals; that would sort of "match" with peripherals = true on single-core.

I do see how peripherals = true is good for backward compatibility (though we are already breaking plenty of things in v0.5) but have no strong opinions on what the defaults should be.

@TeXitoi any thoughts on what the defaults should be?

Modulo the defaults I think we have consensus on the rest of the RFC so I'm going to FCP this with disposition to merge. Let's solve the question about the defaults during the FCP.

@TeXitoi
Copy link
Collaborator

TeXitoi commented Jun 29, 2019

I don't like default values to be true. That's much more intuitive to have false when you don't set something. So my preference is to false even if that's the most common choice.

OK for me for the rest.

@japaric
Copy link
Collaborator Author

japaric commented Jun 29, 2019

@TeXitoi True (no pun intended). The Default implementation of bool returns false and you can think of the multi-core peripheral as being an Option<u8> that defaults to None.

@korken89
Copy link
Collaborator

korken89 commented Jul 2, 2019

Technically I agree, from the old usage patterns I do not agree.
But I do not have any strong opinion here, so LGTM.

@japaric
Copy link
Collaborator Author

japaric commented Jul 8, 2019

🎉 This RFC with the single-core default of false has been formally approved. Implementation is in PR #205 (I'm going to keep this open until the PR lands)

@japaric japaric added S-accepted This RFC has been accepted but not yet implemented and removed disposition-merge labels Jul 8, 2019
bors bot added a commit that referenced this issue Sep 15, 2019
205: rtfm-syntax refactor + heterogeneous multi-core support r=japaric a=japaric

this PR implements RFCs #178, #198, #199, #200, #201, #203 (only the refactor
part), #204, #207, #211 and #212.

most cfail tests have been removed because the test suite of `rtfm-syntax`
already tests what was being tested here. The `rtfm-syntax` crate also has tests
for the analysis pass which we didn't have here -- that test suite contains a
regression test for #183.

the remaining cfail tests have been upgraded into UI test so we can more
thoroughly check / test the error message presented to the end user.

the cpass tests have been converted into plain examples

EDIT: I forgot, there are some examples of the multi-core API for the LPC541xx in [this repository](https://github.com/japaric/lpcxpresso54114)

people that would like to try out this API but have no hardware can try out the
x86_64 [Linux port] which also has multi-core support.

[Linux port]: https://github.com/japaric/linux-rtfm

closes #178 #198 #199 #200 #201 #203 #204 #207 #211 #212 
closes #163 
cc #209 (documents how to deal with errors)

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
bors bot added a commit that referenced this issue Sep 15, 2019
205: rtfm-syntax refactor + heterogeneous multi-core support r=japaric a=japaric

this PR implements RFCs #178, #198, #199, #200, #201, #203 (only the refactor
part), #204, #207, #211 and #212.

most cfail tests have been removed because the test suite of `rtfm-syntax`
already tests what was being tested here. The `rtfm-syntax` crate also has tests
for the analysis pass which we didn't have here -- that test suite contains a
regression test for #183.

the remaining cfail tests have been upgraded into UI test so we can more
thoroughly check / test the error message presented to the end user.

the cpass tests have been converted into plain examples

EDIT: I forgot, there are some examples of the multi-core API for the LPC541xx in [this repository](https://github.com/japaric/lpcxpresso54114)

people that would like to try out this API but have no hardware can try out the
x86_64 [Linux port] which also has multi-core support.

[Linux port]: https://github.com/japaric/linux-rtfm

closes #178 #198 #199 #200 #201 #203 #204 #207 #211 #212 
closes #163 
cc #209 (documents how to deal with errors)

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

japaric commented Sep 15, 2019

Done in PR #205

@japaric japaric closed this as completed Sep 15, 2019
andrewgazelka pushed a commit to andrewgazelka/cortex-m-rtic that referenced this issue Nov 3, 2021
201: Add binary for armv8 hardfloat r=korken89 a=aurabindo

Fixes build for `thumbv8m.main-none-eabihf`

Co-authored-by: Aurabindo Jayamohanan <mail@aurabindo.in>
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

No branches or pull requests

4 participants