Address library feedback (#901)

* use stored skip/verify params

* move command structs out of connection module

* changelog

* use scaling timeout for md5 check

Author: MabezDev

.github/workflows/hil.yml

@@ -17,6 +17,7 @@ on:
 
 env:
   CARGO_TERM_COLOR: always
+  RUST_LOG: debug
 
 # Cancel any currently running workflows from the same PR, branch, or
 # tag when a new workflow is triggered.
@@ -113,7 +114,7 @@ jobs:
 
       - name: save-image/write-bin test
         run: |
-          timeout 90 bash espflash/tests/scripts/save-image_write-bin.sh ${{ matrix.board.mcu }}
+          timeout 180 bash espflash/tests/scripts/save-image_write-bin.sh ${{ matrix.board.mcu }}
 
       - name: erase-region test
         run: timeout 30 bash espflash/tests/scripts/erase-region.sh

CHANGELOG.md

@@ -63,6 +63,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Any reference to `esp_idf` or `EspIdf` has been cut to just `idf` (#891)
 - Renamed `targets` module to `target` (#891)
 - Test data is now excluded from the crates.io release (#897)
+- The command module, and `Command` related structs now exist in a top level module, instead of the `connection` module (#901) 
 
 ### Fixed
 
@@ -76,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The app descriptor is now correctly placed in the front of the binary (#835)
 - espflash now extracts the MMU page size from the app descriptor (#835)
 - `ResetBeforeOperation` & `ResetAfterOperation` are now public, to allow the creation of a `Connection` (#895)
+- `Flasher` now respects its internal `verify` and `skip` flags for all methods. (#901)
 
 ### Removed
 

espflash/src/connection/command.rs renamed to espflash/src/command.rs

@@ -5,7 +5,10 @@ use std::{io::Write, mem::size_of, time::Duration};
 use bytemuck::{Pod, Zeroable, bytes_of};
 use strum::Display;
 
-use crate::flasher::{SpiAttachParams, SpiSetParams};
+use crate::{
+    Error,
+    flasher::{SpiAttachParams, SpiSetParams},
+};
 
 const DEFAULT_TIMEOUT: Duration = Duration::from_secs(3);
 const ERASE_REGION_TIMEOUT_PER_MB: Duration = Duration::from_secs(30);
@@ -14,7 +17,7 @@ const ERASE_CHIP_TIMEOUT: Duration = Duration::from_secs(120);
 const MEM_END_TIMEOUT: Duration = Duration::from_millis(50);
 const SYNC_TIMEOUT: Duration = Duration::from_millis(100);
 const FLASH_DEFLATE_END_TIMEOUT: Duration = Duration::from_secs(10);
-const FLASH_MD5_TIMEOUT: Duration = Duration::from_secs(8);
+const FLASH_MD5_TIMEOUT_PER_MB: Duration = Duration::from_secs(8);
 
 /// Input data for SYNC command (36 bytes: 0x07 0x07 0x12 0x20, followed by
 /// 32 x 0x55)
@@ -63,6 +66,82 @@ pub enum CommandType {
     FlashDetect = 0x9F,
 }
 
+/// The value of a command response.
+#[derive(Debug, Clone)]
+pub enum CommandResponseValue {
+    /// A 32-bit value.
+    ValueU32(u32),
+    /// A 128-bit value.
+    ValueU128(u128),
+    /// A vector of bytes.
+    Vector(Vec<u8>),
+}
+
+impl TryInto<u32> for CommandResponseValue {
+    type Error = Error;
+
+    fn try_into(self) -> Result<u32, Self::Error> {
+        match self {
+            CommandResponseValue::ValueU32(value) => Ok(value),
+            CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
+                "expected `u32` but found `u128`".into(),
+            )),
+            CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
+                "expected `u32` but found `Vec`".into(),
+            )),
+        }
+    }
+}
+
+impl TryInto<u128> for CommandResponseValue {
+    type Error = Error;
+
+    fn try_into(self) -> Result<u128, Self::Error> {
+        match self {
+            CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
+                "expected `u128` but found `u32`".into(),
+            )),
+            CommandResponseValue::ValueU128(value) => Ok(value),
+            CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
+                "expected `u128` but found `Vec`".into(),
+            )),
+        }
+    }
+}
+
+impl TryInto<Vec<u8>> for CommandResponseValue {
+    type Error = Error;
+
+    fn try_into(self) -> Result<Vec<u8>, Self::Error> {
+        match self {
+            CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
+                "expected `Vec` but found `u32`".into(),
+            )),
+            CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
+                "expected `Vec` but found `u128`".into(),
+            )),
+            CommandResponseValue::Vector(value) => Ok(value),
+        }
+    }
+}
+
+/// A response from a target device following a command.
+#[derive(Debug, Clone)]
+pub struct CommandResponse {
+    /// The response byte.
+    pub resp: u8,
+    /// The return operation byte.
+    pub return_op: u8,
+    /// The length of the return value.
+    pub return_length: u16,
+    /// The value of the response.
+    pub value: CommandResponseValue,
+    /// The error byte.
+    pub error: u8,
+    /// The status byte.
+    pub status: u8,
+}
+
 impl CommandType {
     /// Return the default timeout for the [`CommandType`] variant.
     pub fn timeout(&self) -> Duration {
@@ -71,7 +150,14 @@ impl CommandType {
             CommandType::Sync => SYNC_TIMEOUT,
             CommandType::EraseFlash => ERASE_CHIP_TIMEOUT,
             CommandType::FlashDeflEnd => FLASH_DEFLATE_END_TIMEOUT,
-            CommandType::FlashMd5 => FLASH_MD5_TIMEOUT,
+            CommandType::FlashMd5 => {
+                log::warn!(
+                    "Using default timeout for {}, this may not be sufficient for large flash regions. Consider using `timeout_for_size` instead.",
+                    self
+                );
+
+                DEFAULT_TIMEOUT
+            }
             _ => DEFAULT_TIMEOUT,
         }
     }
@@ -93,6 +179,7 @@ impl CommandType {
             CommandType::FlashData | CommandType::FlashDeflData => {
                 calc_timeout(ERASE_WRITE_TIMEOUT_PER_MB, size)
             }
+            CommandType::FlashMd5 => calc_timeout(FLASH_MD5_TIMEOUT_PER_MB, size),
             _ => self.timeout(),
         }
     }

espflash/src/connection/mod.rs

@@ -19,7 +19,6 @@ use slip_codec::SlipDecoder;
 #[cfg(unix)]
 use self::reset::UnixTightReset;
 use self::{
-    command::{Command, CommandType},
     encoder::SlipEncoder,
     reset::{
         ClassicReset,
@@ -32,12 +31,12 @@ use self::{
     },
 };
 use crate::{
+    command::{Command, CommandResponse, CommandResponseValue, CommandType},
     error::{ConnectionError, Error, ResultExt, RomError, RomErrorKind},
     flasher::stubs::CHIP_DETECT_MAGIC_REG_ADDR,
     target::Chip,
 };
 
-pub(crate) mod command;
 pub(crate) mod reset;
 
 pub use reset::{ResetAfterOperation, ResetBeforeOperation};
@@ -53,82 +52,6 @@ pub type Port = serialport::TTYPort;
 /// Alias for the serial COMPort.
 pub type Port = serialport::COMPort;
 
-/// The value of a command response.
-#[derive(Debug, Clone)]
-pub enum CommandResponseValue {
-    /// A 32-bit value.
-    ValueU32(u32),
-    /// A 128-bit value.
-    ValueU128(u128),
-    /// A vector of bytes.
-    Vector(Vec<u8>),
-}
-
-impl TryInto<u32> for CommandResponseValue {
-    type Error = Error;
-
-    fn try_into(self) -> Result<u32, Self::Error> {
-        match self {
-            CommandResponseValue::ValueU32(value) => Ok(value),
-            CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
-                "expected `u32` but found `u128`".into(),
-            )),
-            CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
-                "expected `u32` but found `Vec`".into(),
-            )),
-        }
-    }
-}
-
-impl TryInto<u128> for CommandResponseValue {
-    type Error = Error;
-
-    fn try_into(self) -> Result<u128, Self::Error> {
-        match self {
-            CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
-                "expected `u128` but found `u32`".into(),
-            )),
-            CommandResponseValue::ValueU128(value) => Ok(value),
-            CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
-                "expected `u128` but found `Vec`".into(),
-            )),
-        }
-    }
-}
-
-impl TryInto<Vec<u8>> for CommandResponseValue {
-    type Error = Error;
-
-    fn try_into(self) -> Result<Vec<u8>, Self::Error> {
-        match self {
-            CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
-                "expected `Vec` but found `u32`".into(),
-            )),
-            CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
-                "expected `Vec` but found `u128`".into(),
-            )),
-            CommandResponseValue::Vector(value) => Ok(value),
-        }
-    }
-}
-
-/// A response from a target device following a command.
-#[derive(Debug, Clone)]
-pub struct CommandResponse {
-    /// The response byte.
-    pub resp: u8,
-    /// The return operation byte.
-    pub return_op: u8,
-    /// The length of the return value.
-    pub return_length: u16,
-    /// The value of the response.
-    pub value: CommandResponseValue,
-    /// The error byte.
-    pub error: u8,
-    /// The status byte.
-    pub status: u8,
-}
-
 /// An established connection with a target device.
 #[derive(Debug)]
 pub struct Connection {

espflash/src/connection/reset.rs

@@ -11,13 +11,12 @@ use log::debug;
 use serialport::SerialPort;
 use strum::{Display, EnumIter, EnumString, VariantNames};
 
-use super::{
-    Connection,
-    Port,
-    USB_SERIAL_JTAG_PID,
+use super::{Connection, Port, USB_SERIAL_JTAG_PID};
+use crate::{
+    Error,
     command::{Command, CommandType},
+    flasher::FLASH_WRITE_SIZE,
 };
-use crate::{Error, flasher::FLASH_WRITE_SIZE};
 
 /// Default time to wait before releasing the boot pin after a reset
 const DEFAULT_RESET_DELAY: u64 = 50; // ms

espflash/src/error.rs

@@ -13,7 +13,7 @@ use thiserror::Error;
 #[cfg(feature = "cli")]
 use crate::cli::monitor::parser::esp_defmt::DefmtError;
 #[cfg(feature = "serialport")]
-use crate::connection::command::CommandType;
+use crate::command::CommandType;
 use crate::{
     flasher::{FlashFrequency, FlashSize},
     target::Chip,

espflash/src/flasher/mod.rs

@@ -27,11 +27,8 @@ use crate::{
 };
 #[cfg(feature = "serialport")]
 use crate::{
-    connection::{
-        Connection,
-        command::{Command, CommandType},
-        reset::ResetBeforeOperation,
-    },
+    command::{Command, CommandType},
+    connection::{Connection, reset::ResetBeforeOperation},
     error::{ConnectionError, ResultExt as _},
     flasher::stubs::{
         CHIP_DETECT_MAGIC_REG_ADDR,
@@ -1102,9 +1099,9 @@ impl Flasher {
             });
         }
 
-        let mut target = self
-            .chip
-            .flash_target(self.spi_params, self.use_stub, false, false);
+        let mut target =
+            self.chip
+                .flash_target(self.spi_params, self.use_stub, self.verify, self.skip);
 
         target.begin(&mut self.connection).flashing()?;
 
@@ -1119,15 +1116,17 @@ impl Flasher {
 
     /// Get MD5 of region
     pub fn checksum_md5(&mut self, addr: u32, length: u32) -> Result<u128, Error> {
-        self.connection
-            .with_timeout(CommandType::FlashMd5.timeout(), |connection| {
+        self.connection.with_timeout(
+            CommandType::FlashMd5.timeout_for_size(length),
+            |connection| {
                 connection
                     .command(Command::FlashMd5 {
                         offset: addr,
                         size: length,
                     })?
                     .try_into()
-            })
+            },
+        )
     }
 
     /// Get security info.
@@ -1364,7 +1363,7 @@ fn security_info(connection: &mut Connection, use_stub: bool) -> Result<Security
     connection.with_timeout(CommandType::GetSecurityInfo.timeout(), |connection| {
         let response = connection.command(Command::GetSecurityInfo)?;
         // Extract raw bytes and convert them into `SecurityInfo`
-        if let crate::connection::CommandResponseValue::Vector(data) = response {
+        if let crate::command::CommandResponseValue::Vector(data) = response {
             // HACK: Not quite sure why there seem to be 4 extra bytes at the end of the
             //       response when the stub is not being used...
             let end = if use_stub { data.len() } else { data.len() - 4 };

espflash/src/lib.rs

@@ -31,6 +31,7 @@
 
 pub use self::error::Error;
 
+pub mod command;
 #[cfg(feature = "serialport")]
 #[cfg_attr(docsrs, doc(cfg(feature = "serialport")))]
 pub mod connection;