Merge pull request #35 from Jim-Hodapp-Coaching/refactor_error_handling

Refactor error handling

Author: jhodapp

cross/get_fw_version/src/main.rs

@@ -63,10 +63,7 @@ fn main() -> ! {
     .ok()
     .unwrap();
 
-    let mut delay = cortex_m::delay::Delay::new(
-        core.SYST,
-        clocks.system_clock.freq().integer(),
-    );
+    let mut delay = cortex_m::delay::Delay::new(core.SYST, clocks.system_clock.freq().integer());
 
     // The single-cycle I/O block controls our GPIO pins
     let sio = hal::Sio::new(pac.SIO);
@@ -89,14 +86,14 @@ fn main() -> ! {
     let spi = hal::Spi::<_, _, 8>::new(pac.SPI0);
 
     // Exchange the uninitialized SPI driver for an initialized one
-    let spi = spi.init(
+    let mut spi = spi.init(
         &mut pac.RESETS,
         clocks.peripheral_clock.freq(),
         8_000_000u32.Hz(),
         &MODE_0,
     );
 
-    let esp_pins = esp32_wroom_rp::gpio::EspControlPins {
+    let mut esp_pins = esp32_wroom_rp::gpio::EspControlPins {
         // CS on pin x (GPIO7)
         cs: pins.gpio7.into_mode::<hal::gpio::PushPullOutput>(),
         // GPIO0 on pin x (GPIO2)
@@ -106,7 +103,7 @@ fn main() -> ! {
         // ACK on pin x (GPIO10)
         ack: pins.gpio10.into_mode::<hal::gpio::FloatingInput>(),
     };
-    let mut wifi = esp32_wroom_rp::wifi::Wifi::init(spi, esp_pins, &mut delay).unwrap();
+    let mut wifi = esp32_wroom_rp::wifi::Wifi::init(&mut spi, &mut esp_pins, &mut delay).unwrap();
     let firmware_version = wifi.firmware_version();
     defmt::info!("NINA firmware version: {:?}", firmware_version);
 

cross/join/src/main.rs

@@ -66,10 +66,7 @@ fn main() -> ! {
     .ok()
     .unwrap();
 
-    let mut delay = cortex_m::delay::Delay::new(
-        core.SYST,
-        clocks.system_clock.freq().integer(),
-    );
+    let mut delay = cortex_m::delay::Delay::new(core.SYST, clocks.system_clock.freq().integer());
 
     // The single-cycle I/O block controls our GPIO pins
     let sio = hal::Sio::new(pac.SIO);
@@ -92,14 +89,14 @@ fn main() -> ! {
     let spi = hal::Spi::<_, _, 8>::new(pac.SPI0);
 
     // Exchange the uninitialized SPI driver for an initialized one
-    let spi = spi.init(
+    let mut spi = spi.init(
         &mut pac.RESETS,
         clocks.peripheral_clock.freq(),
         8_000_000u32.Hz(),
         &MODE_0,
     );
 
-    let esp_pins = esp32_wroom_rp::gpio::EspControlPins {
+    let mut esp_pins = esp32_wroom_rp::gpio::EspControlPins {
         // CS on pin x (GPIO7)
         cs: pins.gpio7.into_mode::<hal::gpio::PushPullOutput>(),
         // GPIO0 on pin x (GPIO2)
@@ -110,7 +107,8 @@ fn main() -> ! {
         ack: pins.gpio10.into_mode::<hal::gpio::FloatingInput>(),
     };
 
-    let mut wifi = esp32_wroom_rp::wifi::Wifi::init(spi, esp_pins, &mut delay).unwrap();
+    let mut wifi = esp32_wroom_rp::wifi::Wifi::init(&mut spi, &mut esp_pins, &mut delay).unwrap();
+
     let result = wifi.join(SSID, PASSPHRASE);
     defmt::info!("Join Result: {:?}", result);
 

esp32-wroom-rp/src/gpio.rs

@@ -41,7 +41,7 @@ enum IOError {
 
 /// Provides an internal pin interface that abstracts the extra control lines that
 /// are separate from a data bus (e.g. SPI/I2C).
-/// 
+///
 /// Not meant to be used outside of the crate.
 pub trait EspControlInterface {
     /// Initializes all controls pins to set ready communication with the NINA firmware.
@@ -75,7 +75,7 @@ pub trait EspControlInterface {
 /// A structured representation of all GPIO pins that control a ESP32-WROOM NINA firmware-based
 /// device outside of commands sent over the SPI/I²C bus. Pass a single instance of this struct
 /// into `Wifi::init()`.
-pub struct EspControlPins<CS: OutputPin, GPIO0: OutputPin, RESETN: OutputPin, ACK: InputPin> {
+pub struct EspControlPins<CS, GPIO0, RESETN, ACK> {
     /// Chip select pin to let the NINA firmware know we're going to send it a command over
     /// the SPI bus.
     pub cs: CS,
@@ -98,27 +98,27 @@ where
 {
     fn init(&mut self) {
         // Chip select is active-low, so we'll initialize it to a driven-high state
-        self.cs.set_high().ok().unwrap();
-        self.gpio0.set_high().ok().unwrap();
-        self.resetn.set_high().ok().unwrap();
+        self.cs.set_high().ok();
+        self.gpio0.set_high().ok();
+        self.resetn.set_high().ok();
         self.get_esp_ready();
     }
 
     fn reset<D: DelayMs<u16>>(&mut self, delay: &mut D) {
-        self.gpio0.set_high().ok().unwrap();
-        self.cs.set_high().ok().unwrap();
-        self.resetn.set_low().ok().unwrap();
+        self.gpio0.set_high().ok();
+        self.cs.set_high().ok();
+        self.resetn.set_low().ok();
         delay.delay_ms(10);
-        self.resetn.set_high().ok().unwrap();
+        self.resetn.set_high().ok();
         delay.delay_ms(750);
     }
 
     fn esp_select(&mut self) {
-        self.cs.set_low().ok().unwrap();
+        self.cs.set_low().ok();
     }
 
     fn esp_deselect(&mut self) {
-        self.cs.set_high().ok().unwrap();
+        self.cs.set_high().ok();
     }
 
     fn get_esp_ready(&self) -> bool {
@@ -131,13 +131,13 @@ where
 
     fn wait_for_esp_ready(&self) {
         while self.get_esp_ready() != true {
-            cortex_m::asm::nop(); // Make sure rustc doesn't optimize this loop out
+            //cortex_m::asm::nop(); // Make sure rustc doesn't optimize this loop out
         }
     }
 
     fn wait_for_esp_ack(&self) {
         while self.get_esp_ack() == false {
-            cortex_m::asm::nop(); // Make sure rustc doesn't optimize this loop out
+            //cortex_m::asm::nop(); // Make sure rustc doesn't optimize this loop out
         }
     }
 
@@ -148,6 +148,17 @@ where
     }
 }
 
+impl Default for EspControlPins<(), (), (), ()> {
+    fn default() -> Self {
+        Self {
+            cs: (),
+            gpio0: (),
+            resetn: (),
+            ack: (),
+        }
+    }
+}
+
 #[cfg(test)]
 mod gpio_tests {
     use super::EspControlPins;

esp32-wroom-rp/src/lib.rs

@@ -75,7 +75,7 @@
 //!     ack: pins.gpio10.into_mode::<hal::gpio::FloatingInput>(),
 //! };
 //!
-//! let mut wifi = esp32_wroom_rp::spi::Wifi::init(spi, esp_pins, &mut delay).unwrap();
+//! let mut wifi = esp32_wroom_rp::spi::Wifi::init(&mut spi, &mut esp_pins, &mut delay).unwrap();
 //! let version = wifi.firmware_version();
 //! ```
 
@@ -88,28 +88,41 @@ pub mod gpio;
 /// Fundamental interface for controlling a connected ESP32-WROOM NINA firmware-based Wifi board.
 pub mod wifi;
 
-mod protocol;
+pub mod protocol;
 mod spi;
 
-use protocol::ProtocolInterface;
+use protocol::{ProtocolError, ProtocolInterface};
 
 use defmt::{write, Format, Formatter};
 use embedded_hal::blocking::delay::DelayMs;
 
 const ARRAY_LENGTH_PLACEHOLDER: usize = 8;
 
 /// Highest level error types for this crate.
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub enum Error {
     /// SPI/I2C related communications error with the ESP32 WiFi target
     Bus,
-    /// Timeout in communicating with the ESP32 WiFi target
-    TimeOut,
+    /// Protocol error in communicating with the ESP32 WiFi target
+    Protocol(ProtocolError),
 }
 
 impl Format for Error {
     fn format(&self, fmt: Formatter) {
-        write!(fmt, "Generic ESP32-WROOM-RP Error")
+        match self {
+            Error::Bus => write!(fmt, "Bus error"),
+            Error::Protocol(e) => write!(
+                fmt,
+                "Communication protocol error with ESP32 WiFi target: {}",
+                e
+            ),
+        }
+    }
+}
+
+impl From<protocol::ProtocolError> for Error {
+    fn from(err: protocol::ProtocolError) -> Self {
+        Error::Protocol(err)
     }
 }
 
@@ -152,7 +165,7 @@ impl Format for FirmwareVersion {
     }
 }
 
-#[derive(Debug, Default)]
+#[derive(Debug)]
 struct WifiCommon<PH> {
     protocol_handler: PH,
 }
@@ -170,20 +183,20 @@ where
         self.protocol_handler.reset(delay)
     }
 
-    fn firmware_version(&mut self) -> Result<FirmwareVersion, self::Error> {
-        self.protocol_handler.get_fw_version()
+    fn firmware_version(&mut self) -> Result<FirmwareVersion, Error> {
+        Ok(self.protocol_handler.get_fw_version()?)
     }
 
     fn join(&mut self, ssid: &str, passphrase: &str) -> Result<(), Error> {
-        self.protocol_handler.set_passphrase(ssid, passphrase)
+        Ok(self.protocol_handler.set_passphrase(ssid, passphrase)?)
     }
 
     fn leave(&mut self) -> Result<(), Error> {
-        self.protocol_handler.disconnect()
+        Ok(self.protocol_handler.disconnect()?)
     }
 
-    fn get_connection_status(&mut self) -> Result<u8, self::Error> {
-        self.protocol_handler.get_conn_status()
+    fn get_connection_status(&mut self) -> Result<u8, Error> {
+        Ok(self.protocol_handler.get_conn_status()?)
     }
 }
 

esp32-wroom-rp/src/protocol.rs

@@ -1,12 +1,14 @@
-pub mod operation;
+pub(crate) mod operation;
 
 use super::*;
 
 use embedded_hal::blocking::delay::DelayMs;
 
+use defmt::{write, Format, Formatter};
+
 use heapless::{String, Vec};
 
-pub const MAX_NINA_PARAM_LENGTH: usize = 255;
+pub(crate) const MAX_NINA_PARAM_LENGTH: usize = 255;
 
 #[repr(u8)]
 #[derive(Copy, Clone, Debug)]
@@ -223,16 +225,39 @@ impl NinaParam for NinaLargeArrayParam {
 pub(crate) trait ProtocolInterface {
     fn init(&mut self);
     fn reset<D: DelayMs<u16>>(&mut self, delay: &mut D);
-    fn get_fw_version(&mut self) -> Result<FirmwareVersion, self::Error>;
-    fn set_passphrase(&mut self, ssid: &str, passphrase: &str) -> Result<(), Error>;
-    fn disconnect(&mut self) -> Result<(), self::Error>;
-    fn get_conn_status(&mut self) -> Result<u8, self::Error>;
+    fn get_fw_version(&mut self) -> Result<FirmwareVersion, ProtocolError>;
+    fn set_passphrase(&mut self, ssid: &str, passphrase: &str) -> Result<(), ProtocolError>;
+    fn disconnect(&mut self) -> Result<(), ProtocolError>;
+    fn get_conn_status(&mut self) -> Result<u8, ProtocolError>;
 }
 
-#[derive(Debug, Default)]
-pub(crate) struct NinaProtocolHandler<B, C> {
+#[derive(Debug)]
+pub(crate) struct NinaProtocolHandler<'a, B, C> {
     /// A Spi or I2c instance
-    pub bus: B,
+    pub bus: &'a mut B,
     /// An EspControlPins instance
-    pub control_pins: C,
+    pub control_pins: &'a mut C,
+}
+
+// TODO: look at Nina Firmware code to understand conditions
+// that lead to NinaProtocolVersionMismatch
+#[derive(Debug, PartialEq)]
+pub enum ProtocolError {
+    NinaProtocolVersionMismatch,
+    CommunicationTimeout,
+    InvalidCommand,
+    InvalidNumberOfParameters,
+    TooManyParameters,
+}
+
+impl Format for ProtocolError {
+    fn format(&self, fmt: Formatter) {
+        match self {
+            ProtocolError::NinaProtocolVersionMismatch => write!(fmt, "Encountered an unsupported version of the NINA protocol."),
+            ProtocolError::CommunicationTimeout => write!(fmt, "Communication with ESP32 target timed out."),
+            ProtocolError::InvalidCommand => write!(fmt, "Encountered an invalid command while communicating with ESP32 target."),
+            ProtocolError::InvalidNumberOfParameters => write!(fmt, "Encountered an unexpected number of parameters for a NINA command while communicating with ESP32 target."),
+            ProtocolError::TooManyParameters => write!(fmt, "Encountered too many parameters for a NINA command while communicating with ESP32 target."),
+        }
+    }
 }