Implement InterruptConfigurable (esp-rs#1819)

* Implement `InterruptConfigurable`

* Fix doc-tests

* Clippy

* Fix lp_core_uart example

* CHANGELOG.md

* Have DEFAULT_INTERRUPT_HANDLER

* Fix docs

* Clippy

* Add `set_interrupt_handler` for WDT

esp-hal/CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 ### Changed
+- Peripheral driver constructors don't take `InterruptHandler`s anymore. Use `set_interrupt_handler` to explicitly set the interrupt handler now. (#1819)
 
 ### Fixed
 

esp-hal/src/assist_debug.rs

@@ -19,14 +19,15 @@
 //!
 //! [Debug Assist]: https://github.com/esp-rs/esp-hal/blob/main/examples/src/bin/debug_assist.rs
 //!
-//! ## Implmentation State
+//! ## Implementation State
 //! - Bus write access logging is not available via this API
 //! - This driver has only blocking API
 
 use crate::{
     interrupt::InterruptHandler,
     peripheral::{Peripheral, PeripheralRef},
     peripherals::ASSIST_DEBUG,
+    InterruptConfigurable,
 };
 
 /// The debug assist driver instance.
@@ -36,35 +37,34 @@ pub struct DebugAssist<'d> {
 
 impl<'d> DebugAssist<'d> {
     /// Create a new instance in [crate::Blocking] mode.
-    ///
-    /// Optionally an interrupt handler can be bound.
-    pub fn new(
-        debug_assist: impl Peripheral<P = ASSIST_DEBUG> + 'd,
-        interrupt: Option<InterruptHandler>,
-    ) -> Self {
+    pub fn new(debug_assist: impl Peripheral<P = ASSIST_DEBUG> + 'd) -> Self {
         crate::into_ref!(debug_assist);
 
         // NOTE: We should enable the debug assist, however, it's always enabled in ROM
         //       code already.
 
-        if let Some(interrupt) = interrupt {
-            unsafe {
-                crate::interrupt::bind_interrupt(
-                    crate::peripherals::Interrupt::ASSIST_DEBUG,
-                    interrupt.handler(),
-                );
-                crate::interrupt::enable(
-                    crate::peripherals::Interrupt::ASSIST_DEBUG,
-                    interrupt.priority(),
-                )
-                .unwrap();
-            }
-        }
-
         DebugAssist { debug_assist }
     }
 }
 
+impl<'d> crate::private::Sealed for DebugAssist<'d> {}
+
+impl<'d> InterruptConfigurable for DebugAssist<'d> {
+    fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
+        unsafe {
+            crate::interrupt::bind_interrupt(
+                crate::peripherals::Interrupt::ASSIST_DEBUG,
+                handler.handler(),
+            );
+            crate::interrupt::enable(
+                crate::peripherals::Interrupt::ASSIST_DEBUG,
+                handler.priority(),
+            )
+            .unwrap();
+        }
+    }
+}
+
 #[cfg(assist_debug_sp_monitor)]
 impl<'d> DebugAssist<'d> {
     /// Enable SP monitoring on main core. When the SP exceeds the

esp-hal/src/ecc.rs

@@ -33,6 +33,7 @@ use crate::{
     peripherals::ECC,
     reg_access::{AlignmentHelper, SocDependentEndianess},
     system::{Peripheral as PeripheralEnable, PeripheralClockControl},
+    InterruptConfigurable,
 };
 
 /// The ECC Accelerator driver instance
@@ -81,24 +82,11 @@ pub enum WorkMode {
 
 impl<'d> Ecc<'d, crate::Blocking> {
     /// Create a new instance in [crate::Blocking] mode.
-    ///
-    /// Optionally an interrupt handler can be bound.
-    pub fn new(ecc: impl Peripheral<P = ECC> + 'd, interrupt: Option<InterruptHandler>) -> Self {
+    pub fn new(ecc: impl Peripheral<P = ECC> + 'd) -> Self {
         crate::into_ref!(ecc);
 
         PeripheralClockControl::enable(PeripheralEnable::Ecc);
 
-        if let Some(interrupt) = interrupt {
-            unsafe {
-                crate::interrupt::bind_interrupt(
-                    crate::peripherals::Interrupt::ECC,
-                    interrupt.handler(),
-                );
-                crate::interrupt::enable(crate::peripherals::Interrupt::ECC, interrupt.priority())
-                    .unwrap();
-            }
-        }
-
         Self {
             ecc,
             alignment_helper: AlignmentHelper::default(),
@@ -107,6 +95,18 @@ impl<'d> Ecc<'d, crate::Blocking> {
     }
 }
 
+impl<'d> crate::private::Sealed for Ecc<'d, crate::Blocking> {}
+
+impl<'d> InterruptConfigurable for Ecc<'d, crate::Blocking> {
+    fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
+        unsafe {
+            crate::interrupt::bind_interrupt(crate::peripherals::Interrupt::ECC, handler.handler());
+            crate::interrupt::enable(crate::peripherals::Interrupt::ECC, handler.priority())
+                .unwrap();
+        }
+    }
+}
+
 impl<'d, DM: crate::Mode> Ecc<'d, DM> {
     pub fn reset(&mut self) {
         self.ecc.mult_conf().reset()

esp-hal/src/gpio/mod.rs

@@ -64,6 +64,7 @@ use crate::{
     peripheral::PeripheralRef,
     peripherals::{GPIO, IO_MUX},
     private,
+    InterruptConfigurable,
 };
 
 pub mod any_pin;
@@ -1212,15 +1213,19 @@ impl Io {
             pins,
         }
     }
+}
+
+impl crate::private::Sealed for Io {}
 
+impl InterruptConfigurable for Io {
     /// Install the given interrupt handler replacing any previously set
     /// handler.
     ///
     /// When the async feature is enabled the handler will be called first and
     /// the internal async handler will run after. In that case it's
     /// important to not reset the interrupt status when mixing sync and
     /// async (i.e. using async wait) interrupt handling.
-    pub fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
+    fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
         critical_section::with(|cs| {
             crate::interrupt::enable(crate::peripherals::Interrupt::GPIO, handler.priority())
                 .unwrap();

esp-hal/src/i2c.rs

@@ -33,7 +33,6 @@
 //!     io.pins.gpio2,
 //!     100.kHz(),
 //!     &clocks,
-//!     None,
 //! );
 //! # }
 //! ```
@@ -62,7 +61,6 @@
 //!     io.pins.gpio2,
 //!     100.kHz(),
 //!     &clocks,
-//!     None,
 //! );
 //! loop {
 //!     let mut data = [0u8; 22];
@@ -82,6 +80,7 @@ use crate::{
     peripheral::{Peripheral, PeripheralRef},
     peripherals::i2c0::{RegisterBlock, COMD},
     system::PeripheralClockControl,
+    InterruptConfigurable,
 };
 
 cfg_if::cfg_if! {
@@ -453,7 +452,6 @@ where
         frequency: HertzU32,
         clocks: &Clocks,
         timeout: Option<u32>,
-        isr: Option<InterruptHandler>,
     ) -> Self {
         crate::into_ref!(i2c, sda, scl);
 
@@ -498,15 +496,14 @@ where
         );
 
         i2c.peripheral.setup(frequency, clocks, timeout);
+        i2c
+    }
 
-        if let Some(interrupt) = isr {
-            unsafe {
-                crate::interrupt::bind_interrupt(T::interrupt(), interrupt.handler());
-                crate::interrupt::enable(T::interrupt(), interrupt.priority()).unwrap();
-            }
+    fn internal_set_interrupt_handler(&mut self, handler: InterruptHandler) {
+        unsafe {
+            crate::interrupt::bind_interrupt(T::interrupt(), handler.handler());
+            crate::interrupt::enable(T::interrupt(), handler.priority()).unwrap();
         }
-
-        i2c
     }
 }
 
@@ -523,9 +520,8 @@ where
         scl: impl Peripheral<P = SCL> + 'd,
         frequency: HertzU32,
         clocks: &Clocks,
-        isr: Option<InterruptHandler>,
     ) -> Self {
-        Self::new_with_timeout(i2c, sda, scl, frequency, clocks, None, isr)
+        Self::new_with_timeout(i2c, sda, scl, frequency, clocks, None)
     }
 
     /// Create a new I2C instance with a custom timeout value.
@@ -538,9 +534,19 @@ where
         frequency: HertzU32,
         clocks: &Clocks,
         timeout: Option<u32>,
-        isr: Option<InterruptHandler>,
     ) -> Self {
-        Self::new_internal(i2c, sda, scl, frequency, clocks, timeout, isr)
+        Self::new_internal(i2c, sda, scl, frequency, clocks, timeout)
+    }
+}
+
+impl<'d, T> crate::private::Sealed for I2C<'d, T, crate::Blocking> where T: Instance {}
+
+impl<'d, T> InterruptConfigurable for I2C<'d, T, crate::Blocking>
+where
+    T: Instance,
+{
+    fn set_interrupt_handler(&mut self, handler: crate::interrupt::InterruptHandler) {
+        self.internal_set_interrupt_handler(handler);
     }
 }
 
@@ -573,13 +579,17 @@ where
         clocks: &Clocks,
         timeout: Option<u32>,
     ) -> Self {
-        let handler = Some(match T::I2C_NUMBER {
+        let mut this = Self::new_internal(i2c, sda, scl, frequency, clocks, timeout);
+
+        let handler = match T::I2C_NUMBER {
             0 => asynch::i2c0_handler,
             #[cfg(i2c1)]
             1 => asynch::i2c1_handler,
             _ => panic!("Unexpected I2C peripheral"),
-        });
-        Self::new_internal(i2c, sda, scl, frequency, clocks, timeout, handler)
+        };
+        this.internal_set_interrupt_handler(handler);
+
+        this
     }
 
     pub(crate) fn inner(&self) -> &T {

esp-hal/src/i2s.rs

@@ -113,6 +113,7 @@ use crate::{
     into_ref,
     peripheral::Peripheral,
     system::PeripheralClockControl,
+    InterruptConfigurable,
     Mode,
 };
 
@@ -363,8 +364,7 @@ where
     CH: DmaChannel,
     DmaMode: Mode,
 {
-    /// Sets the interrupt handler, enables it with
-    /// [crate::interrupt::Priority::min()]
+    /// Sets the interrupt handler
     ///
     /// Interrupts are not enabled at the peripheral level here.
     pub fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
@@ -392,6 +392,25 @@ where
     }
 }
 
+impl<'d, I, CH, DmaMode> crate::private::Sealed for I2s<'d, I, CH, DmaMode>
+where
+    I: RegisterAccess,
+    CH: DmaChannel,
+    DmaMode: Mode,
+{
+}
+
+impl<'d, I, CH, DmaMode> InterruptConfigurable for I2s<'d, I, CH, DmaMode>
+where
+    I: RegisterAccess,
+    CH: DmaChannel,
+    DmaMode: Mode,
+{
+    fn set_interrupt_handler(&mut self, handler: crate::interrupt::InterruptHandler) {
+        I2s::set_interrupt_handler(self, handler);
+    }
+}
+
 impl<'d, I, CH, DmaMode> I2s<'d, I, CH, DmaMode>
 where
     I: RegisterAccess,

esp-hal/src/lib.rs

@@ -607,6 +607,26 @@ impl<T, const SIZE: usize> FlashSafeDma<T, SIZE> {
     }
 }
 
+/// Default (unhandled) interrupt handler
+pub const DEFAULT_INTERRUPT_HANDLER: interrupt::InterruptHandler = interrupt::InterruptHandler::new(
+    unsafe { core::mem::transmute::<*const (), extern "C" fn()>(EspDefaultHandler as *const ()) },
+    crate::interrupt::Priority::min(),
+);
+
+/// Trait implemented by drivers which allow the user to set an
+/// [interrupt::InterruptHandler]
+pub trait InterruptConfigurable: private::Sealed {
+    /// Set the interrupt handler
+    ///
+    /// Note that this will replace any previously registered interrupt handler.
+    /// Some peripherals offer a shared interrupt handler for multiple purposes.
+    /// It's the users duty to honor this.
+    ///
+    /// You can restore the default/unhandled interrupt handler by using
+    /// [DEFAULT_INTERRUPT_HANDLER]
+    fn set_interrupt_handler(&mut self, handler: interrupt::InterruptHandler);
+}
+
 #[cfg(riscv)]
 #[export_name = "hal_main"]
 fn hal_main(a0: usize, a1: usize, a2: usize) -> ! {