Closed romancardenas closed 1 month ago
This PR is being prevented from merging because it presents one of the blocking labels: work in progress, do not merge.
I think this is a good approach once it's clean and tested. I only had time to skim through it but don't see anything "wrong" with it.
mcause
and scause
I think the current proposal might become hard to handle for developers, as they now need to always work with generic arguments. I propose the following changes:
riscv::register::xcause
Interrupt
and Exception
Trap<I, E>
to:
pub enum Trap {
Interrupt(usize),
Exception(usize),
}
Xcause::cause
accordingly:
impl Mcause {
pub cause(&self) -> Trap { ...}
}
riscv::interrupt::{machine,supervisor}
Interrupt
and Exception
enums with the implementation of the riscv-pac
traits
pub use riscv_pac::{CoreInterruptNumber, InterruptNumber, ExceptionNumber};
/// Trap cause
pub enum Cause<I, E> { Interrupt(I), Exception(E), }
/// Trap cause error
pub enum CauseError { InvalidInterrupt(usize), InvalidException(usize), }
pub mod machine { use crate::register::{mcause, Trap}
pub fn cause<I: CoreInterruptNumber, E: ExceptionNumber>() -> Result<Cause<I, E>, CauseError> {
match mcause::read().cause() {
Trap::Interrupt(i) => match I::from_number(i) {
Ok(interrupt) => Ok(interrupt),
Err(code) => Err(CauseError::InvalidInterrupt(code)),
},
Trap::Exception(e) => match E::from_number(e) {
Ok(exception) => Ok(exception),
Err(code) => Err(CauseError::InvalidException(code)),
}
}
}
} ...
Naming can be improved. For instance, `Xcause` could return a `riscv::interrupt::Cause<usize, usize>` to avoid having too many similar enums.
Thanks for the feedback @rmsyn !
I implemented my alternative approach in the riscv-pac2
branch. Please take a look and let me know what you think. Personally, I feel like it is more user friendly, and somehow it makes sense to me that the Interrupt
and Exception
enums are in riscv::interrupt
instead of riscv::register
it makes sense to me that the Interrupt and Exception enums are in riscv::interrupt instead of riscv::register
That makes sense to me, too.
Really either one can work, because the values are read from the mcause
and scause
registers (which makes sense for riscv::register
).
riscv::interrupt
makes obvious sense because Interrupt
and Exception
are explicitly about interrupts.
:shrug:
I implemented my alternative approach in the riscv-pac2 branch.
After an initial review, I do like the code organization in riscv-pac2
more. Mostly aesthetic preferences, but it does feel like it clarifies things a bit.
The unification of one Trap
type is also nice from an ergonomics perspective regarding the mcause
/scause
registers.
So I think the new riscv
version looks good (still needs some testing etc.) BUT... it's time for riscv-rt
and the interrupt/exception handlers. We need to provide an out-of-the-box solution for dealing with custom interrupts and exceptions. I guess that it's time for thinking on macros that:
extern "C" fn
__INTERRUPTS
and __EXCEPTIONS
arrays from the enumsI found some time to keep working on this. In the new commit, the following code:
#[pac_enum(unsafe PriorityNumber)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Priority {
P0 = 0,
P1 = 1,
P2 = 2,
P3 = 3,
}
expands to:
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Priority {
P0 = 0,
P1 = 1,
P2 = 2,
P3 = 3,
}
unsafe impl riscv_pac::PriorityNumber for Priority {
const MAX_PRIORITY_NUMBER: u8 = 3;
#[inline]
fn number(self) -> u8 {
self as _
}
#[inline]
fn from_number(number: u8) -> Result<Self, u8> {
match number {
3 => Ok(Self::P3),
0 => Ok(Self::P0),
1 => Ok(Self::P1),
2 => Ok(Self::P2),
_ => Err(number),
}
}
}
On the other hand, the following code:
#[pac_enum(unsafe CoreInterruptNumber)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Interrupt {
I1 = 1,
I2 = 2,
I4 = 4,
I7 = 7,
}
Expands to:
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Interrupt {
I1 = 1,
I2 = 2,
I4 = 4,
I7 = 7,
}
unsafe impl riscv_pac::InterruptNumber for Interrupt {
const MAX_INTERRUPT_NUMBER: usize = 7;
#[inline]
fn number(self) -> usize {
self as _
}
#[inline]
fn from_number(number: usize) -> Result<Self, usize> {
match number {
2 => Ok(Self::I2),
1 => Ok(Self::I1),
4 => Ok(Self::I4),
7 => Ok(Self::I7),
_ => Err(number),
}
}
}
unsafe impl riscv_pac::CoreInterruptNumber for Interrupt {} // it also implements the marker trait
// It declares the existence of all the interrupt handlers
extern "C" {
fn I2();
fn I1();
fn I4();
fn I7();
}
// It generates the array with handlers
#[no_mangle]
pub static __CORE_INTERRUPTS: [Option<unsafe extern "C" fn()>; 7 + 1] = [
None,
Some(I1),
Some(I2),
None,
Some(I4),
None,
None,
Some(I7),
];
// It implements the interrupt dispatching function
#[no_mangle]
unsafe extern "C" fn _dispatch_core_interrupt(code: usize) {
extern "C" {
fn DefaultHandler();
}
if code < __CORE_INTERRUPTS.len() {
let h = &__CORE_INTERRUPTS[code];
if let Some(handler) = h {
handler();
} else {
DefaultHandler();
}
} else {
DefaultHandler();
}
}
It should work as is for direct mode. I still need to adapt this solution to vectored mode.
It already creates the interrupt vector table. However, I think we should discuss the new approach with the @rust-embedded/tools team to align svd2rust
with the new structure
Exciting times! I think the riscv
ecosystem will improve a lot with #222 and this PR.
I will wait until #222 is solved and merge the outcome, so we can adapt the work to the new approach.
In the meantime, I will keep updating my forks for svd2rust
so we have an out-of-the-box, easy-to-apply tool to help RISC-V PACs adopt our new ecosystem.
I updated this PR to use the new features coming from #222 .
An additional change is that I moved the macros to riscv
instead of riscv-pac
. As now riscv-pac
is fully re-exported into riscv
, it makes more sense, specially when leading with svd2rust
(I'm also working on that).
This branch is quite a mess. I will divide it into small PRs so it will be easier to review.
PS: Rust nightly is complaining about a bug in LLVM for inline assembly with x86 targets. I think it is a matter of time that the error message dissapears: https://github.com/rust-lang/rust/pull/127935
Rust nightly is complaining about a bug in LLVM for inline assembly with x86 targets
One potential way to solve that issue before the fix makes it into a nightly release could be to feature-gate all assembly with:
#[cfg(any(target_arch = "riscv32", target_arch = "riscv64"))]
...
The thing is that it still complains for RISC-V targets :/
The thing is that it still complains for RISC-V targets :/
What about switching the label to 1b:
, b1:
, or something similar? Not sure about any concern for label collision.
fixed
Closing this PR in favor of #223 , which has a cleaner commit history and cooler macros for defining interrupt and exception handlers
Currently, the
riscv
andriscv-rt
crates assume that the interrupt and exception sources are the ones gathered in the RISCV ISA. However, we do not support target-specific sources. Moreover, some RISC-V targets do not follow the standard interrupt scheme (e.g., ESP32 C3). Another example is the E310x microcontroller, which does not support supervisor interrupt sources.Proposal
Now that we have the
riscv-pac
trait to define interrupt and exception sources, we should be able to adapt the rest of the crates to rely on generic functions with constraints to these traits. For example, themcause
register can have something like this:By default,
I
andE
should be the standard enumerations (as we are doing now). Still, PACs will be able to opt out of this default implementation and provide their custom interrupt/exception enumeration.Next, we can enrich
riscv-rt
macros to automatically generate the necessary code for letting PACs inject their custom interrupt and exception tables. Also, in the case of vectored mode, we could generate the necessary vector table and so on.NOTE THAT THIS IS JUST MY PROPOSAL, but I would love to hear your opinion as well as clever mechanisms to implement this neatly.
I will do a self-review to point out some to-dos that I would like to discuss with you.
Solves #146