Ravedude Improvements

ravedude/Cargo.toml

@@ -24,3 +24,10 @@ clap = { version = "4.0.0", features = ["derive", "env"] }
 
 [target.'cfg(windows)'.dependencies]
 terminal-link = "0.1.0"
+
+[profile.dev]
+panic = "abort"
+
+[profile.release]
+overflow-checks = true
+panic = "abort"

ravedude/src/avrdude/mod.rs

@@ -132,13 +132,13 @@ impl Avrdude {
                 .prefix(".avrdude-")
                 .suffix(".conf")
                 .tempfile()
-                .unwrap();
+                .context("failed creating name for avrdude config")?;
             let mut f = std::fs::File::create(&config).context("could not create avrdude.conf")?;
             f.write_all(include_bytes!("avrdude-6.conf"))
+                .and_then(|_| f.flush())
                 .context("could not write avrdude.conf for avrdude <=7.0")?;
-            f.flush().unwrap();
 
-            command = command.arg("-C").arg(&config.as_ref());
+            command = command.arg("-C").arg(config.as_ref());
             Some(config)
         } else {
             // For avrdude versions >=7.1, we don't supply a custom configuration file for now.

ravedude/src/board.rs

@@ -19,21 +19,25 @@ fn get_all_boards() -> anyhow::Result<HashMap<String, config::BoardConfig>> {
 }
 
 pub fn get_board_from_name(board_name: &str) -> anyhow::Result<config::RavedudeConfig> {
+    Ok(config::RavedudeConfig {
+        board_config: Some(get_board_config_from_name(board_name)?),
+        ..Default::default()
+    })
+}
+
+fn get_board_config_from_name(board_name: &str) -> anyhow::Result<config::BoardConfig> {
     let mut all_boards = get_all_boards()?;
 
-    Ok(config::RavedudeConfig {
-        board_config: Some(all_boards.remove(board_name).ok_or_else(|| {
-            let mut msg = format!("invalid board: {board_name}\n");
+    all_boards.remove(board_name).ok_or_else(|| {
+        let mut msg = format!("invalid board: {board_name}\n");
 
-            msg.push_str("valid boards:");
+        msg.push_str("valid boards:");
 
-            for board in all_boards.keys() {
-                msg.push('\n');
-                msg.push_str(&board);
-            }
-            anyhow::anyhow!(msg)
-        })?),
-        ..Default::default()
+        for board in all_boards.keys() {
+            msg.push('\n');
+            msg.push_str(board);
+        }
+        anyhow::anyhow!(msg)
     })
 }
 
@@ -53,11 +57,11 @@ pub fn get_board_from_manifest(manifest_path: &Path) -> anyhow::Result<config::R
                 )
             }
             if let Some(inherit) = board_config.inherit.as_deref() {
-                let base_board = get_board_from_name(inherit)?.board_config.unwrap();
+                let base_board = get_board_config_from_name(inherit)?;
                 board.board_config = Some(board.board_config.take().unwrap().merge(base_board));
             }
         } else if let Some(board_name) = board.general_options.board.as_deref() {
-            let base_board = get_board_from_name(board_name)?.board_config.unwrap();
+            let base_board = get_board_config_from_name(board_name)?;
             board.board_config = Some(base_board);
         }
         board

ravedude/src/config.rs

@@ -16,7 +16,11 @@ fn serialize_baudrate<S>(val: &Option<Option<NonZeroU32>>, serializer: S) -> Res
 where
     S: serde::Serializer,
 {
-    let baudrate = val.as_ref().map(|val| val.map_or(-1, |x| x.get() as i32));
+    let baudrate = val
+        .as_ref()
+        .map(|val| val.map_or_else(|| Ok(-1), |x| i32::try_from(x.get())))
+        .transpose()
+        .map_err(|e| serde::ser::Error::custom(format!("failed serializing baudrate: {e}")))?;
 
     baudrate.serialize(serializer)
 }
@@ -28,9 +32,15 @@ where
     Ok(match Option::<i32>::deserialize(deserializer)? {
         None => None,
         Some(-1) => Some(None),
-        Some(baudrate) => Some(Some(NonZeroU32::new(baudrate as _).ok_or_else(|| {
-            serde::de::Error::custom(format!("invalid baudrate: {baudrate}"))
-        })?)),
+        Some(baudrate) => Some(Some(
+            u32::try_from(baudrate)
+                .map_err(|_e| serde::de::Error::custom(format!("baudrate too high: {baudrate}")))
+                .and_then(|b| {
+                    NonZeroU32::new(b).ok_or_else(|| {
+                        serde::de::Error::custom(format!("baudrate must not be zero: {baudrate}"))
+                    })
+                })?,
+        )),
     })
 }
 
@@ -200,26 +210,29 @@ pub struct BoardPortID {
     pub pid: u16,
 }
 
+fn find_port(ports: &[BoardPortID]) -> anyhow::Result<std::path::PathBuf> {
+    for serialport::SerialPortInfo {
+        port_name,
+        port_type,
+    } in
+        serialport::available_ports().context("failed fetching list of available serial ports")?
+    {
+        if let serialport::SerialPortType::UsbPort(usb_info) = port_type {
+            for &BoardPortID { vid, pid } in ports {
+                if usb_info.vid == vid && usb_info.pid == pid {
+                    return Ok(port_name.into());
+                }
+            }
+        }
+    }
+    Err(anyhow::anyhow!("Serial port not found."))
+}
+
 impl BoardConfig {
     pub fn guess_port(&self) -> Option<anyhow::Result<std::path::PathBuf>> {
         match &self.usb_info {
             Some(BoardUSBInfo::Error(err)) => Some(Err(anyhow::anyhow!(err.clone()))),
-            Some(BoardUSBInfo::PortIds(ports)) => {
-                for serialport::SerialPortInfo {
-                    port_name,
-                    port_type,
-                } in serialport::available_ports().unwrap()
-                {
-                    if let serialport::SerialPortType::UsbPort(usb_info) = port_type {
-                        for &BoardPortID { vid, pid } in ports {
-                            if usb_info.vid == vid && usb_info.pid == pid {
-                                return Some(Ok(port_name.into()));
-                            }
-                        }
-                    }
-                }
-                Some(Err(anyhow::anyhow!("Serial port not found.")))
-            }
+            Some(BoardUSBInfo::PortIds(ports)) => Some(find_port(ports)),
             None => None,
         }
     }

ravedude/src/console.rs

@@ -8,7 +8,7 @@ use crate::config::OutputMode;
 use crate::config::OutputMode::*;
 
 pub fn open(
-    port: &std::path::PathBuf,
+    port: &std::path::Path,
     baudrate: u32,
     output_mode: OutputMode,
     newline_mode: NewlineMode,
@@ -34,70 +34,99 @@ pub fn open(
     let mut byte_count = 0;
 
     // Spawn a thread for the receiving end because stdio is not portably non-blocking...
-    std::thread::spawn(move || loop {
-        #[cfg(not(target_os = "windows"))]
-        let mut buf = [0u8; 4098];
+    let receiver = std::thread::spawn(move || -> anyhow::Result<()> {
+        loop {
+            #[cfg(not(target_os = "windows"))]
+            let mut buf = [0u8; 4098];
 
-        // Use buffer size 1 for windows because it blocks on rx.read until the buffer is full
-        #[cfg(target_os = "windows")]
-        let mut buf = [0u8; 1];
+            // Use buffer size 1 for windows because it blocks on rx.read until the buffer is full
+            #[cfg(target_os = "windows")]
+            let mut buf = [0u8; 1];
 
-        match rx.read(&mut buf) {
-            Ok(count) => {
-                #[cfg(target_os = "windows")]
-                {
-                    // On windows, we must ensure that we are not sending anything outside of the
-                    // ASCII range.
-                    for byte in &mut buf[..count] {
-                        if *byte & 0x80 != 0 {
-                            *byte = '?'.try_into().unwrap();
+            match rx.read(&mut buf) {
+                Ok(count) => {
+                    #[cfg(target_os = "windows")]
+                    {
+                        // On windows, we must ensure that we are not sending anything outside of the
+                        // ASCII range.
+                        for byte in &mut buf[..count] {
+                            if *byte & 0x80 != 0 {
+                                *byte = '?'.try_into().unwrap();
+                            }
                         }
                     }
-                }
-                if output_mode == Ascii {
-                    stdout.write(&buf[..count]).unwrap();
-                } else {
-                    for byte in &buf[..count] {
-                        byte_count += 1;
-                        match output_mode {
-                            Ascii => unreachable!(),
-                            Hex => write!(stdout, "{:02x} ", byte).unwrap(),
-                            Dec => write!(stdout, "{:03} ", byte).unwrap(),
-                            Bin => write!(stdout, "{:08b} ", byte).unwrap(),
-                        }
-
-                        if let Some(space_after) = space_after {
-                            if byte_count % space_after == 0 {
-                                write!(stdout, " ").unwrap();
+                    if output_mode == Ascii {
+                        stdout.write_all(&buf[..count])?;
+                    } else {
+                        for byte in &buf[..count] {
+                            byte_count += 1;
+                            match output_mode {
+                                Ascii => unreachable!(),
+                                Hex => write!(stdout, "{:02x} ", byte)?,
+                                Dec => write!(stdout, "{:03} ", byte)?,
+                                Bin => write!(stdout, "{:08b} ", byte)?,
                             }
-                        }
-                        match newline_mode {
-                            NewlineMode::On(newline_on) => {
-                                if *byte == newline_on {
-                                    writeln!(stdout).unwrap()
+
+                            if let Some(space_after) = space_after {
+                                if byte_count % space_after == 0 {
+                                    write!(stdout, " ")?;
                                 }
                             }
-                            NewlineMode::After(newline_after) => {
-                                if byte_count % newline_after == 0 {
-                                    writeln!(stdout).unwrap();
+                            match newline_mode {
+                                NewlineMode::On(newline_on) => {
+                                    if *byte == newline_on {
+                                        writeln!(stdout)?
+                                    }
+                                }
+                                NewlineMode::After(newline_after) => {
+                                    if byte_count % newline_after == 0 {
+                                        writeln!(stdout)?;
+                                    }
                                 }
+                                NewlineMode::Off => {}
                             }
-                            NewlineMode::Off => {}
                         }
                     }
+                    stdout.flush()?;
+                }
+                Err(e) => {
+                    assert!(e.kind() == std::io::ErrorKind::TimedOut);
                 }
-                stdout.flush().unwrap();
-            }
-            Err(e) => {
-                assert!(e.kind() == std::io::ErrorKind::TimedOut);
             }
         }
     });
 
+    // Spawn a thread for the sending end because stdio is not portably non-blocking...
+    let sender = std::thread::spawn(move || -> anyhow::Result<()> {
+        loop {
+            let mut buf = [0u8; 4098];
+            let count = stdin.read(&mut buf)?;
+            tx.write_all(&buf[..count])?;
+            tx.flush()?;
+        }
+    });
+
     loop {
-        let mut buf = [0u8; 4098];
-        let count = stdin.read(&mut buf)?;
-        tx.write(&buf[..count])?;
-        tx.flush()?;
+        if sender.is_finished() {
+            sender
+                .join()
+                .unwrap()
+                .context("error while sending data to target")?;
+            break;
+        }
+
+        if receiver.is_finished() {
+            receiver
+                .join()
+                .unwrap()
+                .context("error while receiving data from target")?;
+            break;
+        }
+
+        // We don't have a portable select so instead we poll our two threads every 100ms.  Not
+        // pretty but at least this should work absolutely everywhere...
+        std::thread::sleep(std::time::Duration::from_millis(100));
     }
+
+    Ok(())
 }

ravedude/src/main.rs

@@ -91,6 +91,9 @@
 //! ```
 //!
 //! For reference, take a look at [`boards.toml`](https://github.com/Rahix/avr-hal/blob/main/ravedude/src/boards.toml).
+
+#![deny(clippy::as_conversions)]
+
 use anyhow::Context as _;
 use colored::Colorize as _;
 use config::OutputMode;
@@ -192,10 +195,7 @@ impl Args {
     ///
     /// Returns `None` if no binary argument was passed.
     fn bin_or_legacy_bin(&self) -> Option<&std::path::Path> {
-        self.bin_legacy
-            .as_ref()
-            .map(|p| p.as_path())
-            .or(self.bin.as_ref().map(|p| p.as_path()))
+        self.bin_legacy.as_deref().or(self.bin.as_deref())
     }
 }
 
@@ -351,7 +351,7 @@ fn ravedude() -> anyhow::Result<()> {
 
         let console_port = ravedude_config.general_options.console_port.clone();
         let port = console_port
-            .or_else(|| port)
+            .or(port)
             .context("no programmer serial port or `console-port` was set")
             .context("cannot open console without a serial port")?;
         let newline_mode = ravedude_config.general_options.newline_mode()?;

ravedude/src/ui.rs

@@ -26,7 +26,7 @@ pub fn print_error(e: anyhow::Error) {
         e.to_string().bold()
     );
 
-    eprintln!("");
+    eprintln!();
 
     for cause in e.chain().skip(1) {
         eprintln!(