Don't silently fail if defmt data isn't available/correct (#524)

* Error if defmt option is used without a parsed Table

* Return meaningful errors if defmt data wasn't found

* Transform bails into an error enum

* Add changelog entry

Author: bugadani

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [Unreleased]
 
 ### Added
+
 - Added reset strategies (#487)
 - Read esp-println generated defmt messages (#466)
 - Add --target-app-partition argument to flash command (#461)
@@ -24,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- espflash will now exit with an error if `defmt` is selected but not usable (#524)
+
 ### Removed
 
 ## [2.1.0] - 2023-10-03

cargo-espflash/src/main.rs

@@ -350,10 +350,9 @@ fn flash(args: FlashArgs, config: &Config) -> Result<()> {
             args.flash_args.monitor_baud.unwrap_or(default_baud),
             args.flash_args.log_format,
         )
-        .into_diagnostic()?;
+    } else {
+        Ok(())
     }
-
-    Ok(())
 }
 
 fn build(

espflash/src/bin/espflash.rs

@@ -272,10 +272,9 @@ fn flash(args: FlashArgs, config: &Config) -> Result<()> {
             args.flash_args.monitor_baud.unwrap_or(default_baud),
             args.flash_args.log_format,
         )
-        .into_diagnostic()?;
+    } else {
+        Ok(())
     }
-
-    Ok(())
 }
 
 fn save_image(args: SaveImageArgs) -> Result<()> {

espflash/src/cli/mod.rs

@@ -325,9 +325,6 @@ pub fn serial_monitor(args: MonitorArgs, config: &Config) -> Result<()> {
         args.connect_args.baud.unwrap_or(default_baud),
         args.log_format,
     )
-    .into_diagnostic()?;
-
-    Ok(())
 }
 
 /// Convert the provided firmware image from ELF to binary

espflash/src/cli/monitor/mod.rs

@@ -70,18 +70,22 @@ pub fn monitor(
     pid: u16,
     baud: u32,
     log_format: LogFormat,
-) -> serialport::Result<()> {
+) -> miette::Result<()> {
     println!("Commands:");
     println!("    CTRL+R    Reset chip");
     println!("    CTRL+C    Exit");
     println!();
 
     // Explicitly set the baud rate when starting the serial monitor, to allow using
     // different rates for flashing.
-    serial.serial_port_mut().set_baud_rate(baud)?;
     serial
         .serial_port_mut()
-        .set_timeout(Duration::from_millis(5))?;
+        .set_baud_rate(baud)
+        .into_diagnostic()?;
+    serial
+        .serial_port_mut()
+        .set_timeout(Duration::from_millis(5))
+        .into_diagnostic()?;
 
     // We are in raw mode until `_raw_mode` is dropped (ie. this function returns).
     let _raw_mode = RawModeGuard::new();
@@ -90,7 +94,7 @@ pub fn monitor(
     let mut stdout = ResolvingPrinter::new(elf, stdout.lock());
 
     let mut parser: Box<dyn InputParser> = match log_format {
-        LogFormat::Defmt => Box::new(parser::esp_defmt::EspDefmt::new(elf)),
+        LogFormat::Defmt => Box::new(parser::esp_defmt::EspDefmt::new(elf)?),
         LogFormat::Serial => Box::new(parser::serial::Serial),
     };
 
@@ -100,30 +104,33 @@ pub fn monitor(
             Ok(count) => Ok(count),
             Err(e) if e.kind() == ErrorKind::TimedOut => Ok(0),
             Err(e) if e.kind() == ErrorKind::Interrupted => continue,
-            err => err,
+            err => err.into_diagnostic(),
         }?;
 
         parser.feed(&buff[0..read_count], &mut stdout);
 
         // Don't forget to flush the writer!
         stdout.flush().ok();
 
-        if poll(Duration::from_secs(0))? {
-            if let Event::Key(key) = read()? {
+        if poll(Duration::from_secs(0)).into_diagnostic()? {
+            if let Event::Key(key) = read().into_diagnostic()? {
                 if key.modifiers.contains(KeyModifiers::CONTROL) {
                     match key.code {
                         KeyCode::Char('c') => break,
                         KeyCode::Char('r') => {
-                            reset_after_flash(&mut serial, pid)?;
+                            reset_after_flash(&mut serial, pid).into_diagnostic()?;
                             continue;
                         }
                         _ => {}
                     }
                 }
 
                 if let Some(bytes) = handle_key_event(key) {
-                    serial.serial_port_mut().write_all(&bytes)?;
-                    serial.serial_port_mut().flush()?;
+                    serial
+                        .serial_port_mut()
+                        .write_all(&bytes)
+                        .into_diagnostic()?;
+                    serial.serial_port_mut().flush().into_diagnostic()?;
                 }
             }
         }

espflash/src/cli/monitor/parser/esp_defmt.rs

@@ -5,9 +5,31 @@ use crossterm::{
     QueueableCommand,
 };
 use defmt_decoder::{Frame, Table};
+use miette::{bail, Context, Diagnostic, Result};
+use thiserror::Error;
 
 use crate::cli::monitor::parser::InputParser;
 
+#[derive(Clone, Copy, Debug, Diagnostic, Error)]
+#[error("Could not set up defmt logger")]
+pub enum DefmtError {
+    #[error("No elf data available")]
+    #[diagnostic(code(espflash::monitor::defmt::no_elf))]
+    NoElf,
+
+    #[error("No defmt data was found in the elf file")]
+    #[diagnostic(code(espflash::monitor::defmt::no_defmt))]
+    NoDefmtData,
+
+    #[error("Failed to parse defmt data")]
+    #[diagnostic(code(espflash::monitor::defmt::parse_failed))]
+    TableParseFailed,
+
+    #[error("Unsupported defmt encoding: {0:?}. Only rzcobs is supported.")]
+    #[diagnostic(code(espflash::monitor::defmt::unsupported_encoding))]
+    UnsupportedEncoding(defmt_decoder::Encoding),
+}
+
 #[derive(Debug, PartialEq)]
 enum FrameKind<'a> {
     Defmt(&'a [u8]),
@@ -79,31 +101,38 @@ impl FrameDelimiter {
 
 pub struct EspDefmt {
     delimiter: FrameDelimiter,
-    table: Option<Table>,
+    table: Table,
 }
 
 impl EspDefmt {
-    fn load_table(elf: Option<&[u8]>) -> Option<Table> {
-        // Load symbols from the ELF file (if provided) and initialize the context.
-        Table::parse(elf?).ok().flatten().and_then(|table| {
-            let encoding = table.encoding();
-
-            // We only support rzcobs encoding because it is the only way to multiplex
-            // a defmt stream and an ASCII log stream over the same serial port.
-            if encoding == defmt_decoder::Encoding::Rzcobs {
-                Some(table)
-            } else {
-                log::warn!("Unsupported defmt encoding: {:?}", encoding);
-                None
-            }
-        })
+    /// Loads symbols from the ELF file (if provided) and initializes the context.
+    fn load_table(elf: Option<&[u8]>) -> Result<Table> {
+        let Some(elf) = elf else {
+            bail!(DefmtError::NoElf);
+        };
+
+        let table = match Table::parse(elf) {
+            Ok(Some(table)) => table,
+            Ok(None) => bail!(DefmtError::NoDefmtData),
+            Err(e) => return Err(DefmtError::TableParseFailed).with_context(|| e),
+        };
+
+        let encoding = table.encoding();
+
+        // We only support rzcobs encoding because it is the only way to multiplex
+        // a defmt stream and an ASCII log stream over the same serial port.
+        if encoding == defmt_decoder::Encoding::Rzcobs {
+            Ok(table)
+        } else {
+            bail!(DefmtError::UnsupportedEncoding(encoding))
+        }
     }
 
-    pub fn new(elf: Option<&[u8]>) -> Self {
-        Self {
+    pub fn new(elf: Option<&[u8]>) -> Result<Self> {
+        Self::load_table(elf).map(|table| Self {
             delimiter: FrameDelimiter::new(),
-            table: Self::load_table(elf),
-        }
+            table,
+        })
     }
 
     fn handle_raw(bytes: &[u8], out: &mut dyn Write) {
@@ -143,12 +172,7 @@ impl EspDefmt {
 
 impl InputParser for EspDefmt {
     fn feed(&mut self, bytes: &[u8], out: &mut dyn Write) {
-        let Some(table) = self.table.as_mut() else {
-            Self::handle_raw(bytes, out);
-            return;
-        };
-
-        let mut decoder = table.new_stream_decoder();
+        let mut decoder = self.table.new_stream_decoder();
 
         self.delimiter.feed(bytes, |frame| match frame {
             FrameKind::Defmt(frame) => {

espflash/src/error.rs

@@ -11,6 +11,7 @@ use strum::{FromRepr, VariantNames};
 use thiserror::Error;
 
 use crate::{
+    cli::monitor::parser::esp_defmt::DefmtError,
     command::CommandType,
     flasher::{FlashFrequency, FlashSize},
     image_format::ImageFormatKind,
@@ -159,6 +160,10 @@ pub enum Error {
     #[error(transparent)]
     #[diagnostic(transparent)]
     UnsupportedImageFormat(#[from] UnsupportedImageFormatError),
+
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    Defmt(#[from] DefmtError),
 }
 
 impl From<io::Error> for Error {