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

RISCV: remove the direct-vectoring & interrupt-preemption features and enable them by default #1310

Merged
merged 10 commits into from
Mar 20, 2024

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Mar 19, 2024

This is an attempt to try and solve #1246

  • Enables the feature by default, removing the cfgs
  • Removes old redundant code
  • renames the old direct_vectoring enable function enable_direct

TODO

  • Can interrupt-preemption also be enabled by default/removed?
  • enable_direct a good name? Maybe we just remove this method completely, it's possible to "build" this function using the other methods available
    • I think we want to keep it for now, which we reserve some interrupts for the vectored feature, we still have some CPU interrupts free which users can use directly if they wish.
  • Test chips other than esp32c3 & esp32c6
  • Test exceptions also work on all chips, they work on esp32c3 at least
  • Test it doesn't regress interrupt latency (I would hope that this is a net benefit)
    • main, first trigger: 25us, after that 13us
    • This RP: consistent 10-11us latency
  • Test the fix-sp feature still works
  • Documentation updates

@MabezDev
Copy link
Member Author

@bjoernQ I know we're not really looking P4 support right now, but do you know if this is going to be viable for the P4?

@MabezDev MabezDev force-pushed the direct-vectoring-by-default branch from 71c8033 to 016cba3 Compare March 19, 2024 00:26
@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 19, 2024

@bjoernQ I know we're not really looking P4 support right now, but do you know if this is going to be viable for the P4?

Vectored interrupts are definitely supported. t.b.h. I don't recall why I omitted dirtect-vectoring support for P4 initially but shouldn't be a huge problem to implement it

esp-hal/src/interrupt/riscv.rs Outdated Show resolved Hide resolved
esp-riscv-rt/src/lib.rs Show resolved Hide resolved
@MabezDev
Copy link
Member Author

I like removing features, so I've made an executive to cut out interrupt-preemption for riscv and enable it by default.

This matches the same behaviour as Xtensa by default, so I think it's a good change all around. You can test the behaviour of both arches with the interrupt preemption example.

@MabezDev MabezDev force-pushed the direct-vectoring-by-default branch from 1f7bba7 to 52d84c7 Compare March 19, 2024 14:50
@MabezDev MabezDev marked this pull request as ready for review March 19, 2024 14:55
@MabezDev
Copy link
Member Author

I ended up doing a couple of changes to Xtensa too, just to being the APIs in line with each other (they're still different and can't be shared, but they have the same capabilities).

Overall I'm quite happy with how this ended up. I just need to test the latency effects and @bjoernQ if you could show me or test yourself the fix-sp feature.

This is now read for review.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 19, 2024

I tested this with esp-wifi and after the necessary adjustments it seems to still work on C3, C6 and S3.

To test the fix-sp functionality I just use some code to overflow the stack and the flip-link feature. I can test that

@MabezDev MabezDev changed the title Remove the direct-vectoring feature and enable it by default Remove the direct-vectoring & interrupt-preemption features and enable them by default Mar 19, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 19, 2024

Ok seems fix-sp is broken here.

There is also a problem enabling "flip-link" in build.rs - but that was already broken before this PR. We need a ".to_string()" there.

To reproduce I changed hello_world.rs into

//! This shows how to write text to UART0.
//!
//! You can see the output with `espflash` if you provide the `--monitor`
//! option.

//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
//% FEATURES: embedded-hal-02

#![no_std]
#![no_main]

use core::fmt::Write;

use embedded_hal_02::timer::CountDown;
use esp_backtrace as _;
use esp_hal::{
    clock::ClockControl,
    peripherals::Peripherals,
    prelude::*,
    timer::TimerGroup,
    uart::Uart,
};
use nb::block;
use esp_println::println;

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

    let timg0 = TimerGroup::new(peripherals.TIMG0, &clocks);
    let mut timer0 = timg0.timer0;
    timer0.start(1u64.secs());

    let mut uart0 = Uart::new(peripherals.UART0, &clocks);

    boom();

    loop {
        writeln!(uart0, "Hello world!").unwrap();
        block!(timer0.wait()).unwrap();
    }
}

#[inline(never)]
fn boom() {
    deadly_recursion([0u8; 2048]);
}

#[ram]
#[allow(unconditional_recursion)]
fn deadly_recursion(data: [u8; 2048]) {
    static mut COUNTER: u32 = 0;

    println!(
        "Iteration {}, data {:02x?}...",
        unsafe { COUNTER },
        &data[0..10]
    );

    unsafe {
        COUNTER = COUNTER.wrapping_add(1);
    };

    deadly_recursion([0u8; 2048]);
}

And enabled flip-link for esp-hal. Works on main but doesn't here. (i.e. it prints "Stack overflow detected at 0x42004124 called by 0x42003e0a" on main but nothing here)

@MabezDev
Copy link
Member Author

Thanks to @bjoernQ's test configuration, I measured the interrupt latency on main and this PR:

  • main, first trigger: 25us, after that 13us
  • This PR: consistent 10-11us latency

I'm pleased about that!

@MabezDev
Copy link
Member Author

Thanks for testing fix-sp, I'll look into fixing that tomorrow.

@MabezDev MabezDev changed the title Remove the direct-vectoring & interrupt-preemption features and enable them by default RISCV: remove the direct-vectoring & interrupt-preemption features and enable them by default Mar 19, 2024
@MabezDev
Copy link
Member Author

Fix-sp/flip-link feature should be working now! I had the code in the wrong place, it should be before we modify the sp :D.

@MabezDev MabezDev mentioned this pull request Mar 19, 2024
14 tasks
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - nice improvement!

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 for doing this!

@MabezDev MabezDev force-pushed the direct-vectoring-by-default branch from c1315c7 to 7917640 Compare March 20, 2024 14:46
@jessebraham
Copy link
Member

Looks like there are some missing imports still

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you once again for tackling this! 😁

@jessebraham jessebraham enabled auto-merge March 20, 2024 16:05
@jessebraham jessebraham added this pull request to the merge queue Mar 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2024
@jessebraham
Copy link
Member

GHA why are you like this ;_;

@jessebraham jessebraham added this pull request to the merge queue Mar 20, 2024
@MabezDev
Copy link
Member Author

image

Merged via the queue into esp-rs:main with commit a61ffef Mar 20, 2024
17 checks passed
@MabezDev MabezDev deleted the direct-vectoring-by-default branch March 20, 2024 16:28
yanshay pushed a commit to yanshay/esp-hal that referenced this pull request Mar 20, 2024
…s and enable them by default (esp-rs#1310)

* Remove the `direct-vectoring` feature

* Enables the feature by default
* renames the old direct_vectoring enable function `enable_direct`

* Make enable_direct safe, move it out of vectored module

* enable interrupt preemption by default for riscv

* remove pub from cpu intr handlers

* add enable_direct for Xtensa too

* Fix flip-link feature

* Fix up interrupt docs

* changelog

* fix clippy suggestions

* Disable P4 workflow
yanshay pushed a commit to yanshay/esp-hal that referenced this pull request Mar 25, 2024
…s and enable them by default (esp-rs#1310)

* Remove the `direct-vectoring` feature

* Enables the feature by default
* renames the old direct_vectoring enable function `enable_direct`

* Make enable_direct safe, move it out of vectored module

* enable interrupt preemption by default for riscv

* remove pub from cpu intr handlers

* add enable_direct for Xtensa too

* Fix flip-link feature

* Fix up interrupt docs

* changelog

* fix clippy suggestions

* Disable P4 workflow
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.

5 participants