Remove bare-metal dependency from generated crates

[therealprof]

The only dependency on bare-metal was the implementation of the Nr
trait which has been removed from version 1.0.0
(rust-embedded/bare-metal#32).

Closes #453

Signed-off-by: Daniel Egger [email protected]

[rust-highfive]

r? @Disasm

(rust_highfive has picked a reviewer for you, use r? to override)

[adamgreig]

We still want the Interrupt enum on Cortex-M to impl cortex_m::interrupt::Nr, otherwise it can't be used with the cortex-m NVIC methods. The difference is moving to an Nr trait defined by each architecture crate instead of defined in bare-metal, at least as I understood it.

[therealprof]

We still want the Interrupt enum on Cortex-M to impl cortex_m::interrupt::Nr, otherwise it can't be used with the cortex-m NVIC methods. The difference is moving to an Nr trait defined by each architecture crate instead of defined in bare-metal, at least as I understood it.

I see, so we need architecture dependent implementations instead? However I don't see those yet on the horizon, even cortex-m doesn't seem to have that yet: https://github.com/rust-embedded/cortex-m/blob/b70c25a9b887fc19ce94a9789a34c1a036186c76/src/interrupt.rs#L5

I have no problem keeping this open for now but we should come up with a definitive plan how we're going to adress that.

[adamgreig]

I don't think this is a problem for svd2rust; we simply use cortex_m::interrupt::Nr and impl Nr as before; when cortex-m swaps to bare-metal 1.0 it will add its own Nr trait instead of exporting the bare_metal one, and svd2rust PACs will eventually update to use cortex-m 1.0 without needing any other Nr-related changes. I expect the same thing to work out for the other architectures.

For today, we should be able to just start using the re-exported Nr available in each architecture crate instead of the bare-metal one.

[therealprof]

I don't think this is a problem for svd2rust; we simply use cortex_m::interrupt::Nr and impl Nr as before

Isn't that a contradiction? If the generated PACs need to use cortex_m::interrupt::Nr that is a problem for svd2rust since it needs to generate the code to do exactly that.

So if I'm reading you right the change needs to be redone to use the architecture specific re-export of interrupt::Nr?

Alternatively, couldn't the architecture specific crates implement this trait themselves if we're already defining it there? The big thing which wasn't available in the beginning but was added later is the #[repr(u8)] on the enum so it is actually possibly to externally figure out the associated interrupt numbers by simply casting to u8 (which is also how Nr is implemented right now). Or even get rid of the Nr trait and let the routines take an interrupt by enum kind instead of something with a bound to the Nr trait and cast internally.

[adamgreig]

Sorry, I meant it's not a problem for svd2rust since it's possible for svd2rust to generate PACs which use the arch crate Nr trait and implement it, which will then keep working with the arch crate interrupt methods.

You're right though, we could also change the arch crates to not use Nr in their methods and instead take raw u8s, or somehow take an Interrupt enum (but how would they get the right type?). I think one of the reasons to split this is that some architectures may need a larger type than a u8 to identify the interrupt. Removing Nr was @jonas-schievink's change, perhaps he remembers what we intended to do in the arch specific crates?

[therealprof]

Uhm, well the signature of Nr was:

pub unsafe trait Nr {
    /// Returns the number associated with an interrupt.
    fn nr(&self) -> u8;
}

so basically up to recently you only had three options to use that anyway:

1. Take a the interrupt number by u8 directly and let someone else use the nr method to get that (or in more recent versions simply cast it to u8)
2. Have your function take a T: Nr parameter and call nr yourself on that
3. Have your function take a parameter of PAC-specific enum type Interrupt and cast to u8

Option 3 would still work and seems the most natural to me. We could even change the repr in the PAC to something else than u8 if required and things would still work since only the architecture needs to know what to cast to. I do think using the interrupt name is most natural anyway (unless of course someone decides to not name them so we have to come up with names like Interrupt116 😅).

[Disasm]

Looks good to me, however highfive still thinks I'm in the tools team :)

[adamgreig]

Have your function take a parameter of PAC-specific enum type Interrupt and cast to u8

How can cortex_m::peripheral::NVIC::request<I: Nr>(interrupt: I) use the PAC-specific enum type?

[therealprof]

Have your function take a parameter of PAC-specific enum type Interrupt and cast to u8

How can cortex_m::peripheral::NVIC::request<I: Nr>(interrupt: I) use the PAC-specific enum type?

No idea yet, marker trait?. OTOH I just checked and the only place where the ominous Nr trait is even used is https://github.com/rust-embedded/cortex-m/blob/master/src/peripheral/nvic.rs which makes it rather dubious. Instead of having such a unprotected trait we could as well use something like cortex_m::peripheral::NVIC::request<I: Into<u8>>(interrupt: I) which has the added benefit of allowing someone to pass an interrupt number directly rather than having to crate a newtype and implement Nr on that.

[adamgreig]

Wouldn't a marker trait just be Nr, defined in cortex-m and implemented for enum Interrupt by svd2rust in the PAC?

we could as well use something like cortex_m::peripheral::NVIC::request<I: Into>(interrupt: I)

This seems fine too, though I don't know if allowing users to pass a u8 is an "advantage" when they "should" be passing an Interrupt variant from their PAC.

[therealprof]

Wouldn't a marker trait just be Nr, defined in cortex-m and implemented for enum Interrupt by svd2rust in the PAC?

I'd define the marker trait in the PAC and leave out the useless nr method.

This seems fine too, though I don't know if allowing users to pass a u8 is an "advantage" when they "should" be passing an Interrupt variant from their PAC.

Well, the Nr trait does not provide that guarantee anyway since it is public. Nr is just a bespoke Into<u8>. With the obvious disadvantage that whenever u8 doesn't cut the mustard for an architecture anymore, the Nr trait (as-is) would be working against us.

[adamgreig]

To summarise the discussion from today's meeting about this, we decided:

• Arch crates such as cortex-m will define a new InterruptNumber trait
• The trait will be unsafe as it has certain preconditions required for memory safety: each variant in an enum that implements the trait must map to a unique integer and must always map to the same integer, to ensure nested interrupt enable/disable calls with the same variant perform the correct action
• svd2rust will generate implementations of these arch-specific crates for each supported arch: the implementations will probably look just like the current Nr implementation

This is a breaking change across the bare-metal/arch crate/svd2rust/PAC ecosystem so we'll have to manage it with the various 1.0 releases we have planned.

[therealprof]

As discussed in todays meeting this is blocked on a cortex-m release. Once that is done we'll move over the ARM target from bare-metal to cortex-m and keep other targets on the old bare-metal for the time being.

[adamgreig]

As discussed in todays meeting this is blocked on a cortex-m release. Once that is done we'll move over the ARM target from bare-metal to cortex-m and keep other targets on the old bare-metal for the time being.

Additionally, it sounds like msp430 doesn't use the Nr implementation at all, so we can probably remove it without replacement.

[cr1901]

My proposal for MSP430 is to:

• Remove the Nr trait.

• Keep the Interrupt enum, and change the docstring of Interrupt to the following to signify intent:

  #[doc = r"Enumeration of all the interrupts. This enum is seldom used in application or library crates. It is present primarily for documenting the device's implemented interrupts."]
  

[therealprof]

From this weeks discussion: Didn't get to it, will look into making the changes for MSP430 and Cortex-M soon, RISC-V is still pending on input from the team.

[Disasm]

Same for RISC-V I think: remove the Nr trait, keep the Interrupt enum. One can always use as u8 instead of .nr().

[therealprof]

Well, for MSP430 we're only keeping it for documentation purposes. For Cortex-M they're actually used and in general the assumption would be that it makes sense to copy what Cortex-M is doing...

[Disasm]

The Interrupt struct is useful for RISC-V, because interrupt controller operates with interrupt numbers, so we need to convert interrupt names and numbers back and forth.

[therealprof]

@Disasm The question was not about the Interrupt enum but about the replacement or necessity of the Nr implementation. As mentiond in new code https://github.com/rust-embedded/cortex-m/blob/master/src/interrupt.rs#L5-L24 (PR: rust-embedded/cortex-m#241) and #455 (comment) as well as in the meeting the conclusion was that it might be helpful to not use the conversion to a plain number directly but have a new unsafe trait and document the invariants which must be upheld.

[Disasm]

@therealprof Thanks for clarification. Indeed, we could do something like this for RISC-V, but right at the moment such Nr-like trait is useless for this arch. All interrupt controllers in RISC-V chips are different, even those that are "compliant". Because of this, there is no common driver for these controllers and no need for a separate trait on riscv/riscv-rt level.

[therealprof]

I think we can close this, right?