Minor error handling and function naming improvements (#795)

* Remove unnecessary `Result` from return type, don't leak constants from `connection` module

* Improve error handling when receiving an invalid command response type

* Update `CHANGELOG.md`

Author: jessebraham

CHANGELOG.md

@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `board-info` now prints `Security information`. (#758)
 - The `command`, `elf` and `error` modules are no longer public (#772)
 - `write-bin` now works for files whose lengths are not divisible by 4 (#780, #788)
+- `get_usb_pid` is now `usb_pid` and no longer needlessly returns a `Result` (#795)
 
 ### Fixed
 

cargo-espflash/src/main.rs

@@ -354,7 +354,7 @@ fn flash(args: FlashArgs, config: &Config) -> Result<()> {
     }
 
     if args.flash_args.monitor {
-        let pid = flasher.get_usb_pid()?;
+        let pid = flasher.usb_pid();
         let mut monitor_args = args.flash_args.monitor_args;
 
         // The 26MHz ESP32-C2's need to be treated as a special case.

espflash/src/bin/espflash.rs

@@ -267,7 +267,7 @@ fn flash(args: FlashArgs, config: &Config) -> Result<()> {
     }
 
     if args.flash_args.monitor {
-        let pid = flasher.get_usb_pid()?;
+        let pid = flasher.usb_pid();
         let mut monitor_args = args.flash_args.monitor_args;
 
         // The 26MHz ESP32-C2's need to be treated as a special case.

espflash/src/cli/mod.rs

@@ -557,7 +557,7 @@ pub fn print_board_info(flasher: &mut Flasher) -> Result<()> {
 /// Open a serial monitor
 pub fn serial_monitor(args: MonitorArgs, config: &Config) -> Result<()> {
     let mut flasher = connect(&args.connect_args, config, true, true)?;
-    let pid = flasher.get_usb_pid()?;
+    let pid = flasher.usb_pid();
 
     let elf = if let Some(elf_path) = args.monitor_args.elf.clone() {
         let path = fs::canonicalize(elf_path).into_diagnostic()?;

espflash/src/connection/mod.rs

@@ -40,7 +40,7 @@ pub(crate) mod reset;
 
 const MAX_CONNECT_ATTEMPTS: usize = 7;
 const MAX_SYNC_ATTEMPTS: usize = 5;
-pub(crate) const USB_SERIAL_JTAG_PID: u16 = 0x1001;
+const USB_SERIAL_JTAG_PID: u16 = 0x1001;
 
 #[cfg(unix)]
 pub type Port = serialport::TTYPort;
@@ -60,8 +60,12 @@ impl TryInto<u32> for CommandResponseValue {
     fn try_into(self) -> Result<u32, Self::Error> {
         match self {
             CommandResponseValue::ValueU32(value) => Ok(value),
-            CommandResponseValue::ValueU128(_) => Err(Error::InternalError),
-            CommandResponseValue::Vector(_) => Err(Error::InternalError),
+            CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
+                "expected `u32` but found `u128`".into(),
+            )),
+            CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
+                "expected `u32` but found `Vec`".into(),
+            )),
         }
     }
 }
@@ -71,9 +75,13 @@ impl TryInto<u128> for CommandResponseValue {
 
     fn try_into(self) -> Result<u128, Self::Error> {
         match self {
-            CommandResponseValue::ValueU32(_) => Err(Error::InternalError),
+            CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
+                "expected `u128` but found `u32`".into(),
+            )),
             CommandResponseValue::ValueU128(value) => Ok(value),
-            CommandResponseValue::Vector(_) => Err(Error::InternalError),
+            CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
+                "expected `u128` but found `Vec`".into(),
+            )),
         }
     }
 }
@@ -83,8 +91,12 @@ impl TryInto<Vec<u8>> for CommandResponseValue {
 
     fn try_into(self) -> Result<Vec<u8>, Self::Error> {
         match self {
-            CommandResponseValue::ValueU32(_) => Err(Error::InternalError),
-            CommandResponseValue::ValueU128(_) => Err(Error::InternalError),
+            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),
         }
     }
@@ -263,7 +275,7 @@ impl Connection {
 
     // Reset the device taking into account the reset after argument
     pub fn reset_after(&mut self, is_stub: bool) -> Result<(), Error> {
-        let pid = self.get_usb_pid()?;
+        let pid = self.usb_pid();
 
         match self.after_operation {
             ResetAfterOperation::HardReset => hard_reset(&mut self.serial, pid),
@@ -461,10 +473,11 @@ impl Connection {
 
     /// Read a register command with a timeout
     pub fn read_reg(&mut self, reg: u32) -> Result<u32, Error> {
-        self.with_timeout(CommandType::ReadReg.timeout(), |connection| {
+        let resp = self.with_timeout(CommandType::ReadReg.timeout(), |connection| {
             connection.command(Command::ReadReg { address: reg })
-        })
-        .map(|v| v.try_into().unwrap())
+        })?;
+
+        resp.try_into()
     }
 
     /// Write a register command with a timeout
@@ -502,8 +515,12 @@ impl Connection {
     }
 
     /// Get the USB PID of the serial port
-    pub fn get_usb_pid(&self) -> Result<u16, Error> {
-        Ok(self.port_info.pid)
+    pub fn usb_pid(&self) -> u16 {
+        self.port_info.pid
+    }
+
+    pub(crate) fn is_using_usb_serial_jtag(&self) -> bool {
+        self.port_info.pid == USB_SERIAL_JTAG_PID
     }
 }
 

espflash/src/error.rs

@@ -205,9 +205,6 @@ pub enum Error {
     #[error(transparent)]
     TryFromSlice(#[from] TryFromSliceError),
 
-    #[error("Internal Error")]
-    InternalError,
-
     #[error("Failed to open file: {0}")]
     FileOpenError(String, #[source] io::Error),
 

espflash/src/flasher/mod.rs

@@ -1188,8 +1188,8 @@ impl Flasher {
         self.connection.into_serial()
     }
 
-    pub fn get_usb_pid(&self) -> Result<u16, Error> {
-        self.connection.get_usb_pid()
+    pub fn usb_pid(&self) -> u16 {
+        self.connection.usb_pid()
     }
 
     pub fn erase_region(&mut self, offset: u32, size: u32) -> Result<(), Error> {
@@ -1248,7 +1248,7 @@ impl Flasher {
         while data.len() < size as usize {
             let response = self.connection.read_response()?;
             let chunk: Vec<u8> = if let Some(response) = response {
-                response.value.try_into().unwrap()
+                response.value.try_into()?
             } else {
                 return Err(Error::IncorrectResponse);
             };
@@ -1268,7 +1268,7 @@ impl Flasher {
 
         let response = self.connection.read_response()?;
         let digest: Vec<u8> = if let Some(response) = response {
-            response.value.try_into().unwrap()
+            response.value.try_into()?
         } else {
             return Err(Error::IncorrectResponse);
         };

espflash/src/targets/flash_target/esp32.rs

@@ -12,7 +12,6 @@ use crate::{
     connection::{
         command::{Command, CommandType},
         Connection,
-        USB_SERIAL_JTAG_PID,
     },
     flasher::ProgressCallbacks,
     targets::FlashTarget,
@@ -76,7 +75,7 @@ impl FlashTarget for Esp32Target {
         //
         // TODO: the stub doesn't appear to disable the watchdog on ESP32-S3, so we
         //       explicitly disable the watchdog here.
-        if connection.get_usb_pid()? == USB_SERIAL_JTAG_PID {
+        if connection.is_using_usb_serial_jtag() {
             match self.chip {
                 Chip::Esp32c3 => {
                     connection.command(Command::WriteReg {