Support for non-standard exception and interrupts (clean)

[romancardenas]

First PR resulting from #211

This PR presents the last version of riscv-pac. Namely, we have a new ExceptionNumber trait.
Also, the InterruptNumber trait now comes with two new marker traits: CoreInterruptNumber and ExternalInterruptNumber.
This will allow us to filter interrupt sources.

Another change is that the InterruptNumber trait now deals with usize instead of u16.
The motivation of this change is the RISC-V ISA specification (there is more information about this in #211)

As a side effect, I also updated the riscv-peripheral trait to use the new approach.

[romancardenas]

My only doubt is: should PriorityNumber and HartIdNumber traits work with usize?

I guess u8 and u16 are more than enough for these traits. The PLIC peripheral uses these data formats. However, it is better to make them usize and let each peripheral/library cast them to a smaller data type than adding a breaking change in the future if we eventually find out that the current data types are not enough.

Let me know what you think and I will update this PR accordingly.

[romancardenas]

In the end, I pushed all the changes of #211 to this PR, but with a cleaner commit history.

In my last commit, I also added three attribute-like macros in riscv-rt: core_interrupt, external_interrupt, and exception. These macros:

• Expect an argument with the path to the corresponding interrupt/exception source. This path is used to check at compile time that the item implements the riscv_pac::CoreInterruptNumber, riscv_pac::ExternalInterruptNumber, orriscv_pac::ExceptionNumber, respectively.
• Check that the signature of the function is correct. In interrupts, there must not be any input parameter. In exceptions, it may be an input parameter with an (mutable) reference to a TrapFrame.
• It uses the export_name label to set the name of the function to the name of the interrupt/exception source. With this, we can use a more Rustacean name for these functions (i.e., snake_case). Here are a few examples from the empty.rs example of riscv-rt:

use riscv_rt::{core_interrupt, entry, exception, external_interrupt};

use riscv::interrupt::{Exception, Interrupt}; // standard enums that implement the riscv-pac traits

#[core_interrupt(Interrupt::SupervisorTimer)]
unsafe fn supervisor_timer() -> ! {
    // do something here
    loop {}
}

// this won't compile, cause ExternalInterruptNumber trait is not implemented!
// #[external_interrupt(Interrupt::SupervisorSoft)]
// fn supervisor_soft() {
//     // do something here
//     loop {}
// }

#[exception(Exception::Breakpoint)]
unsafe fn breakpoint(_trap: &mut riscv_rt::TrapFrame) -> ! {
    // do something here
    loop {}
}

[rmsyn]

My only doubt is: should PriorityNumber and HartIdNumber traits work with usize?

I guess u8 and u16 are more than enough for these traits.

My recommendation would be to stick with usize, since the values read/written by CSRs are usize. Just a suggestion though, no hard preference either way.

[rmsyn]

Similar suggestion to the machine interrupt handler.

[romancardenas]

Same here, not sure about what is wrong with the current implementation

[rmsyn]

Same here, not sure about what is wrong with the current implementation

Nothing, on second inspection I saw the call to csrrs. I was mistaken before, and thought csrrw was called.

[rmsyn]

Some minor style suggestions, and a couple suggestions for not clobbering CSR fields in machine + supervisor nested interrupt handlers.

[romancardenas]

Ok, so the only thing that I miss is improving the pac_enum macro for custom exceptions, so it also implements the _dispatch_exception function following the same fashion.

I will also use usize for all the riscv-pac traits, I think it will simplify the code.

[romancardenas]

Sorry for the delay, I took a few days off to disconnect.

New stuff in riscv

the pac_enum macro now works for exceptions and interrupts properly. When used in a PAC, the macro will implement the corresponding trait and it will also add the dispatch_* function to work with riscv-rt. In the case of core interrupts, it also generates a vector table, gated under the v-trap feature.

New stuff in riscv-pac

All traits work with usize. Each party can then cast these numbers to the appropriate format for their target.

New stuff in riscv-rt

I added the exception, core_interrupt, and external_interrupt macros to help users define their trap handler functions. These macros expect an input argument with a path to a variant of an enum that must implement the corresponding riscv-pac trait. This is very useful to guarantee at compile time that there is in fact an interrupt/exception source with the name given by the caller.

Another small (but cool) change is that now the link section directive in start_trap_rust only applies to riscv targets. This change allows us to compile riscv-rt on native OSs for further testing (which is the next point).

New internal tests crate

I also wanted to add a few try-build test cases to show how the macros work (as well as having a more robust CI). However, try-build and examples do not get along well in the embedded world. Thus, I thought the easiest way to deal with this is by adding a new crate with all the tests that use try-build. We can sort test cases according to which crate we are testing (so far, I am only using riscv-rt in tests, but we can easily move the riscv test cases there).

Let me know what you think!

[rmsyn]

Is there a reason you decided to redefine Trap using generics for the interrupt and exception variants?

If this is to allow external users to use a custom Interrupt and Exception type, maybe we could also provide a top-level alias that defaults to the local Interrupt and Exception type?

Something like:

pub type DefaultTrap = Trap<Interrupt, Exception>;

Another option could be to call the generic:

pub enum GenericTrap<I, E> {
// ...
}

with the alias as:

pub type Trap = GenericTrap<Interrupt, Exception>;

With some notes in the docs, I think the custom implementation would be easy to figure out for those that want it.

Using the latter would minimize the effort of integrating the breaking changes, while still allowing external users to define their own Trap type for non-standard interrupts/exceptions.

[romancardenas]

Yes, the idea now is updating svd2rust so PACs do this kind of type binding in pac::interrupt. PACs could either re-export the Exception and CoreInterrupt enums that are in riscv or define their custom type. Also the interface with mcause can be adjusted to hide this complexity to final users.

I'll do a PoC and share it with you.

[romancardenas]

My PR for svd2rust is already updated and generates a proper pac::interrupt module.

You can check the outcome for the E310x chip here.

@rmsyn , this is perhaps the most interesting fragment of the module:

pub use riscv::{
    interrupt::{disable, enable, free, nested},
    CoreInterruptNumber, ExceptionNumber, HartIdNumber, PriorityNumber,
};
pub type Trap = riscv::interrupt::Trap<CoreInterrupt, Exception>;
#[doc = r" Retrieves the cause of a trap in the current hart."]
#[doc = r""]
#[doc = r" If the raw cause is not a valid interrupt or exception for the target, it returns an error."]
#[inline]
pub fn try_cause() -> riscv::result::Result<Trap> {
    riscv::interrupt::try_cause()
}
#[doc = r" Retrieves the cause of a trap in the current hart (machine mode)."]
#[doc = r""]
#[doc = r" If the raw cause is not a valid interrupt or exception for the target, it panics."]
#[inline]
pub fn cause() -> Trap {
    try_cause().unwrap()
}

The idea is "substituting"/adapting riscv::interrupt to the target. Hope it looks good to you!

[rmsyn]

The idea is "substituting"/adapting riscv::interrupt to the target. Hope it looks good to you!

Yes, everything looks awesome!

This is exactly what I was suggesting, except the location of the alias is up a level of abstraction.

I got a little confused when trying to directly upgrade while playing around with the code. Given the context of including these changes in svd2rust and generated pac crates, it makes sense to do the aliasing where you do.

Maybe for those using the riscv registers/functions directly, we could still provide an alias for the defaults inside that crate?

At least, maybe some doc-tests on the pub enum Trap<I, E> type explaining usage?

[romancardenas]

I added new docs for riscv::interrupt::Trap<I, E>. I also updated riscv-rt docs to push users to use the riscv_rt::{core_interrupt, exception, external_interrupt} macros instead of #[no_mangle]. Let me know what you think!

[rmsyn]

Let me know what you think!

LGTM. Awesome work @romancardenas

[rmsyn]

for the RISC-V ecosystem, it makes more sense to have separate core_interrupt and external_interrupt macros.

I agree, this clarifies how interrupts work in a RISC-V environment. Our docs on the separation can also help introduce the difference between the CLIC/CLINT (core-local) and PLIC (external) interrupt peripherals.

This took me some time to get my head around.

[romancardenas]

Let's wait a couple of days in case anyone wants to leave an additional review and then merge to master.

[thejpster]

How does this work for RP2350, where there is a need to have applications build for both Cortex-M and RISC-V, with no code changes?

[romancardenas]

I still have that target in my to-study list. But I think it should be compatible. Note that the idea is changing the enumeration of interrupts and exceptions available on a target at compile time. So there will not be run-time changes.

[thejpster]

The proposed syntax is this, right?

#[external_interrupt(ExternalInterrupt::UART)]
unsafe fn external_uart() -> ! {

}

That's completely unlike what cortex-m-rt does. Will it (cortex-m-rt) be changed to match, or will the two architectures just be different?

[thejpster]

Oh and to clarify I mean https://github.com/rp-rs/rp-hal/blob/main/rp235x-hal-examples/src/bin/blinky.rs has to build with both --target=thumbv8m.main-none-eabihf AND --target=riscv32imac-unknown-none-elf. And today https://github.com/rp-rs/rp-hal/blob/main/rp235x-hal-examples/src/bin/pwm_irq_input.rs does not because it uses #[interrupt].

[rmsyn]

Oh and to clarify I mean https://github.com/rp-rs/rp-hal/blob/main/rp235x-hal-examples/src/bin/blinky.rs has to build with both --target=thumbv8m.main-none-eabihf AND --target=riscv32imac-unknown-none-elf

Projects targetting that kind of architecture could feature-gate based on target, conditionally defining different trap handlers for each target.

Ostensibly, they would want to do that indepently of the changes in this PR, since there are different traps that need handling, right?

[romancardenas]

Will it (cortex-m-rt) be changed to match, or will the two architectures just be different?

I plan to open a PR in cortex-m to align the syntax with this.

[romancardenas]

Ostensibly, they would want to do that indepently of the changes in this PR, since there are different traps that need handling, right?

I guess that in this kind of scenario, you would be doing this like #[cortex_m_rt::interrupt] and #[riscv_rt::external_interrupt] depending on which target is supposed to handle the interrupt.

[thejpster]

cfg_attr I can live with. Different function signatures would be a problem.

[romancardenas]

Function signatures should be the same except for exception handlers that need access to the trap frame, as each platform has a different TrapFrame struct to save the context.

[romancardenas]

@adamgreig looks like @rmsyn 's review is still not eligible to unblock the merging

[adamgreig]

Sorry, should be fixed now.

[romancardenas]

If nobody opposes, I will merge this PR tomorrow by the end of the weekly meeting

[romancardenas]

Ok, it seems we cannot easily bypass clippy warnings right now. I will look, as the current setup is inconvenient for all the use cases. In the meantime, I fixed the new issues arising in riscv-semihosting.

I will add this PR to the merge queue so it is merged as soon as it gets a positive review.

[rmsyn]

In the meantime, I fixed the new issues arising in riscv-semihosting.

I'll wait until you merge the changes here, and rebase on top of the clippy fixes.