From 12ad2aa5d65abefc2133feda8b7f4d2034394cb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Nov 2023 20:50:28 +0100 Subject: [PATCH 1/4] Error if defmt option is used without a parsed Table --- espflash/src/cli/monitor/mod.rs | 2 +- espflash/src/cli/monitor/parser/esp_defmt.rs | 17 ++++++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/espflash/src/cli/monitor/mod.rs b/espflash/src/cli/monitor/mod.rs index e2bc871d..f66fccd2 100644 --- a/espflash/src/cli/monitor/mod.rs +++ b/espflash/src/cli/monitor/mod.rs @@ -90,7 +90,7 @@ pub fn monitor( let mut stdout = ResolvingPrinter::new(elf, stdout.lock()); let mut parser: Box = match log_format { - LogFormat::Defmt => Box::new(parser::esp_defmt::EspDefmt::new(elf)), + LogFormat::Defmt => Box::new(parser::esp_defmt::EspDefmt::new(elf).unwrap()), LogFormat::Serial => Box::new(parser::serial::Serial), }; diff --git a/espflash/src/cli/monitor/parser/esp_defmt.rs b/espflash/src/cli/monitor/parser/esp_defmt.rs index acef261e..80202d8d 100644 --- a/espflash/src/cli/monitor/parser/esp_defmt.rs +++ b/espflash/src/cli/monitor/parser/esp_defmt.rs @@ -79,7 +79,7 @@ impl FrameDelimiter { pub struct EspDefmt { delimiter: FrameDelimiter, - table: Option, + table: Table, } impl EspDefmt { @@ -99,11 +99,11 @@ impl EspDefmt { }) } - pub fn new(elf: Option<&[u8]>) -> Self { - Self { + pub fn new(elf: Option<&[u8]>) -> Option { + 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 +143,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) => { From 5a0bed4924372e74683002262f1be5245c25dde3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Nov 2023 21:13:12 +0100 Subject: [PATCH 2/4] Return meaningful errors if defmt data wasn't found --- cargo-espflash/src/main.rs | 5 ++- espflash/src/bin/espflash.rs | 5 ++- espflash/src/cli/mod.rs | 3 -- espflash/src/cli/monitor/mod.rs | 27 ++++++++------ espflash/src/cli/monitor/parser/esp_defmt.rs | 37 ++++++++++++-------- 5 files changed, 43 insertions(+), 34 deletions(-) diff --git a/cargo-espflash/src/main.rs b/cargo-espflash/src/main.rs index 4eac04a2..702c6a43 100644 --- a/cargo-espflash/src/main.rs +++ b/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( diff --git a/espflash/src/bin/espflash.rs b/espflash/src/bin/espflash.rs index 3b09b245..9cb4fc64 100644 --- a/espflash/src/bin/espflash.rs +++ b/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<()> { diff --git a/espflash/src/cli/mod.rs b/espflash/src/cli/mod.rs index f11bec4c..16583a47 100644 --- a/espflash/src/cli/mod.rs +++ b/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 diff --git a/espflash/src/cli/monitor/mod.rs b/espflash/src/cli/monitor/mod.rs index f66fccd2..f946d387 100644 --- a/espflash/src/cli/monitor/mod.rs +++ b/espflash/src/cli/monitor/mod.rs @@ -70,7 +70,7 @@ 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"); @@ -78,10 +78,14 @@ pub fn monitor( // 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 = match log_format { - LogFormat::Defmt => Box::new(parser::esp_defmt::EspDefmt::new(elf).unwrap()), + LogFormat::Defmt => Box::new(parser::esp_defmt::EspDefmt::new(elf)?), LogFormat::Serial => Box::new(parser::serial::Serial), }; @@ -100,7 +104,7 @@ 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); @@ -108,13 +112,13 @@ pub fn monitor( // 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; } _ => {} @@ -122,8 +126,11 @@ pub fn monitor( } 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()?; } } } diff --git a/espflash/src/cli/monitor/parser/esp_defmt.rs b/espflash/src/cli/monitor/parser/esp_defmt.rs index 80202d8d..f7ca70a4 100644 --- a/espflash/src/cli/monitor/parser/esp_defmt.rs +++ b/espflash/src/cli/monitor/parser/esp_defmt.rs @@ -5,6 +5,7 @@ use crossterm::{ QueueableCommand, }; use defmt_decoder::{Frame, Table}; +use miette::{bail, miette}; use crate::cli::monitor::parser::InputParser; @@ -83,23 +84,29 @@ pub struct EspDefmt { } impl EspDefmt { - fn load_table(elf: Option<&[u8]>) -> Option
{ - // 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]>) -> miette::Result
{ + let Some(elf) = elf else { + bail!("The defmt log format can not be used without an .elf file"); + }; + + let table = Table::parse(elf).map_err(|e| miette!(e.to_string()))?; + let Some(table) = table else { + bail!("No defmt data was found in the .elf file"); + }; + + 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!("Unsupported defmt encoding: {:?}", encoding) + } } - pub fn new(elf: Option<&[u8]>) -> Option { + pub fn new(elf: Option<&[u8]>) -> miette::Result { Self::load_table(elf).map(|table| Self { delimiter: FrameDelimiter::new(), table, From 8eb84c9f58035fe5789c53de70774f9f884cca3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 29 Nov 2023 09:18:20 +0100 Subject: [PATCH 3/4] Transform bails into an error enum --- espflash/src/cli/monitor/parser/esp_defmt.rs | 38 +++++++++++++++----- espflash/src/error.rs | 5 +++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/espflash/src/cli/monitor/parser/esp_defmt.rs b/espflash/src/cli/monitor/parser/esp_defmt.rs index f7ca70a4..0d441d23 100644 --- a/espflash/src/cli/monitor/parser/esp_defmt.rs +++ b/espflash/src/cli/monitor/parser/esp_defmt.rs @@ -5,10 +5,31 @@ use crossterm::{ QueueableCommand, }; use defmt_decoder::{Frame, Table}; -use miette::{bail, miette}; +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]), @@ -85,14 +106,15 @@ pub struct EspDefmt { impl EspDefmt { /// Loads symbols from the ELF file (if provided) and initializes the context. - fn load_table(elf: Option<&[u8]>) -> miette::Result
{ + fn load_table(elf: Option<&[u8]>) -> Result
{ let Some(elf) = elf else { - bail!("The defmt log format can not be used without an .elf file"); + bail!(DefmtError::NoElf); }; - let table = Table::parse(elf).map_err(|e| miette!(e.to_string()))?; - let Some(table) = table else { - bail!("No defmt data was found in the .elf file"); + 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(); @@ -102,11 +124,11 @@ impl EspDefmt { if encoding == defmt_decoder::Encoding::Rzcobs { Ok(table) } else { - bail!("Unsupported defmt encoding: {:?}", encoding) + bail!(DefmtError::UnsupportedEncoding(encoding)) } } - pub fn new(elf: Option<&[u8]>) -> miette::Result { + pub fn new(elf: Option<&[u8]>) -> Result { Self::load_table(elf).map(|table| Self { delimiter: FrameDelimiter::new(), table, diff --git a/espflash/src/error.rs b/espflash/src/error.rs index 928c01d1..61bee7be 100644 --- a/espflash/src/error.rs +++ b/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 for Error { From 83400f90c6a27abe46e5af43e61fbb84296e866d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 29 Nov 2023 09:19:15 +0100 Subject: [PATCH 4/4] Add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b54a7bde..ee3b4d81 100644 --- a/CHANGELOG.md +++ b/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