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

Improve RISC-V interrupt latency #1679

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Jun 14, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

This improves interrupt latency on RISC-V based chips.

Testing

All interrupt, embassy and esp-wifi examples still work.

To test the latency, I modified the gpio_interrupt.rs example like this:

//! GPIO interrupt
//!
//! This prints "Interrupt" when the boot button is pressed.
//! It also blinks an LED like the blinky example.

//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3

#![no_std]
#![no_main]

use core::cell::RefCell;

use critical_section::Mutex;
use esp_backtrace as _;
use esp_hal::{
    clock::ClockControl,
    delay::Delay,
    gpio::{self, Event, Input, Io, Level, Output, Pull},
    macros::ram,
    peripherals::Peripherals,
    prelude::*,
    system::SystemControl,
};

#[cfg(any(feature = "esp32", feature = "esp32s2", feature = "esp32s3"))]
static BUTTON: Mutex<RefCell<Option<Input<gpio::Gpio0>>>> = Mutex::new(RefCell::new(None));
#[cfg(not(any(feature = "esp32", feature = "esp32s2", feature = "esp32s3")))]
static BUTTON: Mutex<RefCell<Option<Input<gpio::Gpio9>>>> = Mutex::new(RefCell::new(None));

static LED: Mutex<RefCell<Option<Output<gpio::Gpio2>>>> = Mutex::new(RefCell::new(None));

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let system = SystemControl::new(peripherals.SYSTEM);
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    // Set GPIO2 as an output, and set its state high initially.
    let mut io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
    io.set_interrupt_handler(user_gpio_handler);
    let mut led = Output::new(io.pins.gpio2, Level::Low);

    #[cfg(any(feature = "esp32", feature = "esp32s2", feature = "esp32s3"))]
    let button = io.pins.gpio0;
    #[cfg(not(any(feature = "esp32", feature = "esp32s2", feature = "esp32s3")))]
    let button = io.pins.gpio9;

    let mut button = Input::new(button, Pull::Up);

    critical_section::with(|cs| {
        LED.borrow_ref_mut(cs).replace(led);

        button.listen(Event::FallingEdge);
        BUTTON.borrow_ref_mut(cs).replace(button);
    });

    let delay = Delay::new(&clocks);

    loop {}
}

#[handler]
#[ram]
fn user_gpio_handler() {
    critical_section::with(|cs| {
        LED.borrow_ref_mut(cs).as_mut().unwrap().toggle();

        BUTTON
            .borrow_ref_mut(cs)
            .as_mut()
            .unwrap()
            .clear_interrupt()
    });
}

Then use a logic-analyzer triggering on GPIO 9 and measure the time from the falling edge of GPIO 9 to the changing edge on GPIO 2. This obviously includes more than just the time to trigger the interrupt but it's also a somewhat realistic test.

Here is what I measured:
(all measured at boot defaults / 40MHz flash frequency, used rustc 1.80.0-nightly (79734f1db 2024-05-02))

C2
before 31.692 / 12.40
after 18.517 / 5.64

C3
before 21.432 / 10.572
after 11.79 / 4.99

C6
before 17.125 / 10.71
after 8.95 / 5.94

H2
before 25.335 / 11.36
after 14.27 / 5.44

I'm quite happy with the improvement!

The first triggering is still much slower which is because there are cache misses (see #1162 - originally, I wanted to address the things there, will add a comment why I didn't there - see #1162 (comment))

We should probably try something similar for Xtensa

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Jun 14, 2024

CI fails because of current esp-backtrace problems on recent nightlies, once it's fixed this needs a rebase

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

esp-hal/src/lib.rs Show resolved Hide resolved
// jr handle_interrupts
// ```
//
// We can do better manually - use Rust again once/if that changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, this should have been in asm from the start, I was just lazy & hoped the compiler would show just the smallest amount of intelligence 🥲

#[inline]
fn get_configured_interrupts(core: Cpu, mut status: u128) -> [u128; 16] {
fn get_configured_interrupts(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a much better way of doing this, nice! We can also apply to Xtensa too, I think there is a very similar block of code. Let's do that in a separate PR though (I'll make a note in the tracking issue).

/// Get status of peripheral interrupts
#[inline]
pub fn get_status(_core: Cpu) -> u128 {
pub fn get_status(_core: Cpu) -> InterruptStatus {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should avoid u128's where possible in general tbh, it seems the codegen for them is not really that good - nice find.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a big improvement!

@MabezDev
Copy link
Member

@bjoernQ mind rebasing this when you get the chance?

@bjoernQ bjoernQ force-pushed the improve-riscv-interrupt-latency branch from 2a1b023 to ad27c5f Compare June 19, 2024 16:26
@jessebraham jessebraham added this pull request to the merge queue Jun 19, 2024
Merged via the queue into esp-rs:main with commit 13d6b51 Jun 19, 2024
29 checks passed
@bjoernQ bjoernQ mentioned this pull request Jun 28, 2024
5 tasks
@bjoernQ bjoernQ deleted the improve-riscv-interrupt-latency branch November 26, 2024 08:41
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