Use enums in timers to avoid some unreachable panics (#4299)

* Use an enum to avoid timer bounds checks

* Outline debug assert

Author: bugadani

esp-hal/src/timer/systimer.rs

@@ -104,9 +104,9 @@ impl<'d> SystemTimer<'d> {
         etm::enable_etm();
 
         Self {
-            alarm0: Alarm::new(0),
-            alarm1: Alarm::new(1),
-            alarm2: Alarm::new(2),
+            alarm0: Alarm::new(Comparator::Comparator0),
+            alarm1: Alarm::new(Comparator::Comparator1),
+            alarm2: Alarm::new(Comparator::Comparator2),
         }
     }
 
@@ -263,17 +263,25 @@ impl Unit {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+enum Comparator {
+    Comparator0,
+    Comparator1,
+    Comparator2,
+}
+
 /// An alarm unit
 #[derive(Debug)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub struct Alarm<'d> {
-    comp: u8,
+    comp: Comparator,
     unit: Unit,
     _lifetime: PhantomData<&'d mut ()>,
 }
 
 impl Alarm<'_> {
-    const fn new(comp: u8) -> Self {
+    const fn new(comp: Comparator) -> Self {
         Alarm {
             comp,
             unit: Unit::Unit0,
@@ -310,7 +318,7 @@ impl Alarm<'_> {
     /// Returns the comparator's number.
     #[inline]
     fn channel(&self) -> u8 {
-        self.comp
+        self.comp as u8
     }
 
     /// Enables/disables the comparator. If enabled, this means
@@ -639,30 +647,30 @@ mod asynch {
     }
 
     #[inline]
-    fn handle_alarm(alarm: u8) {
+    fn handle_alarm(comp: Comparator) {
         Alarm {
-            comp: alarm,
+            comp,
             unit: Unit::Unit0,
             _lifetime: PhantomData,
         }
         .enable_interrupt(false);
 
-        WAKERS[alarm as usize].wake();
+        WAKERS[comp as usize].wake();
     }
 
     #[handler]
     pub(crate) fn target0_handler() {
-        handle_alarm(0);
+        handle_alarm(Comparator::Comparator0);
     }
 
     #[handler]
     pub(crate) fn target1_handler() {
-        handle_alarm(1);
+        handle_alarm(Comparator::Comparator1);
     }
 
     #[handler]
     pub(crate) fn target2_handler() {
-        handle_alarm(2);
+        handle_alarm(Comparator::Comparator2);
     }
 }
 

esp-hal/src/timer/timg.rs

@@ -274,14 +274,14 @@ where
         Self {
             _timer_group: PhantomData,
             timer0: Timer {
-                timer: 0,
+                timer: TimerId::Timer0,
                 tg: T::id(),
                 register_block: T::register_block(),
                 _lifetime: PhantomData,
             },
             #[cfg(timergroup_timg_has_timer1)]
             timer1: Timer {
-                timer: 1,
+                timer: TimerId::Timer1,
                 tg: T::id(),
                 register_block: T::register_block(),
                 _lifetime: PhantomData,
@@ -380,10 +380,18 @@ impl super::Timer for Timer<'_> {
 pub struct Timer<'d> {
     register_block: *const RegisterBlock,
     _lifetime: PhantomData<&'d mut ()>,
-    timer: u8,
+    timer: TimerId,
     tg: u8,
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+enum TimerId {
+    Timer0,
+    #[cfg(timergroup_timg_has_timer1)]
+    Timer1,
+}
+
 impl Sealed for Timer<'_> {}
 unsafe impl Send for Timer<'_> {}
 
@@ -444,7 +452,7 @@ impl Timer<'_> {
     }
 
     fn timer_number(&self) -> u8 {
-        self.timer
+        self.timer as u8
     }
 
     fn t(&self) -> &crate::pac::timg0::T {
@@ -517,7 +525,7 @@ impl Timer<'_> {
     fn clear_interrupt(&self) {
         self.register_block()
             .int_clr()
-            .write(|w| w.t(self.timer).clear_bit_by_one());
+            .write(|w| w.t(self.timer as _).clear_bit_by_one());
         let periodic = self.t().config().read().autoreload().bit_is_set();
         self.set_alarm_active(periodic);
     }
@@ -567,7 +575,7 @@ impl Timer<'_> {
         self.register_block()
             .int_raw()
             .read()
-            .t(self.timer)
+            .t(self.timer as _)
             .bit_is_set()
     }
 
@@ -577,8 +585,7 @@ impl Timer<'_> {
                 // On ESP32 and S2, the `int_ena` register is ineffective - interrupts fire even
                 // without int_ena enabling them. We use level interrupts so that we have a status
                 // bit available.
-                self.register_block()
-                    .t(self.timer as usize)
+                self.t()
                     .config()
                     .modify(|_, w| w.level_int_en().bit(state));
             } else if #[cfg(timergroup_timg_has_timer1)] {
@@ -868,7 +875,7 @@ mod asynch {
         handle_irq(Timer {
             register_block: TIMG0::regs(),
             _lifetime: PhantomData,
-            timer: 0,
+            timer: TimerId::Timer0,
             tg: 0,
         });
     }
@@ -879,7 +886,7 @@ mod asynch {
         handle_irq(Timer {
             register_block: TIMG1::regs(),
             _lifetime: PhantomData,
-            timer: 0,
+            timer: TimerId::Timer0,
             tg: 1,
         });
     }
@@ -890,7 +897,7 @@ mod asynch {
         handle_irq(Timer {
             register_block: TIMG0::regs(),
             _lifetime: PhantomData,
-            timer: 1,
+            timer: TimerId::Timer1,
             tg: 0,
         });
     }
@@ -901,7 +908,7 @@ mod asynch {
         handle_irq(Timer {
             register_block: TIMG1::regs(),
             _lifetime: PhantomData,
-            timer: 1,
+            timer: TimerId::Timer1,
             tg: 1,
         });
     }

esp-sync/src/raw.rs

@@ -66,9 +66,22 @@ impl RawLock for SingleCoreInterruptLock {
                     }
                 }
             } else if #[cfg(xtensa)] {
-                // Reserved bits in the PS register, these must be written as 0.
-                const RESERVED_MASK: u32 = 0b1111_1111_1111_1000_1111_0000_0000_0000;
-                debug_assert!(token & RESERVED_MASK == 0);
+                #[cfg(debug_assertions)]
+                {
+                    // Reserved bits in the PS register, these must be written as 0.
+                    const RESERVED_MASK: u32 = 0b1111_1111_1111_1000_1111_0000_0000_0000;
+                    if token & RESERVED_MASK != 0 {
+                        // We could do this transformation in fmt.rs automatically, but experiments
+                        // show this is only worth it in terms of binary size for code inlined into many places.
+                        #[cold]
+                        #[inline(never)]
+                        fn __assert_failed() {
+                            panic!("Reserved bits in PS register must be written as 0");
+                        }
+
+                        __assert_failed();
+                    }
+                }
                 unsafe {
                     core::arch::asm!(
                         "wsr.ps {0}",