Skip to content

Commit

Permalink
Fix S2 systimers (esp-rs#1979)
Browse files Browse the repository at this point in the history
* Add basic systimer interrupt tests

* Remove unnecessary condition

* Fix edge interrupt bitmasks

* Modify target_conf in critical section

* Remove unnecessary fn call

* Fix test

* Add failing test case

* Fix S2 systimer interrupts being fired unexpectedly

* Add changelog entry

* Format
  • Loading branch information
bugadani authored Aug 21, 2024
1 parent dc6c53e commit 69cf454
Show file tree
Hide file tree
Showing 7 changed files with 253 additions and 23 deletions.
1 change: 1 addition & 0 deletions esp-hal/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- ESP32C6: Make ADC usable after TRNG deinicialization (#1945)
- We should no longer generate 1GB .elf files for ESP32C2 and ESP32C3 (#1962)
- Reset peripherals in driver constructors where missing (#1893, #1961)
- Fixed ESP32-S2 systimer interrupts (#1979)

### Removed

Expand Down
4 changes: 0 additions & 4 deletions esp-hal/src/interrupt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,6 @@ impl Iterator for InterruptStatusIterator {
type Item = u8;

fn next(&mut self) -> Option<Self::Item> {
if self.idx == usize::MAX {
return None;
}

for i in self.idx..STATUS_WORDS {
if self.status.status[i] != 0 {
let bit = self.status.status[i].trailing_zeros();
Expand Down
13 changes: 6 additions & 7 deletions esp-hal/src/interrupt/xtensa.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Interrupt handling
use xtensa_lx::interrupt::{self, InterruptNumber};
use xtensa_lx::interrupt;
use xtensa_lx_rt::exception::Context;

pub use self::vectored::*;
Expand Down Expand Up @@ -502,7 +502,7 @@ mod vectored {
fn EspDefaultHandler(level: u32, interrupt: Interrupt);
}

let handler = peripherals::__INTERRUPTS[interrupt.number() as usize]._handler;
let handler = peripherals::__INTERRUPTS[interrupt as usize]._handler;
if core::ptr::eq(
handler as *const _,
EspDefaultHandler as *const unsafe extern "C" fn(),
Expand All @@ -520,9 +520,9 @@ mod vectored {
mod chip_specific {
use super::*;
pub const INTERRUPT_EDGE: InterruptStatus = InterruptStatus::from(
0b0000_0000_0000_0000_0000_0000_0000_0011,
0b1111_1100_0000_0000_0000_0000_0000_0000,
0b0000_0000_0000_0000_0000_0000_0000_0000,
0b1111_1100_0000_0000_0000_0000_0000_0000,
0b0000_0000_0000_0000_0000_0000_0000_0011,
);
#[inline]
pub fn interrupt_is_edge(interrupt: Interrupt) -> bool {
Expand All @@ -541,14 +541,13 @@ mod vectored {
}
}

#[allow(clippy::unusual_byte_groupings)]
#[cfg(esp32s2)]
mod chip_specific {
use super::*;
pub const INTERRUPT_EDGE: InterruptStatus = InterruptStatus::from(
0b0000_0000_0000_0000_0000_0011_1011_1111,
0b1100_0000_0000_0000_0000_0000_0000_0000,
0b0000_0000_0000_0000_0000_0000_0000_0000,
0b1100_0000_0000_0000_0000_0000_0000_0000,
0b0000_0000_0000_0000_0000_0011_1011_1111,
);
#[inline]
pub fn interrupt_is_edge(interrupt: Interrupt) -> bool {
Expand Down
51 changes: 45 additions & 6 deletions esp-hal/src/timer/systimer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,20 +396,20 @@ pub trait Comparator {
fn set_enable(&self, enable: bool) {
let systimer = unsafe { &*SYSTIMER::ptr() };

#[cfg(not(esp32s2))]
critical_section::with(|_| {
#[cfg(not(esp32s2))]
systimer.conf().modify(|_, w| match self.channel() {
0 => w.target0_work_en().bit(enable),
1 => w.target1_work_en().bit(enable),
2 => w.target2_work_en().bit(enable),
_ => unreachable!(),
});
});

#[cfg(esp32s2)]
systimer
.target_conf(self.channel() as usize)
.modify(|_r, w| w.work_en().bit(enable));
#[cfg(esp32s2)]
systimer
.target_conf(self.channel() as usize)
.modify(|_r, w| w.work_en().bit(enable));
});
}

/// Returns true if the comparator has been enabled. This means
Expand Down Expand Up @@ -524,9 +524,48 @@ pub trait Comparator {
2 => Interrupt::SYSTIMER_TARGET2,
_ => unreachable!(),
};

#[cfg(not(esp32s2))]
unsafe {
interrupt::bind_interrupt(interrupt, handler.handler());
}

#[cfg(esp32s2)]
{
// ESP32-S2 Systimer interrupts are edge triggered. Our interrupt
// handler calls each of the handlers, regardless of which one triggered the
// interrupt. This mess registers an intermediate handler that
// checks if an interrupt is active before calling the associated
// handler functions.

static mut HANDLERS: [Option<extern "C" fn()>; 3] = [None, None, None];

#[crate::prelude::ram]
unsafe extern "C" fn _handle_interrupt<const CH: u8>() {
if unsafe { &*SYSTIMER::PTR }
.int_raw()
.read()
.target(CH)
.bit_is_set()
{
let handler = unsafe { HANDLERS[CH as usize] };
if let Some(handler) = handler {
handler();
}
}
}

unsafe {
HANDLERS[self.channel() as usize] = Some(handler.handler());
let handler = match self.channel() {
0 => _handle_interrupt::<0>,
1 => _handle_interrupt::<1>,
2 => _handle_interrupt::<2>,
_ => unreachable!(),
};
interrupt::bind_interrupt(interrupt, handler);
}
}
unwrap!(interrupt::enable(interrupt, handler.priority()));
}
}
Expand Down
7 changes: 1 addition & 6 deletions examples/src/bin/systimer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ use esp_backtrace as _;
use esp_hal::{
clock::ClockControl,
delay::Delay,
interrupt::{self, Priority},
peripherals::{Interrupt, Peripherals},
peripherals::Peripherals,
prelude::*,
system::SystemControl,
timer::systimer::{
Expand Down Expand Up @@ -87,10 +86,6 @@ fn main() -> ! {
ALARM2.borrow_ref_mut(cs).replace(alarm2);
});

interrupt::enable(Interrupt::SYSTIMER_TARGET0, Priority::Priority1).unwrap();
interrupt::enable(Interrupt::SYSTIMER_TARGET1, Priority::Priority3).unwrap();
interrupt::enable(Interrupt::SYSTIMER_TARGET2, Priority::Priority3).unwrap();

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

loop {
Expand Down
5 changes: 5 additions & 0 deletions hil-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ harness = false
name = "spi_half_duplex_write"
harness = false

[[test]]
name = "systimer"
harness = false

[[test]]
name = "pcnt"
harness = false
Expand Down Expand Up @@ -163,6 +167,7 @@ elliptic-curve = { version = "0.13.8", default-features = false, features =
embassy-executor = { version = "0.6.0", default-features = false }
# Add the `embedded-test/defmt` feature for more verbose testing
embedded-test = { version = "0.4.0", default-features = false }
fugit = "0.3.7"
hex-literal = "0.4.1"
nb = "1.1.0"
p192 = { version = "0.13.0", default-features = false, features = ["arithmetic"] }
Expand Down
195 changes: 195 additions & 0 deletions hil-test/tests/systimer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
//! System Timer Test
// esp32 disabled as it does not have a systimer
//% CHIPS: esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3

#![no_std]
#![no_main]

use core::cell::RefCell;

use critical_section::Mutex;
use embedded_hal::delay::DelayNs;
use esp_hal::{
clock::{ClockControl, Clocks},
delay::Delay,
peripherals::Peripherals,
prelude::*,
system::SystemControl,
timer::systimer::{
Alarm,
FrozenUnit,
Periodic,
SpecificComparator,
SpecificUnit,
SystemTimer,
Target,
},
Blocking,
};
use fugit::ExtU32;
use hil_test as _;
use portable_atomic::{AtomicUsize, Ordering};
use static_cell::StaticCell;

type TestAlarm<M, const C: u8> =
Alarm<'static, M, Blocking, SpecificComparator<'static, C>, SpecificUnit<'static, 0>>;

static ALARM_TARGET: Mutex<RefCell<Option<TestAlarm<Target, 0>>>> = Mutex::new(RefCell::new(None));
static ALARM_PERIODIC: Mutex<RefCell<Option<TestAlarm<Periodic, 1>>>> =
Mutex::new(RefCell::new(None));

struct Context {
unit: FrozenUnit<'static, SpecificUnit<'static, 0>>,
comparator0: SpecificComparator<'static, 0>,
comparator1: SpecificComparator<'static, 1>,
clocks: Clocks<'static>,
}

impl Context {
pub fn init() -> Self {
let peripherals = Peripherals::take();
let system = SystemControl::new(peripherals.SYSTEM);
let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

let systimer = SystemTimer::new(peripherals.SYSTIMER);
static UNIT0: StaticCell<SpecificUnit<'static, 0>> = StaticCell::new();

let unit0 = UNIT0.init(systimer.unit0);
let frozen_unit = FrozenUnit::new(unit0);

Context {
clocks,
unit: frozen_unit,
comparator0: systimer.comparator0,
comparator1: systimer.comparator1,
}
}
}

#[handler(priority = esp_hal::interrupt::Priority::min())]
fn pass_test_if_called() {
critical_section::with(|cs| {
ALARM_TARGET
.borrow_ref_mut(cs)
.as_mut()
.unwrap()
.clear_interrupt()
});
embedded_test::export::check_outcome(());
}

#[handler(priority = esp_hal::interrupt::Priority::min())]
fn handle_periodic_interrupt() {
critical_section::with(|cs| {
ALARM_PERIODIC
.borrow_ref_mut(cs)
.as_mut()
.unwrap()
.clear_interrupt()
});
}

static COUNTER: AtomicUsize = AtomicUsize::new(0);

#[handler(priority = esp_hal::interrupt::Priority::min())]
fn pass_test_if_called_twice() {
critical_section::with(|cs| {
ALARM_PERIODIC
.borrow_ref_mut(cs)
.as_mut()
.unwrap()
.clear_interrupt()
});
COUNTER.fetch_add(1, Ordering::Relaxed);
if COUNTER.load(Ordering::Relaxed) == 2 {
embedded_test::export::check_outcome(());
}
}

#[handler(priority = esp_hal::interrupt::Priority::min())]
fn target_fail_test_if_called_twice() {
critical_section::with(|cs| {
ALARM_TARGET
.borrow_ref_mut(cs)
.as_mut()
.unwrap()
.clear_interrupt()
});
COUNTER.fetch_add(1, Ordering::Relaxed);
assert!(COUNTER.load(Ordering::Relaxed) != 2);
}

#[cfg(test)]
#[embedded_test::tests]
mod tests {
use super::*;

#[init]
fn init() -> Context {
Context::init()
}

#[test]
#[timeout(3)]
fn target_interrupt_is_handled(ctx: Context) {
let alarm0 = Alarm::new(ctx.comparator0, &ctx.unit);

critical_section::with(|cs| {
alarm0.set_interrupt_handler(pass_test_if_called);
alarm0.set_target(SystemTimer::now() + SystemTimer::TICKS_PER_SECOND / 10);
alarm0.enable_interrupt(true);

ALARM_TARGET.borrow_ref_mut(cs).replace(alarm0);
});

// We'll end the test in the interrupt handler.
loop {}
}

#[test]
#[timeout(3)]
fn target_interrupt_is_handled_once(ctx: Context) {
let alarm0 = Alarm::new(ctx.comparator0, &ctx.unit);
let alarm1 = Alarm::new(ctx.comparator1, &ctx.unit);

COUNTER.store(0, Ordering::Relaxed);

critical_section::with(|cs| {
alarm0.set_interrupt_handler(target_fail_test_if_called_twice);
alarm0.set_target(SystemTimer::now() + SystemTimer::TICKS_PER_SECOND / 10);
alarm0.enable_interrupt(true);

let alarm1 = alarm1.into_periodic();
alarm1.set_interrupt_handler(handle_periodic_interrupt);
alarm1.set_period(100u32.millis());
alarm1.enable_interrupt(true);

ALARM_TARGET.borrow_ref_mut(cs).replace(alarm0);
ALARM_PERIODIC.borrow_ref_mut(cs).replace(alarm1);
});

let mut delay = Delay::new(&ctx.clocks);
delay.delay_ms(300);
}

#[test]
#[timeout(3)]
fn periodic_interrupt_is_handled(ctx: Context) {
let alarm1 = Alarm::new(ctx.comparator1, &ctx.unit);

COUNTER.store(0, Ordering::Relaxed);

critical_section::with(|cs| {
let alarm1 = alarm1.into_periodic();
alarm1.set_interrupt_handler(pass_test_if_called_twice);
alarm1.set_period(100u32.millis());
alarm1.enable_interrupt(true);

ALARM_PERIODIC.borrow_ref_mut(cs).replace(alarm1);
});

// We'll end the test in the interrupt handler.
loop {}
}
}

0 comments on commit 69cf454

Please sign in to comment.