Add (some level of) SDM support #832
Conversation
| 4 | ||
| }; | ||
|
|
||
| let status_len = |
There was a problem hiding this comment.
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.
| @@ -631,7 +631,7 @@ pub struct DeviceInfo { | |||
| /// Device features | |||
| pub features: Vec<String>, | |||
| /// MAC address | |||
| pub mac_address: String, | |||
| pub mac_address: Option<String>, | |||
There was a problem hiding this comment.
The ROM bootloader doesn't send MAC in SDM?
There was a problem hiding this comment.
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!
| @@ -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> { | |||
There was a problem hiding this comment.
Can we detect sd mode in the normal detection routine, instead of doing it twice?
There was a problem hiding this comment.
I.e, inside connection.begin(), can we return the download mode type there?
| @@ -709,11 +711,18 @@ impl Flasher { | |||
|
|
|||
| // Load flash stub if enabled | |||
| if use_stub { | |||
| info!("Using flash stub"); | |||
| flasher.load_stub()?; | |||
| if !sdm { | |||
There was a problem hiding this comment.
If you bring the !sdm if outside, you can avoid two seperate if's here and below
| _segment: Segment<'_>, | ||
| _progress: &mut Option<&mut dyn ProgressCallbacks>, | ||
| ) -> Result<(), Error> { | ||
| todo!() |
There was a problem hiding this comment.
If we can't implement this, this should return a nice error, not panic.
There was a problem hiding this comment.
If it can be implemented at some point, please also add a TODO with a relevant issue.
| @@ -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 { | |||
There was a problem hiding this comment.
We can use https://doc.rust-lang.org/std/primitive.bool.html#method.then_some to remove the need for the if statements.
There was a problem hiding this comment.
@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()?;
| @@ -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) | |||
There was a problem hiding this comment.
Would be nice to have some details here, not really sure what this means TBH.
| @@ -62,8 +62,14 @@ impl FlashTarget for Esp32Target { | |||
| spi_params: self.spi_attach_params, | |||
| } | |||
| } else { | |||
| Command::SpiAttach { | |||
| spi_params: self.spi_attach_params, | |||
| if connection.secure_download_mode { | |||
There was a problem hiding this comment.
Would be cleaner to just use else if here rather than nested if statements, IMO.
|
I've found a great bug there, will get back to it later |
|
Thank you @MabezDev and @jessebraham for the reviews though, will address them when I'm back to this task. |
* Cut off the write_bin part from original #832 * Changelog entry * Akela missed... * Dumb * Address reviews
closes #726
Initial issue contains just a little bit of lie: espflash didn't work with SDM almost at all 😅
After this is merged, at least board-info and write-bin (mentioned in the original issue) functions will be supported.
No extra flags are needed, user will be able to just execute:
espflash write-bin 0x0 --monitor <bin-name>I recommend setting a higher baudrate as otherwise flashing in SDM takes a lot of time.
Also,
--no-stubis applied automatically when tool detects that connected chip is in SDM mode.