More robust handling of responses from NINA-FW

esp32-wroom-rp/src/gpio.rs

@@ -30,6 +30,8 @@
 //! };
 //! ```
 
+use core::hint;
+
 use embedded_hal::blocking::delay::DelayMs;
 use embedded_hal::digital::v2::{InputPin, OutputPin};
 
@@ -131,13 +133,13 @@ where
 
     fn wait_for_esp_ready(&self) {
         while !self.get_esp_ready() {
-            //cortex_m::asm::nop(); // Make sure rustc doesn't optimize this loop out
+            hint::spin_loop(); // Make sure rustc doesn't optimize this loop out
         }
     }
 
     fn wait_for_esp_ack(&self) {
         while !self.get_esp_ack() {
-            //cortex_m::asm::nop(); // Make sure rustc doesn't optimize this loop out
+            hint::spin_loop(); // Make sure rustc doesn't optimize this loop out
         }
     }
 

esp32-wroom-rp/src/lib.rs

@@ -163,8 +163,6 @@ use network::NetworkError;
 
 use protocol::ProtocolError;
 
-const ARRAY_LENGTH_PLACEHOLDER: usize = 8;
-
 /// Highest level error types for this crate.
 #[derive(Debug, Eq, PartialEq)]
 pub enum Error {
@@ -212,17 +210,15 @@ pub struct FirmwareVersion {
 }
 
 impl FirmwareVersion {
-    fn new(version: [u8; ARRAY_LENGTH_PLACEHOLDER]) -> FirmwareVersion {
+    fn new(version: &[u8]) -> FirmwareVersion {
         Self::parse(version)
     }
 
     // Takes in 8 bytes (e.g. 1.7.4) and returns a FirmwareVersion instance
-    fn parse(version: [u8; ARRAY_LENGTH_PLACEHOLDER]) -> FirmwareVersion {
-        let major_version: u8;
-        let minor_version: u8;
-        let patch_version: u8;
-
-        [major_version, _, minor_version, _, patch_version, _, _, _] = version;
+    fn parse(version: &[u8]) -> FirmwareVersion {
+        let major_version: u8 = version[0];
+        let minor_version: u8 = version[2];
+        let patch_version: u8 = version[4];
 
         FirmwareVersion {
             major: major_version,
@@ -248,8 +244,7 @@ mod tests {
 
     #[test]
     fn firmware_new_returns_a_populated_firmware_struct() {
-        let firmware_version: FirmwareVersion =
-            FirmwareVersion::new([0x1, 0x2e, 0x7, 0x2e, 0x4, 0x0, 0x0, 0x0]);
+        let firmware_version: FirmwareVersion = FirmwareVersion::new(&[0x1, 0x2e, 0x7, 0x2e, 0x4]);
 
         assert_eq!(
             firmware_version,

esp32-wroom-rp/src/protocol.rs

@@ -13,9 +13,18 @@ use heapless::{String, Vec};
 
 use super::network::{ConnectionState, IpAddress, Port, Socket, TransportMode};
 use super::wifi::ConnectionStatus;
-use super::{Error, FirmwareVersion, ARRAY_LENGTH_PLACEHOLDER};
+use super::{Error, FirmwareVersion};
 
-pub(crate) const MAX_NINA_PARAM_LENGTH: usize = 4096;
+// The maximum number of NINA param u8 bytes in a command send/receive byte stream
+pub(crate) const MAX_NINA_PARAMS: usize = 8;
+
+pub(crate) const MAX_NINA_BYTE_PARAM_BUFFER_LENGTH: usize = 1;
+pub(crate) const MAX_NINA_WORD_PARAM_BUFFER_LENGTH: usize = 2;
+pub(crate) const MAX_NINA_SMALL_ARRAY_PARAM_BUFFER_LENGTH: usize = 255;
+pub(crate) const MAX_NINA_LARGE_ARRAY_PARAM_BUFFER_LENGTH: usize = 1024;
+
+// The maximum length that a 2-byte length NINA response can be
+pub(crate) const MAX_NINA_RESPONSE_LENGTH: usize = 1064;
 
 #[repr(u8)]
 #[derive(Copy, Clone, Debug)]
@@ -35,6 +44,7 @@ pub(crate) enum NinaCommand {
 }
 
 pub(crate) trait NinaConcreteParam {
+    type DataBuffer;
     // Length of parameter in bytes
     type LengthAsBytes: IntoIterator<Item = u8>;
 
@@ -49,35 +59,6 @@ pub(crate) trait NinaConcreteParam {
     fn length(&self) -> u16;
 }
 
-// Used for Nina protocol commands with no parameters
-pub(crate) struct NinaNoParams {
-    _placeholder: u8,
-}
-
-impl NinaConcreteParam for NinaNoParams {
-    type LengthAsBytes = [u8; 0];
-
-    fn new(_data: &str) -> Self {
-        Self { _placeholder: 0 }
-    }
-
-    fn from_bytes(_bytes: &[u8]) -> Self {
-        Self { _placeholder: 0 }
-    }
-
-    fn data(&self) -> &[u8] {
-        &[0u8]
-    }
-
-    fn length_as_bytes(&self) -> Self::LengthAsBytes {
-        []
-    }
-
-    fn length(&self) -> u16 {
-        0u16
-    }
-}
-
 pub(crate) trait NinaParam {
     fn length_as_bytes(&self) -> [u8; 2];
     fn data(&self) -> &[u8];
@@ -88,32 +69,32 @@ pub(crate) trait NinaParam {
 // Used for single byte params
 pub(crate) struct NinaByteParam {
     length: u8,
-    data: Vec<u8, 1>,
+    data: <NinaByteParam as NinaConcreteParam>::DataBuffer,
 }
 
 // Used for 2-byte params
 pub(crate) struct NinaWordParam {
     length: u8,
-    data: Vec<u8, 2>,
+    data: <NinaWordParam as NinaConcreteParam>::DataBuffer,
 }
 
 // Used for params that are smaller than 255 bytes
 pub(crate) struct NinaSmallArrayParam {
     length: u8,
-    data: Vec<u8, MAX_NINA_PARAM_LENGTH>,
+    data: <NinaSmallArrayParam as NinaConcreteParam>::DataBuffer,
 }
 
 // Used for params that can be larger than 255 bytes up to MAX_NINA_PARAM_LENGTH
 pub(crate) struct NinaLargeArrayParam {
     length: u16,
-    data: Vec<u8, MAX_NINA_PARAM_LENGTH>,
+    data: <NinaLargeArrayParam as NinaConcreteParam>::DataBuffer,
 }
 
 pub(crate) struct NinaAbstractParam {
     // Byte representation of length of data
     length_as_bytes: [u8; 2],
-    // Data to be transfered over SPI bus
-    data: Vec<u8, MAX_NINA_PARAM_LENGTH>,
+    // Data to be transferred over SPI bus
+    data: <NinaLargeArrayParam as NinaConcreteParam>::DataBuffer,
     // Number of bytes in data
     length: u16,
     // The number of bytes needed to represent
@@ -139,17 +120,6 @@ impl NinaParam for NinaAbstractParam {
     }
 }
 
-impl From<NinaNoParams> for NinaAbstractParam {
-    fn from(concrete_param: NinaNoParams) -> NinaAbstractParam {
-        NinaAbstractParam {
-            length_as_bytes: [0, 0],
-            data: Vec::from_slice(concrete_param.data()).unwrap(),
-            length: concrete_param.length(),
-            length_size: 0,
-        }
-    }
-}
-
 impl From<NinaByteParam> for NinaAbstractParam {
     fn from(concrete_param: NinaByteParam) -> NinaAbstractParam {
         NinaAbstractParam {
@@ -195,18 +165,20 @@ impl From<NinaLargeArrayParam> for NinaAbstractParam {
 }
 
 impl NinaConcreteParam for NinaByteParam {
+    type DataBuffer = Vec<u8, MAX_NINA_BYTE_PARAM_BUFFER_LENGTH>;
     type LengthAsBytes = [u8; 1];
 
     fn new(data: &str) -> Self {
-        let data_as_bytes: Vec<u8, 1> = String::from(data).into_bytes();
+        let data_as_bytes: Self::DataBuffer = String::from(data).into_bytes();
         Self {
             length: data_as_bytes.len() as u8,
             data: data_as_bytes,
         }
     }
 
     fn from_bytes(bytes: &[u8]) -> Self {
-        let mut data_as_bytes: Vec<u8, 1> = Vec::new();
+        let mut data_as_bytes: Self::DataBuffer = Vec::new();
+
         data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
         Self {
             length: data_as_bytes.len() as u8,
@@ -228,18 +200,19 @@ impl NinaConcreteParam for NinaByteParam {
 }
 
 impl NinaConcreteParam for NinaWordParam {
+    type DataBuffer = Vec<u8, MAX_NINA_WORD_PARAM_BUFFER_LENGTH>;
     type LengthAsBytes = [u8; 1];
 
     fn new(data: &str) -> Self {
-        let data_as_bytes: Vec<u8, 2> = String::from(data).into_bytes();
+        let data_as_bytes: Self::DataBuffer = String::from(data).into_bytes();
         Self {
             length: data_as_bytes.len() as u8,
             data: data_as_bytes,
         }
     }
 
     fn from_bytes(bytes: &[u8]) -> Self {
-        let mut data_as_bytes: Vec<u8, 2> = Vec::new();
+        let mut data_as_bytes: Self::DataBuffer = Vec::new();
         data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
         Self {
             length: data_as_bytes.len() as u8,
@@ -261,18 +234,19 @@ impl NinaConcreteParam for NinaWordParam {
 }
 
 impl NinaConcreteParam for NinaSmallArrayParam {
+    type DataBuffer = Vec<u8, MAX_NINA_SMALL_ARRAY_PARAM_BUFFER_LENGTH>;
     type LengthAsBytes = [u8; 1];
 
     fn new(data: &str) -> Self {
-        let data_as_bytes: Vec<u8, MAX_NINA_PARAM_LENGTH> = String::from(data).into_bytes();
+        let data_as_bytes: Self::DataBuffer = String::from(data).into_bytes();
         Self {
             length: data_as_bytes.len() as u8,
             data: data_as_bytes,
         }
     }
 
     fn from_bytes(bytes: &[u8]) -> Self {
-        let mut data_as_bytes: Vec<u8, MAX_NINA_PARAM_LENGTH> = Vec::new();
+        let mut data_as_bytes: Self::DataBuffer = Vec::new();
         data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
         Self {
             length: data_as_bytes.len() as u8,
@@ -294,18 +268,19 @@ impl NinaConcreteParam for NinaSmallArrayParam {
 }
 
 impl NinaConcreteParam for NinaLargeArrayParam {
+    type DataBuffer = Vec<u8, MAX_NINA_LARGE_ARRAY_PARAM_BUFFER_LENGTH>;
     type LengthAsBytes = [u8; 2];
 
     fn new(data: &str) -> Self {
-        let data_as_bytes: Vec<u8, MAX_NINA_PARAM_LENGTH> = String::from(data).into_bytes();
+        let data_as_bytes: Self::DataBuffer = String::from(data).into_bytes();
         Self {
             length: data_as_bytes.len() as u16,
             data: data_as_bytes,
         }
     }
 
     fn from_bytes(bytes: &[u8]) -> Self {
-        let mut data_as_bytes: Vec<u8, MAX_NINA_PARAM_LENGTH> = Vec::new();
+        let mut data_as_bytes: Self::DataBuffer = Vec::new();
         data_as_bytes.extend_from_slice(bytes).unwrap_or_default();
         Self {
             length: data_as_bytes.len() as u16,
@@ -338,7 +313,7 @@ pub(crate) trait ProtocolInterface {
     fn get_conn_status(&mut self) -> Result<ConnectionStatus, Error>;
     fn set_dns_config(&mut self, dns1: IpAddress, dns2: Option<IpAddress>) -> Result<(), Error>;
     fn req_host_by_name(&mut self, hostname: &str) -> Result<u8, Error>;
-    fn get_host_by_name(&mut self) -> Result<[u8; 8], Error>;
+    fn get_host_by_name(&mut self) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error>;
     fn resolve(&mut self, hostname: &str) -> Result<IpAddress, Error>;
     fn get_socket(&mut self) -> Result<Socket, Error>;
     fn start_client_tcp(
@@ -354,7 +329,7 @@ pub(crate) trait ProtocolInterface {
         &mut self,
         data: &str,
         socket: Socket,
-    ) -> Result<[u8; ARRAY_LENGTH_PLACEHOLDER], Error>;
+    ) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error>;
 }
 
 #[derive(Debug)]

esp32-wroom-rp/src/spi.rs

@@ -16,11 +16,11 @@ use super::protocol::operation::Operation;
 use super::protocol::{
     NinaByteParam, NinaCommand, NinaConcreteParam, NinaLargeArrayParam, NinaParam,
     NinaProtocolHandler, NinaSmallArrayParam, NinaWordParam, ProtocolError, ProtocolInterface,
+    MAX_NINA_PARAMS, MAX_NINA_RESPONSE_LENGTH,
 };
 use super::wifi::ConnectionStatus;
-use super::{Error, FirmwareVersion, ARRAY_LENGTH_PLACEHOLDER};
+use super::{Error, FirmwareVersion};
 
-// TODO: this should eventually move into NinaCommandHandler
 #[repr(u8)]
 #[derive(Debug)]
 enum ControlByte {
@@ -52,8 +52,9 @@ where
         self.execute(&operation)?;
 
         let result = self.receive(&operation, 1)?;
+        let (version, _) = result.split_at(5);
 
-        Ok(FirmwareVersion::new(result)) // e.g. 1.7.4
+        Ok(FirmwareVersion::new(version)) // e.g. 1.7.4
     }
 
     fn set_passphrase(&mut self, ssid: &str, passphrase: &str) -> Result<(), Error> {
@@ -118,7 +119,7 @@ where
         Ok(result[0])
     }
 
-    fn get_host_by_name(&mut self) -> Result<[u8; 8], Error> {
+    fn get_host_by_name(&mut self) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error> {
         let operation = Operation::new(NinaCommand::GetHostByName);
 
         self.execute(&operation)?;
@@ -212,7 +213,7 @@ where
         &mut self,
         data: &str,
         socket: Socket,
-    ) -> Result<[u8; ARRAY_LENGTH_PLACEHOLDER], Error> {
+    ) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error> {
         let operation = Operation::new(NinaCommand::SendDataTcp)
             .param(NinaLargeArrayParam::from_bytes(&[socket]).into())
             .param(NinaLargeArrayParam::new(data).into());
@@ -270,7 +271,7 @@ where
         &mut self,
         operation: &Operation<P>,
         expected_num_params: u8,
-    ) -> Result<[u8; ARRAY_LENGTH_PLACEHOLDER], Error> {
+    ) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error> {
         self.control_pins.wait_for_esp_select();
 
         let result = self.wait_response_cmd(&operation.command, expected_num_params);
@@ -302,7 +303,7 @@ where
         &mut self,
         cmd: &NinaCommand,
         num_params: u8,
-    ) -> Result<[u8; ARRAY_LENGTH_PLACEHOLDER], Error> {
+    ) -> Result<[u8; MAX_NINA_RESPONSE_LENGTH], Error> {
         self.check_start_cmd()?;
         let byte_to_check: u8 = *cmd as u8 | ControlByte::Reply as u8;
         let result = self.read_and_check_byte(&byte_to_check).ok().unwrap();
@@ -317,21 +318,24 @@ where
             return Err(ProtocolError::InvalidNumberOfParameters.into());
         }
 
-        let num_params_to_read = self.get_byte().ok().unwrap() as usize;
+        let number_of_params_to_read = self.get_byte().ok().unwrap() as usize;
 
-        // TODO: use a constant instead of inline params max == 8
-        if num_params_to_read > 8 {
+        if number_of_params_to_read > MAX_NINA_PARAMS {
             return Err(ProtocolError::TooManyParameters.into());
         }
 
-        let mut params: [u8; ARRAY_LENGTH_PLACEHOLDER] = [0; ARRAY_LENGTH_PLACEHOLDER];
-        for (index, _param) in params.into_iter().enumerate() {
-            params[index] = self.get_byte().ok().unwrap()
+        let mut response_param_buffer: [u8; MAX_NINA_RESPONSE_LENGTH] =
+            [0; MAX_NINA_RESPONSE_LENGTH];
+        if number_of_params_to_read > 0 {
+            for i in 0..number_of_params_to_read {
+                response_param_buffer[i] = self.get_byte().ok().unwrap()
+            }
         }
+
         let control_byte: u8 = ControlByte::End as u8;
         self.read_and_check_byte(&control_byte).ok();
 
-        Ok(params)
+        Ok(response_param_buffer)
     }
 
     fn send_end_cmd(&mut self) -> Result<(), Infallible> {