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

Decouple embassy-hal-common InterruptExt from cortex-m #1689

Conversation

aarongowatch
Copy link

@aarongowatch aarongowatch commented Jul 25, 2023

Decouples embassy_hal_common::interrupt from Cortex-M and Cortex-M dependencies, allowing implementations of embassy_hal_common::interrupt::InterruptExt for non-NVIC interrupt controllers (PLIC, CLIC, GIC, ...). Adds a sample implementation for the RISC-V PLIC from the riscv crate.

There are a couple of things to highlight:

  • The example depends unreleased features in the riscv crate, and it looks like the PLIC implementation is in flux.
  • Not all interrupt controllers have feature parity. For example, a PLIC cannot pend/unpend interrupts. InterruptExt is unopinionated about how to handle such feature gaps in interrupt controller implementations. The RISC-V PLIC also supports a priority threshold.
  • Interrupt priorities are inverted from the NVIC as noted in the implementation.
  • Implementation currently only supports embassy-hal-common/prio-bits-2.

Nonetheless this begins to decouple the InterruptExt abstraction from the NVIC, allow of non-NVIC implementations, and maintains backwards compatibility.

@aarongowatch aarongowatch force-pushed the feature/cortex-m-interrupt-decoupling branch 2 times, most recently from a11b5c9 to cc885d4 Compare July 25, 2023 22:34
@aarongowatch aarongowatch force-pushed the feature/cortex-m-interrupt-decoupling branch from cc885d4 to 8d286e7 Compare July 25, 2023 22:51
@Dirbaio
Copy link
Member

Dirbaio commented Jul 25, 2023

Hi! Thanks for the PR.

Unfortunately I don't think we should port embassy_hal_common::interrupt to other archs, instead each HAL should bring its own implementation for the chip's arch.

  • The embassy-hal-common crate is not intended to be used directly, it's just a way to share code between the 3 HALs maintained by the Embassy project (embassy-nrf, embassy-stm32, embassy-rp). These are all Cortex-M chips.
  • A single implementation/API supporting multiple archs doesn't enable code reuse, since code touching interrupts is already hardware-dependent and will need porting anyway if you switch chips.
  • Single API presents challenges because different archs have different capabilities, different priority numbering...
  • Lots of cfgs makes the code more complicated.

If you want to write a HAL for a RISCV chip, you can make it bring its own independent interrupt implementation. It'll work just fine with Embassy, because there's nothing requiring the HAL to use embassy_hal_common::interrupt specifically. The executor, -sync, -futures don't depend on it, all they care is the HAL somehow wakes the wakers when needed.

@aarongowatch
Copy link
Author

aarongowatch commented Jul 25, 2023

Thanks for the reply!

My use case, building the same Embassy application to run on multiple targets, made this abstraction highly useful (aside from the feature gaps we've highlighted between interrupt controller hardware implementations). Use cases exist where "board" implementations share common peripherals and are divergent only in processor and interrupt controller hardware implementations. RISC-V supports several different interrupt controllers, including the PLIC and CLIC. I was under the impression from #745 (comment) that the abstraction might be generally useful, but if that's not the case I'll happily close the PR!

@xoviat
Copy link
Contributor

xoviat commented Jul 26, 2023

fwiw, GIC is needed for stm32mp series chips. If I ever get around to it, it would be nice to have portability between mcu and mpu. That's still inside the arm universe, though.

@aarongowatch
Copy link
Author

There exists a community that is interested in developing device drivers and applications that are portable among processors and physical devices that would make these abstractions useful, but perhaps that use case is not generally useful. Thank you for the feedback!

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

Successfully merging this pull request may close these issues.

4 participants