-
Notifications
You must be signed in to change notification settings - Fork 8k
feat(bootloader): Request factory reset (IDFGH-15620) #16247
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
base: master
Are you sure you want to change the base?
feat(bootloader): Request factory reset (IDFGH-15620) #16247
Conversation
👋 Hello nebkat, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
9813f02 to
53a7538
Compare
sudeep-mohanty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nebkat,
Thank you for the contribution!
This is a neat feature addition. The overall design LGTM. Left a few minor comments to work upon. Thanks.
6b5f7f7 to
f442b24
Compare
sudeep-mohanty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebkat Thank you for the re-work and the nicely formed PR. I've suggested only few nitpicks/documentation details to fix and the rest LGTM. In case you'd like to make the updates then it's perfectly fine, and I'll wait for it. If not, I can pull in the changes to our internal repository, make the changes and test them out. Thanks.
| This option enables performing a factory reset by a request from the app, which may include: | ||
| - clear one or more data partitions; | ||
| - reset to boot from "factory" partition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant information. We present it at the "Factory reset" menu level so it can be removed from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above config BOOTLOADER_FACTORY_RESET_ENABLE is a hidden item. I can make it be an explicit option, but it is useless without one of the two triggers being enabled, so this arrangement made more sense to me.
| This option enables performing a factory reset by long holding a GPIO input, which may include: | ||
| - clear one or more data partitions; | ||
| - reset to boot from "factory" partition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Can be removed now.
f442b24 to
00201e3
Compare
|
Hi @nebkat, Also, alternatively or in addition to the bootloader hooks, you could in fact manipulate the |
|
@sudeep-mohanty
The GPIO trick would mean exposing a GPIO as a factory reset pin, which may not be available or desirable. The bootloader hooks would be a maintenance overhead for developers, though I'm aware this is just shifting that responsibility to the framework. |
00201e3 to
65f868e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| esp_restart(); | ||
| break; | ||
| case 4: | ||
| reset_output_pin(CONFIG_BOOTLOADER_NUM_PIN_FACTORY_RESET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test uses undefined config when GPIO reset disabled
The new test_flow5 function is guarded by #ifdef CONFIG_BOOTLOADER_FACTORY_RESET_REQUEST_FROM_APP, but line 231 calls reset_output_pin(CONFIG_BOOTLOADER_NUM_PIN_FACTORY_RESET). This config macro only exists when CONFIG_BOOTLOADER_FACTORY_RESET_GPIO is enabled, which is an independent option. If someone enables only app-request factory reset without GPIO reset, this would fail to compile. Additionally, this call is semantically incorrect since test_flow5 tests app-requested reset via RTC memory, not GPIO - there's no pin to reset. This appears to be a copy-paste error from test_flow4.
| factory reset. | ||
| config BOOTLOADER_FACTORY_RESET_GPIO | ||
| bool "GPIO triggers factory reset" | ||
| default y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: GPIO factory reset default changed from disabled to enabled
The original CONFIG_BOOTLOADER_FACTORY_RESET had default n (disabled), but the renamed CONFIG_BOOTLOADER_FACTORY_RESET_GPIO now has default y (enabled). While existing projects preserve their settings via sdkconfig.rename, new projects will now have GPIO factory reset enabled by default. This contradicts the PR's claim of no breaking changes. New projects would unexpectedly have factory reset enabled on GPIO 4, potentially causing unintended factory resets if that pin is held at the trigger level during boot.

Description
Provides a way for the app to request that a factory reset be performed by the bootloader on next boot:
bootloader_common_get_rtc_retain_mem_factory_reset_request();This ensures consistency between factory reset by button and a programmatic reset, and is safer as there is no risk of a runtime crash due to e.g. NVS partition being loaded at runtime detecting corruption during the process.
Also raises log level when performing factory reset to
WARNto make it easier to see.Testing
New test added to verify functionality.
Tested in own project and working as expected.
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Add app-requested factory reset handled by bootloader, restructure factory reset Kconfig, update tests/docs, and raise reset logs to WARN.
bootloader_common_get_and_clear_rtc_retain_mem_factory_reset_request).CONFIG_BOOTLOADER_FACTORY_RESET_ENABLE.rtc_retain_mem_t.flagswithfactory_reset_request.bootloader_common_set_rtc_retain_mem_factory_reset_request,bootloader_common_get_and_clear_rtc_retain_mem_factory_reset_request.CONFIG_BOOTLOADER_FACTORY_RESET_ENABLE.CONFIG_BOOTLOADER_FACTORY_RESET_REQUEST_FROM_APP; rename/scope GPIO option toCONFIG_BOOTLOADER_FACTORY_RESET_GPIOand related deps.CONFIG_BOOTLOADER_HOLD_TIME_GPIOdependency; add sdkconfig rename mapping from deprecatedCONFIG_BOOTLOADER_FACTORY_RESET.components/app_update/test_apps/test_app_update):test_flow5).CONFIG_BOOTLOADER_FACTORY_RESET_GPIO; renumber flows (test_flow6→test_flow7) and update test names.sdkconfig.defaultsto new option names.Written by Cursor Bugbot for commit 65f868e. This will update automatically on new commits. Configure here.