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] Change the cortex-m-rt attribute syntax #407

Open
jonas-schievink opened this issue Jan 11, 2020 · 5 comments
Open

[RFC] Change the cortex-m-rt attribute syntax #407

jonas-schievink opened this issue Jan 11, 2020 · 5 comments

Comments

@jonas-schievink
Copy link
Contributor

Summary

Change the syntax of interrupt and exception handlers provided by cortex-m-rt to be more obvious,
more internally consistent, and closer to RTFM's:

  • Resource Syntax: Resources are declared as function arguments of type &mut T, with the initial value provided via an #[init(<expr>)] attribute.
  • Handler Names: Handler macros are now invoked with the exception name as an argument (eg. #[exception(SVCall)]), instead of using the handler function name as the interrupt/exception to handle.
  • Handler Arguments:
    • When declaring a DefaultHandler, the IRQ number can be requested by attaching #[irqn] to an i16 function argument. Currently the handler signature is required to take exactly one i16 argument.
    • Exception handlers other than DefaultHandler can access the exception frame by attaching #[exception_frame] to an argument of type &cortex_m_rt::ExceptionFrame.

Motivation

Resource Syntax

The biggest motivation for changing the syntax used by cortex-m-rt is to improve the way handler resources work.

Currently, handlers defined via cortex-m-rt's #[interrupt], #[exception] and #[entry] macros
can declare resources owned by them by declaring a list of static mut items as their first
statements. The procedural macros then transform them to function arguments, and their types are
changed from T to &mut T. This comes with a few drawbacks:

  • It silently modifies Rust language semantics, which is surprising and can make communication about the syntax more difficult.
  • It does not work consistently: Resources must be the first N statements inside the handler. Other static mut items in the function are just regular static muts. While this is potentially fixable, it still leaves static mut meaning something very different in handler functions than in any other function.
  • Its implementation requires modifying the function signature, making the function not callable from other Rust code as expected.

Here is an example that shows the shortcomings of the current syntax:

#[exception]
fn SVCall() {
    static mut RESOURCE: u8 = 123;

    // Type of `RESOURCE` is `&mut u8` here

    ();

    static mut RESOURCE2: u8 = 42;

    // Type of `RESOURCE2` is `u8` (it's a normal `static mut` item, so any access is `unsafe`)
}

// `SVCall` cannot be called by normal Rust code, as it was transformed (and renamed) by the macro:
fn somewhere_else() {
    SVCall();
}
// error[E0425]: cannot find function, tuple struct or tuple variant `SVCall` in this scope
//   --> examples/minimal.rs:16:5
//    |
// 16 |     SVCall();
//    |     ^^^^^^ not found in this scope

This RFC proposes a new way of declaring resources below, which should address all of these issues.

Handler Names

Another minor issue with the current implementation is that the name of the handler function determines the exception or interrupt that is handled:

#[interrupt]
fn USART1() {
    // ...
}

This forces the user to write an unidiomatic function name containing upper-case letters, and which
might not describe the purpose of the handler very clearly.

For example, an MCU might have 3 USARTs with dedicated interrupts, each used to handle a different external peripheral. In such an application it would be clearer to call those handlers gps_rx, modem_rx, and user_rx instead of USART0, USART1 and USART2.

Handler Arguments

Finally, two exception handlers, DefaultHandler and HardFault, are special in that they take an
argument of type i16 and &ExceptionFrame, respectively. The &ExceptionFrame in particular is
provided by an assembly shim that cannot be disabled.

This RFC proposes a way to add the &ExceptionFrame (actually, &mut ExceptionFrame) to any
exception handler, and makes it optional for HardFault. The i16 exception number passed to
DefaultHandler is also made optional.

How handlers are expanded now

Note how the code for declaring an SVCall exception handler is currently transformed by the
#[exception] attribute:

#[exception]
fn SVCall() {
    static mut RESOURCE: u8 = 123;

    ();

    static mut RESOURCE2: u8 = 42;
}

This code becomes:

#[export_name = "SVCall"]
pub unsafe extern "C" fn __cortex_m_rt_SVCall_trampoline() {
   __cortex_m_rt_SVCall({
       static mut RESOURCE: u8 = 123;
       &mut RESOURCE
   })
}

fn __cortex_m_rt_SVCall(RESOURCE: &mut u8) {
    ();

    static mut RESOURCE2: u8 = 42;
}

The proposed syntax will embrace this transformation instead of hiding it.

Detailed design

To start off, this is how the above code would be written with the syntax proposed by this RFC:

#[exception(SVCall)]
fn svc_handler(
    #[init(123)]
    resource: &mut u8,
) {
    ();

    static mut RESOURCE2: u8 = 42;
}

It expands to the following code:

#[export_name = "SVCall"]
pub unsafe extern "C" fn __cortex_m_rt_SVCall_trampoline() {
    svc_handler({
        static mut RESOURCE: u8 = 123;
        &mut RESOURCE
    })
}

fn svc_handler(
    resource: &mut u8,
) {
    ();

    static mut RESOURCE2: u8 = 42;
}

Notably, the user-defined function stays as-is and is not transformed at all. The type of the
resource doesn't get silently changed, and the name of the function can be freely chosen.

In addition to #[init], resource arguments also accept the following built-in Rust attributes:
#[export_name], #[link_section], #[no_mangle], #[used]. When these are encountered, they
will be applied to the generated static item in the trampoline, allowing fine-grained control
over the allocated memory.

Like now, resources declared on the #[entry] handler can be given 'static lifetime, while other
handlers are restricted to elided lifetimes to ensure soundness. This check now has to be done in
the macro implementation by inspecting the parameter type. Named lifetime parameters are not
permitted in the parameter type. So a parameter of type &'a mut T would be rejected, while
&mut Type<'static> would be allowed.

Exception Frame access

Currently, the HardFault handler is required to take an immutable &ExceptionFrame. The proposed syntax makes that optional and opt-in.

To obtain an &ExceptionFrame with the new syntax, the #[exception_frame] attribute has to be
placed on the handler argument.

Another possible improvement to the &ExceptionFrame API is described in issue #234. While it is
also a breaking change, it is orthogonal to the syntax changes proposed in this RFC.

A more complicated example

#[exception(DefaultHandler)]
fn default(
    #[init(false)]
    flag: &mut bool,

    #[irqn]
    irq: i16,
) {
    *flag = true;
    hprintln!("Unhandled IRQ #{}", irq).ok();
}

#[exception(HardFault)]
fn fault(
    #[exception_frame]
    frame: &ExceptionFrame,

    #[init(0)]
    fault_count: &mut u32,
) {
    *fault_count += 1;
    hprintln!("HardFault: {:?}", frame).ok();
}

This will get expanded to:

#[export_name = "DefaultHandler"]
pub unsafe extern "C" fn __cortex_m_rt_DefaultHandler_trampoline() {
    // ... fetch IRQN like it does now ...
    let irqn = ...;

    default({
        static mut FLAG: bool = false;
        &mut FLAG
    }, {
        irqn
    })
}

fn default(
    flag: &mut bool,
    irq: i16,
) {
    *flag = true;
    hprintln!("Unhandled IRQ #{}", irq).ok();
}

#[export_name = "HardFault"]
pub unsafe extern "C" fn __cortex_m_rt_HardFault_trampoline(ef: &ExceptionFrame) {
    // The `ef` argument is provided by the ASM trampoline, like before.

    svc_handler({
        ef
    }, {
        static mut FAULT_COUNT: u32 = 0;
        &mut FAULT_COUNT
    })
}

fn fault(
    frame: &ExceptionFrame,
    fault_count: &mut u32,
) {
    *fault_count += 1;
    hprintln!("HardFault: {:?}", frame).ok();
}

How We Teach This

Most of the API documentation in cortex-m-rt will have to be rewritten to reflect these changes.

The Book also makes use of cortex-m-rt, mostly in the Exceptions and Interrupts chapters.
These will likewise be updated to reflect the changes.

The cortex-m-quickstart repository will be
updated to the new version of cortex-m-rt once this change has been released. Other repositories
and HALs will follow.

With the proposed changes, cortex-m-rt should generally become easier to teach, since it moves
things closer to RTFM and normal Rust.

Drawbacks

This is a breaking change. The proposed syntax is completely incompatible to the current syntax. Any users of cortex-m-rt will have to migrate.

Alternatives

  • Do nothing and keep the old syntax.
  • Only do a subset of the changes proposed here (eg. leave out the #[exception(<name>)] change and continue using the function name to determine the exception).
  • Adopt an entirely different syntax

Unresolved questions

As is tradition with syntactic changes, a large amount of bikeshedding is expected.

The syntax of the #[exception] attribute could be adjusted to match RTFM's more closely:
#[exception(binds = ExceptionName)] instead of #[exception(ExceptionName)]. This syntax would also be more extensible: New arguments beside binds could be added easily without changing the rest of the syntax.

We could completely remove the support for the irqn argument for DefaultHandler. The number of the currently handled IRQ can be fetched at any time by using SCB::vect_active() (though this API could use some improvement).

@therealprof
Copy link
Contributor

I haven't thought about all of it yet but a few points:

  • Resources must the the first N statements inside the handler.

    Typo

  • Its implementation requires modifying the function signature, making the function not callable from other Rust code as expected.

    My understanding is that this is a feature, not a bug.

  • Handler Names

    So basically you want to go back to something more akin to the original interrupt handlers?

  • #[init(123)]
    

    Are macros on function arguments even possible?

@jonas-schievink
Copy link
Contributor Author

Typo fixed, thanks.

  • Its implementation requires modifying the function signature, making the function not callable from other Rust code as expected.

    My understanding is that this is a feature, not a bug.

This does make sense when viewing an #[exception] function as "this function is the exception handler". I'd like to move this more towards "this functions gets called when this exception happens", which requires less bending of Rust's semantics (it's just a normal function).

  • Handler Names

    So basically you want to go back to something more akin to the original interrupt handlers?

Well, it looks like they do share this property, yes. But the old handlers were still using macro! syntax, which fits them very poorly IMO (the attribute syntax seems much better).

#[init(123)]

Are macros on function arguments even possible?

Yes, this should definitely be implementable as proposed.

@jonas-schievink
Copy link
Contributor Author

This also has the potential to make interrupt handler registration more robust. Currently it works by reexporting the interrupt enum under the same name as the #[interrupt] macro and hoping that the user doesn't rename or fully qualify it as #[some_crate::interrupt] (doing so breaks it and fails to compile with an obscure name resolution error).

With this RFC it would use a syntax like #[interrupt(MyInterruptEnum::INT0)] and could output code to typecheck MyInterruptEnum and register a handler for INT0. The syntax would not accept an unqualified enum variant, so doing use MyInterruptEnum::INT0; followed by #[interrupt(INT0)] wouldn't be accepted (but with a good error message). This also removes a lot of the magic behind this, and would remove the need for rust-embedded/cortex-m-rt#231 since the attribute from cortex-m-rt could be used directly.

(I haven't yet thought too deeply about this, but this seems like a nice improvement if it works out – of course with rust-embedded/cortex-m-rt#250 this would instead apply to some hypothetical porcelain crate)

@korken89
Copy link
Contributor

korken89 commented Mar 5, 2020

Great RFC, it is very well described!

However in my opinion we should not be extending the functionality of the cortex-m-rt crate in this way, I think this goes very much hand-in-hand with https://github.com/rust-embedded/cortex-m-rt/issues/250
I think rather than extending cortex-m-rt we should be aiming for making it a library only and users should not have to touch this low-level, unless writing porcelain.
I'd like to even go further and break out a lot of the current functionality as discussed in rust-embedded/cortex-m-rt#250 so that in the end we only have the helper crates and cortex-m-rt stops existing.

I think this is going to be an important discussion to have soon. And I do not think making cortex-m-rt closer to RTFM is the right answer. If you want something RTFM like, use RTFM - if you want some thing Arduino like, use the hypothetical adruino-procelain crate etc, so we have a clear distinction between plumbing and where porcelain begins - and not approach this middle land where cortex-m-rt becomes something in the middle.

To conclude, I argue that cortex-m-rt should not be extended in this way, we should look to rust-embedded/cortex-m-rt#250 and go towards the idea of "porcelain" crates so cortex-m-rt does not become the middle ground between library and porcelain.

@jonas-schievink
Copy link
Contributor Author

Now that rust-analyzer diagnoses unsafe operations outside of unsafe blocks or functions, the static mut transform causes spurious errors.

@adamgreig adamgreig transferred this issue from rust-embedded/cortex-m-rt Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants