Skip to content
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

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

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

jnross
Copy link
Contributor

@jnross jnross commented Aug 21, 2023

No description provided.

@jnross jnross closed this Aug 22, 2023
@jnross jnross deleted the erase-flash branch August 22, 2023 20:04
@jnross jnross restored the erase-flash branch August 24, 2023 18:37
@jnross jnross reopened this Aug 24, 2023
@jnross
Copy link
Contributor Author

jnross commented Aug 24, 2023

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

@jnross
Copy link
Contributor Author

jnross commented Aug 25, 2023

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

@jnross jnross changed the title Add erase-flash and erase-parts subcommands. For #460 Add erase-flash, erase-region, and erase-parts subcommands. For #460 Sep 14, 2023
@SergioGasquez SergioGasquez linked an issue Sep 18, 2023 that may be closed by this pull request
@SergioGasquez SergioGasquez changed the title Add erase-flash, erase-region, and erase-parts subcommands. For #460 Add erase-flash, erase-region, and erase-parts subcommands Sep 18, 2023
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Same as #461, we want to keep the CLI as it is (for the moment) to avoid having to bump SemVer major. This will be later addressed in #473

@jnross
Copy link
Contributor Author

jnross commented Sep 20, 2023

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

@jnross
Copy link
Contributor Author

jnross commented Sep 20, 2023

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.

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

I left some minor comments and also have a few suggestions:

  • Can we implement the subcommands also for cargo-espflash?
    • Note that we don't require partition_table for erase-parts in cargo-espflash as it can be obtained from the package metadada, or it will result in a similar issue to #390
  • EraseFlash and EraseRegion are only available when using stub
    • It would be great if we could check if we are using a stub, if not, throw an informative error
      • esptool does not have such check or I haven't found it
      • This might not be possible to do without breaking semver. If that's the case we can just open an issue for the future
    • At the moment, if you flash with no-stub and then do an erase-flash and you get invalid header: 0xffffffff

espflash/src/flasher/mod.rs Outdated Show resolved Hide resolved
espflash/src/bin/espflash.rs Outdated Show resolved Hide resolved
espflash/src/bin/espflash.rs Outdated Show resolved Hide resolved
espflash/src/bin/espflash.rs Outdated Show resolved Hide resolved
espflash/src/bin/espflash.rs Outdated Show resolved Hide resolved
@jnross
Copy link
Contributor Author

jnross commented Sep 22, 2023

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

@jnross
Copy link
Contributor Author

jnross commented Sep 23, 2023

@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
Copy link
Contributor Author

jnross commented Sep 24, 2023

@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.

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Just a small comment to avoid using bail! and use a defined Error that we can reuse

cargo-espflash/src/main.rs Outdated Show resolved Hide resolved
espflash/src/bin/espflash.rs Outdated Show resolved Hide resolved
espflash/src/cli/mod.rs Outdated Show resolved Hide resolved
espflash/src/cli/mod.rs Outdated Show resolved Hide resolved
@jnross
Copy link
Contributor Author

jnross commented Sep 29, 2023

@SergioGasquez: Thanks for catching that, you're quite right I should have defined a single error for these cases. Fixed in 22d792a

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fixes! One last detail!

espflash/src/error.rs Outdated Show resolved Hide resolved
Co-authored-by: Sergio Gasquez Arcos <sergio.gasquez@gmail.com>
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution and for addressing all the changes!

@SergioGasquez SergioGasquez merged commit 59a8243 into esp-rs:main Sep 29, 2023
19 checks passed
@jnross
Copy link
Contributor Author

jnross commented Sep 29, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add command to erase all flash
2 participants