Skip to content

Add (some level of) SDM support #832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Make the default flashing frequency target specific (#389)
- Add note about permissions on Linux (#391)
- Add a diagnostic to tell the user about the partition table format (#397)
- Add (some level of) SDM support (#832)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have some details here, not really sure what this means TBH.


### Fixed

Expand Down
8 changes: 6 additions & 2 deletions espflash/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,17 +572,21 @@ pub fn parse_chip_rev(chip_rev: &str) -> Result<u16> {
/// Print information about a chip
pub fn print_board_info(flasher: &mut Flasher) -> Result<()> {
let info = flasher.device_info()?;

print!("Chip type: {}", info.chip);

if let Some((major, minor)) = info.revision {
println!(" (revision v{major}.{minor})");
} else {
println!();
}

println!("Crystal frequency: {}", info.crystal_frequency);
println!("Flash size: {}", info.flash_size);
println!("Features: {}", info.features.join(", "));
println!("MAC address: {}", info.mac_address);

if let Some(mac) = info.mac_address {
println!("MAC address: {}", mac);
}

Ok(())
}
Expand Down
21 changes: 12 additions & 9 deletions espflash/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ pub struct Connection {
decoder: SlipDecoder,
after_operation: ResetAfterOperation,
before_operation: ResetBeforeOperation,
pub(crate) secure_download_mode: bool,
}

impl Connection {
Expand All @@ -139,6 +140,7 @@ impl Connection {
decoder: SlipDecoder::new(),
after_operation,
before_operation,
secure_download_mode: false,
}
}

Expand Down Expand Up @@ -449,11 +451,13 @@ impl Connection {
// - https://docs.espressif.com/projects/esptool/en/latest/esp32/advanced-topics/serial-protocol.html?highlight=md5#response-packet
// - https://docs.espressif.com/projects/esptool/en/latest/esp32/advanced-topics/serial-protocol.html?highlight=md5#status-bytes
// - https://docs.espressif.com/projects/esptool/en/latest/esp32/advanced-topics/serial-protocol.html?highlight=md5#verifying-uploaded-data
let status_len = if response.len() == 10 || response.len() == 26 {
2
} else {
4
};

let status_len =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could benefit from some comments, and perhaps a permalink to either the esptool impl, or the docs explaining this return data from the ROM bootloader.

if response.len() == 10 || response.len() == 26 {
2
} else {
4
};

let value = match response.len() {
10 | 12 => CommandResponseValue::ValueU32(u32::from_le_bytes(
Expand Down Expand Up @@ -483,8 +487,8 @@ impl Connection {
return_op: response[1],
return_length: u16::from_le_bytes(response[2..][..2].try_into().unwrap()),
value,
error: response[response.len() - status_len],
status: response[response.len() - status_len + 1],
error: response[response.len() - status_len + 1],
status: response[response.len() - status_len],
};

Ok(Some(header))
Expand Down Expand Up @@ -524,11 +528,10 @@ impl Connection {
pub fn command(&mut self, command: Command<'_>) -> Result<CommandResponseValue, Error> {
let ty = command.command_type();
self.write_command(command).for_command(ty)?;

for _ in 0..100 {
match self.read_response().for_command(ty)? {
Some(response) if response.return_op == ty as u8 => {
return if response.error != 0 {
return if response.status != 0 {
let _error = self.flush();
Err(Error::RomError(RomError::new(
command.command_type(),
Expand Down
52 changes: 45 additions & 7 deletions espflash/src/flasher/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ pub struct DeviceInfo {
/// Device features
pub features: Vec<String>,
/// MAC address
pub mac_address: String,
pub mac_address: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ROM bootloader doesn't send MAC in SDM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the MAC is read from efuse, but in SDM you can't read efuses, so that's the reason. I should probably add a comment to clarify this though, thanks!

}

/// Connect to and flash a target device
Expand Down Expand Up @@ -674,6 +674,8 @@ impl Flasher {
connection.begin()?;
connection.set_timeout(DEFAULT_TIMEOUT)?;

let sdm = detect_sdm(&mut connection)?;

let detected_chip = if before_operation != ResetBeforeOperation::NoResetNoSync {
// Detect which chip we are connected to.
let detected_chip = detect_chip(&mut connection, use_stub)?;
Expand Down Expand Up @@ -709,11 +711,18 @@ impl Flasher {

// Load flash stub if enabled
if use_stub {
info!("Using flash stub");
flasher.load_stub()?;
if !sdm {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you bring the !sdm if outside, you can avoid two seperate if's here and below

info!("Using flash stub");
flasher.load_stub()?;
} else {
warn!("Stub is not supported in Secure Download Mode, setting --no-stub");
flasher.use_stub = !use_stub;
}
}

flasher.spi_autodetect()?;
if !sdm {
flasher.spi_autodetect()?;
}

// Now that we have established a connection and detected the chip and flash
// size, we can set the baud rate of the connection to the configured value.
Expand Down Expand Up @@ -982,14 +991,25 @@ impl Flasher {
let chip = self.chip();
let target = chip.into_target();

let revision = Some(target.chip_revision(self.connection())?);
// chip_revision reads from efuse, which is not possible in Secure Download Mode
let revision = if !self.connection.secure_download_mode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use https://doc.rust-lang.org/std/primitive.bool.html#method.then_some to remove the need for the if statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MabezDev
Not "then_some", as it eagerly evaluates arguments inside of it, which means, that even if chip IS in Secure Download Mode, "read_reg" inside of that "chip_revision" function will be evaluated -> everything will fail.

Instead, me might want to use something like

let revision = (!self.connection.secure_download_mode)
            .then(|| target.chip_revision(self.connection()))
            .transpose()?;

Some(target.chip_revision(self.connection())?)
} else {
None
};

let crystal_frequency = target.crystal_freq(self.connection())?;
let features = target
.chip_features(self.connection())?
.iter()
.map(|s| s.to_string())
.collect::<Vec<_>>();
let mac_address = target.mac_address(self.connection())?;

let mac_address = if !self.connection.secure_download_mode {
Some(target.mac_address(self.connection())?)
} else {
None
};

let info = DeviceInfo {
chip,
Expand Down Expand Up @@ -1099,10 +1119,17 @@ impl Flasher {
let mut target = self
.chip
.flash_target(self.spi_params, self.use_stub, false, false);

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

for segment in segments {
target.write_segment(&mut self.connection, segment.borrow(), &mut progress)?;
if self.connection.secure_download_mode {
target.write_segment_sdm(&mut self.connection, segment.borrow(), &mut progress)?;
} else {
target.write_segment(&mut self.connection, segment.borrow(), &mut progress)?;
}
}

target.finish(&mut self.connection, true).flashing()?;

Ok(())
Expand Down Expand Up @@ -1382,3 +1409,14 @@ fn detect_chip(connection: &mut Connection, use_stub: bool) -> Result<Chip, Erro
}
}
}

fn detect_sdm(connection: &mut Connection) -> Result<bool, Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we detect sd mode in the normal detection routine, instead of doing it twice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e, inside connection.begin(), can we return the download mode type there?

match connection.read_reg(CHIP_DETECT_MAGIC_REG_ADDR) {
Ok(_) => return Ok(false),
Err(_) => {
log::warn!("Secure Download Mode is enabled on this chip");
connection.secure_download_mode = true;
return Ok(true);
}
};
}
79 changes: 78 additions & 1 deletion espflash/src/targets/flash_target/esp32.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl FlashTarget for Esp32Target {
spi_params: self.spi_attach_params,
}
};

connection.command(command)
})?;

Expand Down Expand Up @@ -135,6 +135,83 @@ impl FlashTarget for Esp32Target {
Ok(())
}

fn write_segment_sdm(
&mut self,
connection: &mut Connection,
segment: Segment<'_>,
progress: &mut Option<&mut dyn ProgressCallbacks>,
) -> Result<(), Error> {
let addr = segment.addr;

let target = self.chip.into_target();
let flash_write_size = target.flash_write_size(connection)?;
let block_count = segment.data.len().div_ceil(flash_write_size);

connection.with_timeout(
CommandType::FlashBegin.timeout_for_size(segment.data.len() as u32),
|connection| {
connection.command(Command::FlashBegin {
size: segment.data.len() as u32,
blocks: block_count as u32,
block_size: flash_write_size as u32,
offset: addr,
supports_encryption: false,
})?;
Ok(())
},
)?;

if let Some(cb) = progress.as_mut() {
cb.init(addr, block_count)
}

let mut remaining_data = &segment.data[..];

for i in 0.. {
if remaining_data.is_empty() {
break;
}

let block_size = std::cmp::min(flash_write_size, remaining_data.len());
let block = &remaining_data[..block_size];

let padding_needed = flash_write_size.saturating_sub(block.len());

let block_vec = if padding_needed > 0 {
let mut owned = block.to_vec();
owned.extend(std::iter::repeat(0xFF).take(padding_needed));
owned
} else {
block.to_vec()
};

connection.with_timeout(
CommandType::FlashData.timeout_for_size(remaining_data.len() as u32),
|connection| {
connection.command(Command::FlashData {
data: &block_vec,
pad_to: 0,
pad_byte: 0,
sequence: i,
})?;
Ok(())
},
)?;

remaining_data = &remaining_data[block_size..];

if let Some(cb) = progress.as_mut() {
cb.update(i as usize + 1)
}
}

if let Some(cb) = progress.as_mut() {
cb.finish()
}

Ok(())
}

fn write_segment(
&mut self,
connection: &mut Connection,
Expand Down
8 changes: 8 additions & 0 deletions espflash/src/targets/flash_target/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ pub trait FlashTarget {
progress: &mut Option<&mut dyn ProgressCallbacks>,
) -> Result<(), Error>;

/// Write a segment to the target device in SDM mode
fn write_segment_sdm(
&mut self,
connection: &mut Connection,
segment: Segment<'_>,
progress: &mut Option<&mut dyn ProgressCallbacks>,
) -> Result<(), Error>;

/// Complete the flashing operation
fn finish(&mut self, connection: &mut Connection, reboot: bool) -> Result<(), Error>;
}
9 changes: 9 additions & 0 deletions espflash/src/targets/flash_target/ram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ impl FlashTarget for RamTarget {
Ok(())
}

fn write_segment_sdm(
&mut self,
_connection: &mut Connection,
_segment: Segment<'_>,
_progress: &mut Option<&mut dyn ProgressCallbacks>,
) -> Result<(), Error> {
todo!()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can't implement this, this should return a nice error, not panic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can be implemented at some point, please also add a TODO with a relevant issue.

}

fn write_segment(
&mut self,
connection: &mut Connection,
Expand Down
Loading