-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
A new approach for erasing WiFi Settings #8828
Conversation
Add support for hardware reset function call - simulates EXT_RST via HWDT. Add reset selection to `ESP.eraseConfig()` for calling hardware reset after erasing the WiFi Settings. Update ArduinoOTA to use `ESP.eraseConfig(true)` Externalized `ArduinoOTA.eraseConfigAndReset()` Add OTA examples to illustrate using erase config changes.
libraries/ArduinoOTA/ArduinoOTA.cpp
Outdated
eraseConfigAndReset(); // returns on failure | ||
//C What is the best action to take on failure? | ||
//C 1) On failure, we could invalidate eboot_command buffer - | ||
//C aborting the flash update. | ||
//C 2) Just ignore it and restart. | ||
//C 3) Retry forever | ||
if (_error_callback) { | ||
_error_callback(OTA_ERASE_SETTINGS_ERROR); | ||
} |
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.
At failure, there are no good choices.
I picked 3 to retry forever.
If they power cycle, the download is aborted.
Update:
I think an EXT_RST would result in the flash update completing. RTC should be preserved.
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.
Why a separate condition & callback though? We can retrieve error id
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.
I don't understand your reference. I am using the existing error callback with a new error number to identify the situation.
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.
(review UI sent a draft :/)
Why even mention it? Since we want this to be done, probably unroll is a better option than leave things in an indeterminate state?
Another part - it is the only state that continues execution. We still have eboot part like you said, even after ::end()ing the instance (suppose that from user pov it is an option to recover).
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.
Arduino/libraries/ArduinoOTA/ArduinoOTA.cpp
Lines 328 to 346 in 0e1059d
delay(100); | |
if (OTA_ERASE_CFG_NO != _eraseConfig) { | |
eraseConfigAndReset(); // returns on failure | |
if (_error_callback) { | |
_error_callback(OTA_ERASE_SETTINGS_ERROR); | |
} | |
if (OTA_ERASE_CFG_ABORT_ON_ERROR == _eraseConfig) { | |
eboot_command_clear(); | |
return; | |
} | |
#ifdef OTA_DEBUG | |
else if (OTA_ERASE_CFG_IGNORE_ERROR == _eraseConfig) { | |
// Fallthrough and restart | |
} else { | |
panic(); | |
} | |
#endif | |
} | |
ESP.restart(); |
When enabling erase WiFi settings, the user does so by selecting the fail option ignore or abort. Does that satisfy your concerns?
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.
Can you expand why we want sys reset happen that way vs. __builtin_trap()
or similar? (which iirc works in this case. or, why it is not appropriate to trap vs. using this handler)
Do we want trick SDK into thinking we pressed RST?
libraries/ArduinoOTA/ArduinoOTA.cpp
Outdated
eraseConfigAndReset(); // returns on failure | ||
//C What is the best action to take on failure? | ||
//C 1) On failure, we could invalidate eboot_command buffer - | ||
//C aborting the flash update. | ||
//C 2) Just ignore it and restart. | ||
//C 3) Retry forever | ||
if (_error_callback) { | ||
_error_callback(OTA_ERASE_SETTINGS_ERROR); | ||
} |
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.
Why a separate condition & callback though? We can retrieve error id
And... why not in Updater? Don't we want this feature across all updater users? |
If I followed this right, the updater action has to be driven from the library side. Where in the updater are you thinking this can be placed?
Yes. I didn't want to give the SDK the opportunity to do the wrong thing. I am trying to create a situation where the SDK will have no reason and minimum opportunity to write to the freshly erase data area. (Which it rightfully thinks it owns.) We are possibly going to load a different SDK. I didn't want to go down the path of a graceful shutdown and reboot with the SDK in full control. I know it is very heavy-handed to force a reboot this way. I felt it was the safer path. Until I found a method to force a quick hardware reset through software, I rejected this approach. Without it, we would need to force a regular HWDT.
Let me know if you prefer the 8-second HWDT method. |
Removed continuous retry of "eraseConfig" and allow the script to assign an error handling option for "eraseConfig" failure. Update example to use error handling option.
I was thinking of something like existing setAsync or installSignature, just another Updater method to set a boolean flag Update.setEraseConfigBeforeReboot(true);` Explicit code is probably better then, lets keep the hardware_reboot as-is |
There is an odd behavior with
Update: After Power Enable Pin, EXT_RST, or HWDT events, at "main()" in eboot, WDT is disabled. Key WDT hardware registers are zero. After In eboot, ets_wdt_enable was defined as After the OTA update to flash, when the reboot was started by: Power Enable Pin, EXT_RST, or HWDT, the bad argument list - caused a fast HWDT. With the flash update complete on the reboot, WDT is disabled, and the image loads. |
I started to do something like that. I had trouble putting stuff in Correction: |
Out of scope here, but I see #7979 as a possible solution to that 3.1.2 is slightly stretched, so holding for a bit. |
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.
Pending FPM change, going to rewrite into Updater part some time later
lgtm otherwise
Avoid using "[[noreturn]]" - not accepted for a .c file function Updated to use __attribute__((noreturn)) to handle both .cpp and .c file functions.
* A new approach for erasing WiFi Settings Add support for hardware reset function call - simulates EXT_RST via HWDT. Add reset selection to `ESP.eraseConfig()` for calling hardware reset after erasing the WiFi Settings. Update ArduinoOTA to use `ESP.eraseConfig(true)` Externalized `ArduinoOTA.eraseConfigAndReset()` Add OTA examples to illustrate using erase config changes. * style fixed confused example * improve wording * Add new state to retry eraseConfigAndReset * Removed unreachable error test from examples. Removed continuous retry of "eraseConfig" and allow the script to assign an error handling option for "eraseConfig" failure. Update example to use error handling option. * In eboot for function ets_wdt_enable() added missing arguments * Update comments and example * Wording * Rebuilt eboot.elf with current tools from ./get.py * Requested changes. * cleanup comments * Update hardware_reset Avoid using "[[noreturn]]" - not accepted for a .c file function Updated to use __attribute__((noreturn)) to handle both .cpp and .c file functions.
Add hardware reset function - simulates
EXT_RST
via accelerated HWDT.Add reset selection to
ESP.eraseConfig()
for calling hardware reset after erasing the WiFi Settings.Update ArduinoOTA to use
ESP.eraseConfig(true)
Externalized a function to Stop WiFi, Erase WiFi Settings, and Reset,
ArduinoOTA.eraseConfigAndReset()
.Add OTA examples to illustrate using changes.