search

esp-rs / espflash

Serial flasher utility for Espressif SoCs and modules based on esptool.py
Apache License 2.0
457 stars 111 forks source link

Add `erase-flash`, `erase-region`, and `erase-parts` subcommands #462

Closed jnross closed 9 months ago

jnross commented 11 months ago

Whoops, I didn't mean to close this PR. I landed the branch in our fork, but shouldn't have deleted the branch.

jnross commented 11 months ago

I rebased this on top of #465. That should land first before this one.

jnross commented 10 months ago

Thanks for the comments here and in #461. I'll make these changes to avoid the need for the SemVer bump.

jnross commented 10 months ago

Ok, I think I've resolved the concerns here. This PR now proposes no changes to cli/mod.rs, and only additive changes and bug fixes to flasher/mod.rs.

jnross commented 10 months ago

Thanks @SergioGasquez for taking a detailed look and for these great suggestions. I'll work them into my branch.

jnross commented 10 months ago

@SergioGasquez : In this PR as originally authored, there were changes to cli/mod.rs, but they were additive only. There were new pub structs and functions added, but none of the existing interface was altered or broken. Isn't that permitted in a minor version bump in SemVer? I'd working on adding these subcommands to cargo-espflash, but without making additive changes to cli/mod.rs there's quite a bit of duplicated code required.

jnross commented 10 months ago

@SergioGasquez : I adopted all of your suggestions. Then I went about adding support for these subcommands to cargo-espflash, and found that I would have to duplicate several new structs and new functions in both espflash/src/bin/espflash.rs and cargo-espflash/src/main.rs. Since these structs and functions are purely additive this shouldn't require a major SemVer bump, only a minor one.

I'd ask that you take a close look at the erase-parts implementation in cargo-espflash. I wasn't sure if I should require a build there, or whether I should fail if a cargo package isn't found in the current directory or per the --package argument, as long as an alternative --partition-table is provided.

jnross commented 9 months ago

@SergioGasquez: Thanks for catching that, you're quite right I should have defined a single error for these cases. Fixed in https://github.com/esp-rs/espflash/pull/462/commits/22d792ada3795ae0ce284b7e530aba89856b9de3

jnross commented 9 months ago

Thanks for your stewardship and excellent suggestions here. I'm happy to see these changes get integrated.