-
Notifications
You must be signed in to change notification settings - Fork 198
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
Xtensa vectored interrupts #103
Conversation
17cc393
to
623d4d6
Compare
3d55e7c
to
1c19c96
Compare
This is now ready for review. @bjoernQ do you think you could try and use the vectored interrupts in esp-wifi and report back? |
After looking into it I don't know how to do this While the WiFi driver calls Following interrupts are used by WiFi and BLE
|
The GPIO interrupt example looks good but unfortunately timer_interrupt and serial_interrupts examples don't work anymore. For timer_interrupt it uses a reserved interrupt but even after changing it, it still doesn't work anymore Probably we could change them to use vectored interrupts but we should keep one of the examples using the low-level way so we can always verify it is still working Only tested on ESP32 so far |
Maybe the low-level function could be renamed and made private and we add a new public function with the old name and signature which does the check? |
We could try to change the interrupt handling to allow higher priority interrupt levels while executing the Rust ISR or the ISR could opt-in to get interrupted by lowering |
Thanks for the review!
Hmm, this would be fine but we need an entry for the interrupt in the
I don't think it will be possible to mix vectoring interrupts & non-vectoring interrupts in their current state, as vectoring requires registering all the level handlers to go to one handler. One thing I can see is not registering levels 4 through 7, and only using vectoring for 'low-level' interrupts (1 through 3). Given that we have this limitation in xtensa-lx-rt already it sort of makes sense. Then high prio interrupts could be written in the low level way. To write an example using the low-level handlers would require
This is a good idea, I'll try this out, thanks!
This sounds good! Having at least a couple of levels of preemption will be helpful. Only three priorities may be an issue if we ever want an RTIC port or something, but by that time we could look into software interrupt priorities. |
Actually, I guess I can just read the mapping registers, see what CPU interrupt it's assigned to a have a lookup table of priorities there... I'll give that a try |
Ah sure - I probably need more coffee 😄 One thing we shouldn't forget: get rid of the patch sections in the TOMLs ... just mentioning that because those things often happen to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to dig into the interrupt/xtensa.rs
file a bit more and do some local testing, but mostly looks fine to me. Left a few comments, not all necessarily require action.
9116950
to
4591176
Compare
Rebased on the main, a couple of things are left:
|
PR in |
a1f495f
to
40401a4
Compare
There is one thing missing unfortunately (before I can actually try it): The task-scheduler needs a mutable reference to the trap frame - we don't have that for vectored interrupts yet Besides that, it looks quite nice and convenient - can't wait to get that for RISCV Two last things:
|
Hmm, we would need to change the handler generation in the pacs, right? Or maybe we can be cheeky and accept both handlers with args and without? We can (ab)use the linker to allow interrupt handlers to either have no arguments or the context. I don't think that will cause issues, provided we always call the handler with the arguments. |
I also think that should work fine |
With the latest commit it's now possible to pass the context into interrupt handlers: #[interrupt]
fn GPIO(frame: &mut xtensa_lx_rt::exception::Context) {
unsafe {
esp_println::println!(
"GPIO Interrupt with priority {}",
xtensa_lx::interrupt::get_level()
);
esp_println::println!(
"{:?}",
frame
);
(&BUTTON).lock(|data| {
let mut button = data.borrow_mut();
let button = button.as_mut().unwrap();
button.clear_interrupt();
});
}
} The frame can be omitted if its not needed. |
34c8628
to
f674581
Compare
Still trying to get esp-hal/esp-hal-procmacros/src/lib.rs Line 184 in 0f1aaaf
Creates problems since |
🎉 I got at least the WiFi example working with the vectored interrupts ... need to look into BT-BLE |
Oh, not that easy for BLE ... it needs Interrupt7SoftwarePriority1 but I can only register peripheral interrupts? Ah yes I see: |
The interrupt levels static introduces a few issues - A lock is needed when configuring interrupts to keep INTERRUPT_LEVELS in a consistent state - Interrupts enabled from outside the Rust domain wouldn't be serviced, this is the case with the wifi blobs To remove it, the prioty configuration is now calculated dynamically in the interrupt handler. Essentially INTERRUPT_LEVELS is now created once the interrupt triggers. It has some benefits, such as only having to look at interrupts configured on the current core, not both, but there is of course an overhead with doing this in the interrupt.
I'm fine with pushing off examples to a later PR, if you'd like to get this wrapped up. |
BTLE is not yet fully working ... but I think it should and the reason it's not working is most probably not because of the vectored interrupts |
I'm not so sure about that ... I mean the examples will just not work at all if they don't get adjusted - ideally we should never have such a situation on the main branch |
move vectored feature to chip specific hals
0f1aaaf
to
1eb70a0
Compare
- rename `enable_with_priority` to `enable` - add docs for interrupt macro
I'm going to fix the old examples to use vectoring, I think Jesse is referring to my last comment about a raw example using levels 4-7. |
Yes sorry I should have been more clear, I expect the existing examples to continue to work but adding new ones can be deferred for now. |
be7bd14
to
c0c6df1
Compare
c0c6df1
to
67ba5f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this!
* Xtensa interrupt vectoring: peripheral source - Initial Xtensa vectoring, updated esp32 gpio example to use new interrupt macro. - Only peripheral sources supported. - Only level one priority supported. - CPU & Edge interrupts still need to be handled. * Xtensa interrupt vectoring: CPU & EDGE - Add support for handling CPU interrupts and edge interrupts - PR required to xtensa-lx-rt for CPU handlers * Xtensa interrupt vectoring: Priority - Finally implement priortization - Only three priorities available at the moment. Xtensa programmer guide discourages using highpri interrupts in Rust/C. Guide also mentions using software priortization to increase the number of Priorities available * support CPU interrupts, using patch xtensa-lx-rt * Update example * Add support & examples for the s2 & s3 too * Fix formatting and missing imports * Run interrupt handling in ram, optionally run the vector handler in ram in the examples * Use xtensa_lx::Mutex CS when enabling interrupts * Run clippy on each target * Remove redundant features * Fix C3 builds * make enable unsafe. Add note about preallocated interrupts in vectored mode. * Remove `INTERRUPT_LEVELS` static The interrupt levels static introduces a few issues - A lock is needed when configuring interrupts to keep INTERRUPT_LEVELS in a consistent state - Interrupts enabled from outside the Rust domain wouldn't be serviced, this is the case with the wifi blobs To remove it, the prioty configuration is now calculated dynamically in the interrupt handler. Essentially INTERRUPT_LEVELS is now created once the interrupt triggers. It has some benefits, such as only having to look at interrupts configured on the current core, not both, but there is of course an overhead with doing this in the interrupt. * Allow raw interrupts on levels 4-7, whilst also supporting vectoring on levels 1-3 * rename core number features * Fix examples and formatting * use xtensa-lx-rt release, update pacs * Support passing the trap frame into interrupt handlers * cfg away the #[interrupt] macro when not using vectoring * rename enable to map move vectored feature to chip specific hals * export vectored functions - rename `enable_with_priority` to `enable` - add docs for interrupt macro * Update all examples to use vectored interrupts
It seems that this PR broke the esp32c3-hal. The Hal won't build unless I revert back to the commit before this one. Is anyone else having the same issue? |
Never mind. The lock file was causing problems |
This only adds support for Xtensa, RISCV will be in a follow-up PR. Quite a biggy, so best reviewed commit by each commit.
Summary
Interrupt10EdgePriority1
,Interrupt22EdgePriority3
,Interrupt1LevelPriority1
,Interrupt19LevelPriority2
&Interrupt23LevelPriority3
.#[ram]
attributeIssues
enable(core: Cpu, interrupt: Interrupt, which: CpuInterrupt)
&disable(core: Cpu, interrupt: Interrupt)
be madeunsafe
? They may break vectoring unless you know what you're doing.Maybe the CPU interrupts should becfg
'd away when vectoring is enabled?xtensa_lx::mutex
's for nowesp-wifi
, instead of raw level handlers? Note that the relevant WiFi and Bluetooth interrupts were added in the PACS in Add WiFi and bluetooth interrupts for each chip esp-pacs#26.Add examples of using non-vectoring? It will require some CI work, seesmartled
feature shouldn't be activated by default #105