Skip to content

Conversation

@nebkat
Copy link
Contributor

@nebkat nebkat commented Jul 3, 2025

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 WARN to 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:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

Note

Add app-requested factory reset handled by bootloader, restructure factory reset Kconfig, update tests/docs, and raise reset logs to WARN.

  • Bootloader:
    • Handle factory reset requested by app via RTC flag (bootloader_common_get_and_clear_rtc_retain_mem_factory_reset_request).
    • Integrate with existing GPIO long-hold reset; consolidate under CONFIG_BOOTLOADER_FACTORY_RESET_ENABLE.
    • Elevate factory reset logs to WARN and print partitions to erase.
  • RTC retain memory / API:
    • Extend rtc_retain_mem_t.flags with factory_reset_request.
    • New APIs: bootloader_common_set_rtc_retain_mem_factory_reset_request, bootloader_common_get_and_clear_rtc_retain_mem_factory_reset_request.
  • Kconfig:
    • New "Factory reset" menu with CONFIG_BOOTLOADER_FACTORY_RESET_ENABLE.
    • Add CONFIG_BOOTLOADER_FACTORY_RESET_REQUEST_FROM_APP; rename/scope GPIO option to CONFIG_BOOTLOADER_FACTORY_RESET_GPIO and related deps.
    • Update CONFIG_BOOTLOADER_HOLD_TIME_GPIO dependency; add sdkconfig rename mapping from deprecated CONFIG_BOOTLOADER_FACTORY_RESET.
  • Tests (components/app_update/test_apps/test_app_update):
    • Add multi-stage test for app-requested factory reset (test_flow5).
    • Adjust existing tests/guards to CONFIG_BOOTLOADER_FACTORY_RESET_GPIO; renumber flows (test_flow6test_flow7) and update test names.
    • Update sdkconfig.defaults to new option names.
  • Docs:
    • Document app-requested factory reset and split GPIO long-hold section.
    • Reflect new Kconfig options and behavior.

Written by Cursor Bugbot for commit 65f868e. This will update automatically on new commits. Configure here.

@github-actions
Copy link

github-actions bot commented Jul 3, 2025

Messages
📖 🎉 Good Job! All checks are passing!

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


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 65f868e

@nebkat nebkat force-pushed the feat/bootloader-request-factory-reset branch 2 times, most recently from 9813f02 to 53a7538 Compare July 3, 2025 14:41
@github-actions github-actions bot changed the title feat(bootloader): Request factory reset feat(bootloader): Request factory reset (IDFGH-15620) Jul 3, 2025
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jul 3, 2025
Copy link
Collaborator

@sudeep-mohanty sudeep-mohanty left a 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.

@nebkat
Copy link
Contributor Author

nebkat commented Jul 7, 2025

@sudeep-mohanty

  • Renamed (sdkconfig.rename) CONFIG_BOOTLOADER_FACTORY_RESET to CONFIG_BOOTLOADER_FACTORY_RESET_GPIO to preserve existing behavior.
  • Added CONFIG_BOOTLOADER_FACTORY_RESET_REQUEST_FROM_APP.
  • Added invisible CONFIG_BOOTLOADER_FACTORY_RESET_ENABLE that is selected by the above.
  • Did not #ifdef CONFIG_BOOTLOADER_FACTORY_RESET_REQUEST_FROM_APP around bootloader_common_set_rtc_retain_mem_factory_reset_request() as (in theory) the app could be running with different bootloader versions that are configured differently:
    • Example I can think of is a security upgrade was performed that removed the ability to perform factory reset, but old devices still need to be factory reset to perform the upgrade i.e. CONFIG_BOOTLOADER_FACTORY_RESET_REQUEST_FROM_APP is not currently set, but is still used by the app.
    • Matches bootloader_common_get_rtc_retain_mem_factory_reset_state() lack of #ifdef.
  • Updated documentation to include separate subsubsections for app triggered and gpio triggered resets.
  • Added new log message for when app or GPIO triggers reset.
    • GPIO status is checked even if app request was detected, which in theory may add some unnecessary delay, but helps in testing?
    • Original message is preserved to not affect any tests.
image

Copy link
Collaborator

@sudeep-mohanty sudeep-mohanty left a 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.

Comment on lines +165 to +167
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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Comment on lines +180 to +182
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.
Copy link
Collaborator

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.

@nebkat nebkat force-pushed the feat/bootloader-request-factory-reset branch from f442b24 to 00201e3 Compare July 9, 2025 11:24
@sudeep-mohanty
Copy link
Collaborator

Hi @nebkat,
Thank you for the updates. We had further discussions around this feature request and are now of the opinion that a new method to perform a factory reset from the app may not be necessary. The same could be achieved using custom bootloader hooks. An example demonstrating how to use it to extend the bootloader functionality is here.

Also, alternatively or in addition to the bootloader hooks, you could in fact manipulate the BOOTLOADER_NUM_PIN_FACTORY_RESET directly from the application and issue a reset, triggering a factory reset. Something like this is done in the test_app here.

@nebkat
Copy link
Contributor Author

nebkat commented Jul 15, 2025

@sudeep-mohanty
My hope with this change was to have something readily available for all developers that is fully tested/maintained considering:

  • It is a critical feature that could result in bricked or unintentionally reset devices if wrongly implemented
  • Most of the factory reset logic is already there
  • RTC mem is already set up, checksummed, etc

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.

@nebkat nebkat force-pushed the feat/bootloader-request-factory-reset branch from 00201e3 to 65f868e Compare December 17, 2025 13:36
Copy link

@cursor cursor bot left a 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);
Copy link

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.

Fix in Cursor Fix in Web

factory reset.
config BOOTLOADER_FACTORY_RESET_GPIO
bool "GPIO triggers factory reset"
default y
Copy link

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.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Opened Issue is new

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants