riscv-rust / e310x-hal

Implementation of the `embedded-hal` traits for E310x microcontrollers
17 stars 18 forks source link

New optional feature: PLIC external interrupt handling with virq #49

Closed steewbsd closed 1 year ago

steewbsd commented 1 year ago

This pull request adds a new optional feature: virq (vectorized interrupts). When enabled, any of the 52 PLIC external interrupts are automatically handled by an already provided MachineExternal function, which in turn calls the appropriate handler for the interrupt source. Every interrupt is redirected to a handler with a name equal to theirs, like shown below: GPIO4 -> GPIO4(). As every handler is declared as a weak symbol in interrupts.x, the user can override any of these 52 functions with a simple

#[no_mangle]
fn GPIO4() {
/* ... */
} 

which will override the default handler, thus providing a very simple way of customizing behaviors per-interrupt. An example usage of this feature is provided in the hifive1 repository, currently residing in a personal fork, virq.rs which will also be pull requested to the upstream repository. That pull request would be needed in order to use this feature, as the interrupts.x file provided is included from their hifive1-link.x file, as to not disrupt any kind of linker script dependency. As a side note, the GPIO0 as well as the PLIC peripherals would very much benefit from some abstractions to manage their respective priorities, such as having a clear_priorities function in PLIC, or an interrupt_enable() function in GPIO to enable GPIO interrupts. A further pull request will possibly be made addressing a proposal for such functions. As an explanation for why this was not implemented in e310x instead, it all boils down on the MachineExternal function depending directly on CorePeripherals, so it was more convenient to implement here. Lastly, I noticed there is a convenience macro in e310x/interrupts.rs , interrupt! which would achieve a similar function to what this pull request implements, but it currently has no implementation and is not working. I would like your feedback on somewhat merging the two efforts to achieve a common goal.

romancardenas commented 1 year ago

Any news on this PR? @almindor we would like to know what you think about this contribution and how we could improve it if needed

almindor commented 1 year ago

Hey, sorry I just didn't have the time to look into this yet. It looks ok on first glance, could you fix the conflicts please? I'll let the tests run (we probably need to also make the CI builds to take this feature into account using the --features flag)

romancardenas commented 1 year ago

Great! We are working on it. We are currently facing a conceptual issue regarding the linking of the new parts. I'll try to explain the problem the best I can so you could give us some advice.

In e310x, you have a few lines regarding interrupts. Basically, you only define the external interrupt sources. We initially thought that it would be a good idea to implement the vectored IRQ table in that crate. However, e310x does not provide functionalities to work with the PLIC etc., which are particularly useful for implementing the MachineExternal function in this vectored version.

That is why we decided to move all the contribution here. The problem is that we now have a new linker file: interrupts.x. We also need to implement a build.rs file for this crate. It may not be an issue, but it increases the pieces of code to maintain.

Next, we must update the hifive1 BSP so it now includes interrupts.x in hifive1-link.x. This step should be straightforward.

Before resolving all the conflicts and the CI stuff, we wanted to know your point of view on how to structure the code among these three crates. Then, we will put all our efforts to integrate our feature following your style.

almindor commented 1 year ago

Could you please split this into two PRs? One for just the lint fixes so we get those out of the way without mixing things up and one for the actual interrupt additions.

romancardenas commented 1 year ago

Sure, my bad! We're working on it

steewbsd commented 1 year ago

Hello, I have separated the changes into a new Pull Request in https://github.com/riscv-rust/e310x-hal/pull/51 . If you deem it appropriate I can close this now.