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

[feature] --mass-erase for st-flash write commands #1397

Merged
merged 2 commits into from
May 29, 2024

Conversation

hannesweisbach
Copy link
Contributor

@hannesweisbach hannesweisbach commented May 14, 2024

Hello,

I have an STM32H7 where flash verification fails (probably somehow related to #1249 and/or #1294). #1238 indicates that a --mass-erase-flag for st-flash write would be welcome.

Open points:

  • I use _Bool to store flags, instead of int32_t, which conveys better the meaning of the variable. However, I have no issue changing to int32_t or something similar to keep the codebase uniform. Changed to int32_t.
  • Should passing --mass-erase to read, erase, and reset commands issue an error? As it has no meaning nor impact on those commands, it is currently silently ignored. stlink --mass-erase erase implemented.
  • This PR is against the develop branch, as indicated by the Contributing Guidelines. Is this correct? Base branch changed to testing.

Best regards,
Hannes

(Closes #1238)

@Ant-ON
Copy link
Collaborator

Ant-ON commented May 15, 2024

@hannesweisbach

My thoughts on open points:

  • it's better to use int32_t or enum (to be more informative) instead _Bool.
  • It may not be processed. But it would be nice to add a new masserase command. Now, to do this, you need to call erase command with zero address or zero size, which is not obvious.

In general, I think it’s not worth adding mass_erase field to struct _stlink. It would be nice to add a erase type parameter to the stlink_write_flash and stlink_mwrite_flash functions.

@hannesweisbach
Copy link
Contributor Author

hannesweisbach commented May 16, 2024

I changed the the type of the mass_erase member in flash_opts to int32_t. I also introduced an enum erase_type_t in flash_common.h, which now gets passed to stlink_fwrite_flash, stlink_mwrite_flash, and stlink_write_flash.

I think it's a bit unclean in the regard that calling stlink_fwrite_flash for OTP-programming ignores this argument alltogether, but oh well, you can't win them all ;)

With the introduction of the erase_type-flag, I also removed the mass_erasefield fromstruct stlink`.

Regarding the masserase command: I found stlink erase quite intuitive.

Do you imagine a command like: stlink mass_erase (i.e. completely new command), or stlink --mass-erase erase [...] (i.e. if the flag is given, the erase command ignores additional arguments and performs a mass erase)?

@Ant-ON
Copy link
Collaborator

Ant-ON commented May 17, 2024

@hannesweisbach
Hmm, you're right. With OTP calls it really looks a little illogical. We can replace MASS_ERASE to something like NO_ERASE, then everything will look more logical.
I imagined stlink --mass-erase erase without address and size.

@Nightwalker-87 Nightwalker-87 added this to the v1.8.1 milestone May 18, 2024
@hannesweisbach
Copy link
Contributor Author

I changed the erase command such that the --mass-erase is observed. I added it as separate commit only for review reasons. I'll squash the commit before merging.

I have NO_ERASE yet to implement.

@Nightwalker-87
Copy link
Member

@hannesweisbach Thank you very much for the update. There is no need to squash the commits. An upcoming merge will add all separate changes at once and basically has the same effect seen from the perspective of the destination branch.

@hannesweisbach
Copy link
Contributor Author

Yes, I know. I'm always unhappy with the resulting commit message though. Atleast on GitLab. I haven't done merge squashes with the UI on Github.

@Nightwalker-87 Nightwalker-87 changed the title feat: --mass-erase for st-flash write commands [feature] --mass-erase for st-flash write commands May 22, 2024
Force a flash mass erase instead a flash sector erase when programming the
flash by providing a '--mass-erase' flag for the 'write' and 'erase' commands.

Signed-off-by: Hannes Weisbach <weisbach@neosat.de>
@hannesweisbach
Copy link
Contributor Author

hannesweisbach commented May 24, 2024

Sorry for the delay. I squashed the commits and cleaned the commit message up (no code changes).

@Nightwalker-87
Copy link
Member

Yes, I know. I'm always unhappy with the resulting commit message though. Atleast on GitLab. I haven't done merge squashes with the UI on Github.

@hannesweisbach: I've noticed that you force pushed in order to squash. It shall be ok here, but please take into account for future contributions that this is not considered a good practise for technical reasons: https://docs.github.com/de/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests.

@Nightwalker-87 Nightwalker-87 merged commit a7fa1ae into stlink-org:testing May 29, 2024
10 checks passed
@stlink-org stlink-org locked as resolved and limited conversation to collaborators May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants