Skip to content

Commit f31f75e

Browse files
MabezDevMabezDev
authored
Address library feedback (#901)
* use stored skip/verify params * move command structs out of connection module * changelog * use scaling timeout for md5 check
1 parent 0a983fa commit f31f75e

File tree

11 files changed

+127
-115
lines changed

11 files changed

+127
-115
lines changed

‎.github/workflows/hil.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ on:
1717

1818
env:
1919
CARGO_TERM_COLOR: always
20+
RUST_LOG: debug
2021

2122
# Cancel any currently running workflows from the same PR, branch, or
2223
# tag when a new workflow is triggered.
@@ -113,7 +114,7 @@ jobs:
113114

114115
- name: save-image/write-bin test
115116
run: |
116-
timeout 90 bash espflash/tests/scripts/save-image_write-bin.sh ${{ matrix.board.mcu }}
117+
timeout 180 bash espflash/tests/scripts/save-image_write-bin.sh ${{ matrix.board.mcu }}
117118
118119
- name: erase-region test
119120
run: timeout 30 bash espflash/tests/scripts/erase-region.sh

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
6363
- Any reference to `esp_idf` or `EspIdf` has been cut to just `idf` (#891)
6464
- Renamed `targets` module to `target` (#891)
6565
- Test data is now excluded from the crates.io release (#897)
66+
- The command module, and `Command` related structs now exist in a top level module, instead of the `connection` module (#901)
6667

6768
### Fixed
6869

@@ -76,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
7677
- The app descriptor is now correctly placed in the front of the binary (#835)
7778
- espflash now extracts the MMU page size from the app descriptor (#835)
7879
- `ResetBeforeOperation` & `ResetAfterOperation` are now public, to allow the creation of a `Connection` (#895)
80+
- `Flasher` now respects its internal `verify` and `skip` flags for all methods. (#901)
7981

8082
### Removed
8183

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

Lines changed: 90 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@ use std::{io::Write, mem::size_of, time::Duration};
55
use bytemuck::{Pod, Zeroable, bytes_of};
66
use strum::Display;
77

8-
use crate::flasher::{SpiAttachParams, SpiSetParams};
8+
use crate::{
9+
Error,
10+
flasher::{SpiAttachParams, SpiSetParams},
11+
};
912

1013
const DEFAULT_TIMEOUT: Duration = Duration::from_secs(3);
1114
const ERASE_REGION_TIMEOUT_PER_MB: Duration = Duration::from_secs(30);
@@ -14,7 +17,7 @@ const ERASE_CHIP_TIMEOUT: Duration = Duration::from_secs(120);
1417
const MEM_END_TIMEOUT: Duration = Duration::from_millis(50);
1518
const SYNC_TIMEOUT: Duration = Duration::from_millis(100);
1619
const FLASH_DEFLATE_END_TIMEOUT: Duration = Duration::from_secs(10);
17-
const FLASH_MD5_TIMEOUT: Duration = Duration::from_secs(8);
20+
const FLASH_MD5_TIMEOUT_PER_MB: Duration = Duration::from_secs(8);
1821

1922
/// Input data for SYNC command (36 bytes: 0x07 0x07 0x12 0x20, followed by
2023
/// 32 x 0x55)
@@ -63,6 +66,82 @@ pub enum CommandType {
6366
FlashDetect = 0x9F,
6467
}
6568

69+
/// The value of a command response.
70+
#[derive(Debug, Clone)]
71+
pub enum CommandResponseValue {
72+
/// A 32-bit value.
73+
ValueU32(u32),
74+
/// A 128-bit value.
75+
ValueU128(u128),
76+
/// A vector of bytes.
77+
Vector(Vec<u8>),
78+
}
79+
80+
impl TryInto<u32> for CommandResponseValue {
81+
type Error = Error;
82+
83+
fn try_into(self) -> Result<u32, Self::Error> {
84+
match self {
85+
CommandResponseValue::ValueU32(value) => Ok(value),
86+
CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
87+
"expected `u32` but found `u128`".into(),
88+
)),
89+
CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
90+
"expected `u32` but found `Vec`".into(),
91+
)),
92+
}
93+
}
94+
}
95+
96+
impl TryInto<u128> for CommandResponseValue {
97+
type Error = Error;
98+
99+
fn try_into(self) -> Result<u128, Self::Error> {
100+
match self {
101+
CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
102+
"expected `u128` but found `u32`".into(),
103+
)),
104+
CommandResponseValue::ValueU128(value) => Ok(value),
105+
CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
106+
"expected `u128` but found `Vec`".into(),
107+
)),
108+
}
109+
}
110+
}
111+
112+
impl TryInto<Vec<u8>> for CommandResponseValue {
113+
type Error = Error;
114+
115+
fn try_into(self) -> Result<Vec<u8>, Self::Error> {
116+
match self {
117+
CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
118+
"expected `Vec` but found `u32`".into(),
119+
)),
120+
CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
121+
"expected `Vec` but found `u128`".into(),
122+
)),
123+
CommandResponseValue::Vector(value) => Ok(value),
124+
}
125+
}
126+
}
127+
128+
/// A response from a target device following a command.
129+
#[derive(Debug, Clone)]
130+
pub struct CommandResponse {
131+
/// The response byte.
132+
pub resp: u8,
133+
/// The return operation byte.
134+
pub return_op: u8,
135+
/// The length of the return value.
136+
pub return_length: u16,
137+
/// The value of the response.
138+
pub value: CommandResponseValue,
139+
/// The error byte.
140+
pub error: u8,
141+
/// The status byte.
142+
pub status: u8,
143+
}
144+
66145
impl CommandType {
67146
/// Return the default timeout for the [`CommandType`] variant.
68147
pub fn timeout(&self) -> Duration {
@@ -71,7 +150,14 @@ impl CommandType {
71150
CommandType::Sync => SYNC_TIMEOUT,
72151
CommandType::EraseFlash => ERASE_CHIP_TIMEOUT,
73152
CommandType::FlashDeflEnd => FLASH_DEFLATE_END_TIMEOUT,
74-
CommandType::FlashMd5 => FLASH_MD5_TIMEOUT,
153+
CommandType::FlashMd5 => {
154+
log::warn!(
155+
"Using default timeout for {}, this may not be sufficient for large flash regions. Consider using `timeout_for_size` instead.",
156+
self
157+
);
158+
159+
DEFAULT_TIMEOUT
160+
}
75161
_ => DEFAULT_TIMEOUT,
76162
}
77163
}
@@ -93,6 +179,7 @@ impl CommandType {
93179
CommandType::FlashData | CommandType::FlashDeflData => {
94180
calc_timeout(ERASE_WRITE_TIMEOUT_PER_MB, size)
95181
}
182+
CommandType::FlashMd5 => calc_timeout(FLASH_MD5_TIMEOUT_PER_MB, size),
96183
_ => self.timeout(),
97184
}
98185
}

‎espflash/src/connection/mod.rs‎

Lines changed: 1 addition & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ use slip_codec::SlipDecoder;
1919
#[cfg(unix)]
2020
use self::reset::UnixTightReset;
2121
use self::{
22-
command::{Command, CommandType},
2322
encoder::SlipEncoder,
2423
reset::{
2524
ClassicReset,
@@ -32,12 +31,12 @@ use self::{
3231
},
3332
};
3433
use crate::{
34+
command::{Command, CommandResponse, CommandResponseValue, CommandType},
3535
error::{ConnectionError, Error, ResultExt, RomError, RomErrorKind},
3636
flasher::stubs::CHIP_DETECT_MAGIC_REG_ADDR,
3737
target::Chip,
3838
};
3939

40-
pub(crate) mod command;
4140
pub(crate) mod reset;
4241

4342
pub use reset::{ResetAfterOperation, ResetBeforeOperation};
@@ -53,82 +52,6 @@ pub type Port = serialport::TTYPort;
5352
/// Alias for the serial COMPort.
5453
pub type Port = serialport::COMPort;
5554

56-
/// The value of a command response.
57-
#[derive(Debug, Clone)]
58-
pub enum CommandResponseValue {
59-
/// A 32-bit value.
60-
ValueU32(u32),
61-
/// A 128-bit value.
62-
ValueU128(u128),
63-
/// A vector of bytes.
64-
Vector(Vec<u8>),
65-
}
66-
67-
impl TryInto<u32> for CommandResponseValue {
68-
type Error = Error;
69-
70-
fn try_into(self) -> Result<u32, Self::Error> {
71-
match self {
72-
CommandResponseValue::ValueU32(value) => Ok(value),
73-
CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
74-
"expected `u32` but found `u128`".into(),
75-
)),
76-
CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
77-
"expected `u32` but found `Vec`".into(),
78-
)),
79-
}
80-
}
81-
}
82-
83-
impl TryInto<u128> for CommandResponseValue {
84-
type Error = Error;
85-
86-
fn try_into(self) -> Result<u128, Self::Error> {
87-
match self {
88-
CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
89-
"expected `u128` but found `u32`".into(),
90-
)),
91-
CommandResponseValue::ValueU128(value) => Ok(value),
92-
CommandResponseValue::Vector(_) => Err(Error::InvalidResponse(
93-
"expected `u128` but found `Vec`".into(),
94-
)),
95-
}
96-
}
97-
}
98-
99-
impl TryInto<Vec<u8>> for CommandResponseValue {
100-
type Error = Error;
101-
102-
fn try_into(self) -> Result<Vec<u8>, Self::Error> {
103-
match self {
104-
CommandResponseValue::ValueU32(_) => Err(Error::InvalidResponse(
105-
"expected `Vec` but found `u32`".into(),
106-
)),
107-
CommandResponseValue::ValueU128(_) => Err(Error::InvalidResponse(
108-
"expected `Vec` but found `u128`".into(),
109-
)),
110-
CommandResponseValue::Vector(value) => Ok(value),
111-
}
112-
}
113-
}
114-
115-
/// A response from a target device following a command.
116-
#[derive(Debug, Clone)]
117-
pub struct CommandResponse {
118-
/// The response byte.
119-
pub resp: u8,
120-
/// The return operation byte.
121-
pub return_op: u8,
122-
/// The length of the return value.
123-
pub return_length: u16,
124-
/// The value of the response.
125-
pub value: CommandResponseValue,
126-
/// The error byte.
127-
pub error: u8,
128-
/// The status byte.
129-
pub status: u8,
130-
}
131-
13255
/// An established connection with a target device.
13356
#[derive(Debug)]
13457
pub struct Connection {

‎espflash/src/connection/reset.rs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,12 @@ use log::debug;
1111
use serialport::SerialPort;
1212
use strum::{Display, EnumIter, EnumString, VariantNames};
1313

14-
use super::{
15-
Connection,
16-
Port,
17-
USB_SERIAL_JTAG_PID,
14+
use super::{Connection, Port, USB_SERIAL_JTAG_PID};
15+
use crate::{
16+
Error,
1817
command::{Command, CommandType},
18+
flasher::FLASH_WRITE_SIZE,
1919
};
20-
use crate::{Error, flasher::FLASH_WRITE_SIZE};
2120

2221
/// Default time to wait before releasing the boot pin after a reset
2322
const DEFAULT_RESET_DELAY: u64 = 50; // ms

‎espflash/src/error.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use thiserror::Error;
1313
#[cfg(feature = "cli")]
1414
use crate::cli::monitor::parser::esp_defmt::DefmtError;
1515
#[cfg(feature = "serialport")]
16-
use crate::connection::command::CommandType;
16+
use crate::command::CommandType;
1717
use crate::{
1818
flasher::{FlashFrequency, FlashSize},
1919
target::Chip,

‎espflash/src/flasher/mod.rs‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,8 @@ use crate::{
2727
};
2828
#[cfg(feature = "serialport")]
2929
use crate::{
30-
connection::{
31-
Connection,
32-
command::{Command, CommandType},
33-
reset::ResetBeforeOperation,
34-
},
30+
command::{Command, CommandType},
31+
connection::{Connection, reset::ResetBeforeOperation},
3532
error::{ConnectionError, ResultExt as _},
3633
flasher::stubs::{
3734
CHIP_DETECT_MAGIC_REG_ADDR,
@@ -1102,9 +1099,9 @@ impl Flasher {
11021099
});
11031100
}
11041101

1105-
let mut target = self
1106-
.chip
1107-
.flash_target(self.spi_params, self.use_stub, false, false);
1102+
let mut target =
1103+
self.chip
1104+
.flash_target(self.spi_params, self.use_stub, self.verify, self.skip);
11081105

11091106
target.begin(&mut self.connection).flashing()?;
11101107

@@ -1119,15 +1116,17 @@ impl Flasher {
11191116

11201117
/// Get MD5 of region
11211118
pub fn checksum_md5(&mut self, addr: u32, length: u32) -> Result<u128, Error> {
1122-
self.connection
1123-
.with_timeout(CommandType::FlashMd5.timeout(), |connection| {
1119+
self.connection.with_timeout(
1120+
CommandType::FlashMd5.timeout_for_size(length),
1121+
|connection| {
11241122
connection
11251123
.command(Command::FlashMd5 {
11261124
offset: addr,
11271125
size: length,
11281126
})?
11291127
.try_into()
1130-
})
1128+
},
1129+
)
11311130
}
11321131

11331132
/// Get security info.
@@ -1364,7 +1363,7 @@ fn security_info(connection: &mut Connection, use_stub: bool) -> Result<Security
13641363
connection.with_timeout(CommandType::GetSecurityInfo.timeout(), |connection| {
13651364
let response = connection.command(Command::GetSecurityInfo)?;
13661365
// Extract raw bytes and convert them into `SecurityInfo`
1367-
if let crate::connection::CommandResponseValue::Vector(data) = response {
1366+
if let crate::command::CommandResponseValue::Vector(data) = response {
13681367
// HACK: Not quite sure why there seem to be 4 extra bytes at the end of the
13691368
// response when the stub is not being used...
13701369
let end = if use_stub { data.len() } else { data.len() - 4 };

‎espflash/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
pub use self::error::Error;
3333

34+
pub mod command;
3435
#[cfg(feature = "serialport")]
3536
#[cfg_attr(docsrs, doc(cfg(feature = "serialport")))]
3637
pub mod connection;

0 commit comments

Comments
 (0)