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

A new approach for erasing WiFi Settings #8828

Merged
merged 21 commits into from
Aug 29, 2023

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented Jan 24, 2023

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.

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.
fixed confused example
@d-a-v d-a-v added the alpha included in alpha release label Feb 8, 2023
@mhightower83 mhightower83 marked this pull request as ready for review February 8, 2023 22:24
Comment on lines 326 to 334
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);
}
Copy link
Contributor Author

@mhightower83 mhightower83 Feb 8, 2023

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@mcspr mcspr left a 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?

Comment on lines 326 to 334
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);
}
Copy link
Collaborator

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

@mcspr
Copy link
Collaborator

mcspr commented Feb 8, 2023

And... why not in Updater? Don't we want this feature across all updater users?

@mhightower83
Copy link
Contributor Author

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?

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?

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.

__builtin_trap() compiles to a break 1, 15. The NON-OS SDK's default breakpoint handler is to loop on a waiti 2 until the HWDT kinks in after 8 seconds. I guess this is an option (or a simple while(true);) if the 8-second delay is not an issue; it would save a little code space.

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.
@mcspr
Copy link
Collaborator

mcspr commented Feb 9, 2023

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

@mhightower83
Copy link
Contributor Author

mhightower83 commented Feb 9, 2023

There is an odd behavior with eboot copy flash and load when an HWDT is involved in any way.
eboot plays with disable/enable WDT in the copy flash function. This leaves the WDT running when it was originally off. After an HWDT the WDT does not appear to be configured; however, a soft restart has it configured.
What I see is that there is an extra HWDT event after the flash copy has finished. After the extra HWDT event, the WD is off again and the flash can load without obstruction.

I need to verify more. I think the WDT is always off at the start of eboot

Update:

After Power Enable Pin, EXT_RST, or HWDT events, at "main()" in eboot, WDT is disabled. Key WDT hardware registers are zero.

After ESP.restart(), at main() in eboot, WDT is enabled.

In eboot, ets_wdt_enable was defined as void ets_wdt_enable(void). It should have three arguments. These are used to set up the WDT.

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.

@mhightower83
Copy link
Contributor Author

mhightower83 commented Feb 10, 2023

I was thinking of something like existing setAsync or installSignature, just another Updater method to set a boolean flag

Update.setEraseConfigBeforeReboot(true);`

I started to do something like that. I had trouble putting stuff in UpdaterClass that needed to call library methods.
To complete an Update.setEraseConfigBeforeReboot(true), I would need to call two library functions: WiFi.mode(WIFI_OFF) and ESP.eraseConfig(true) from the UpdaterClass.

Correction: ESP.eraseConfig(true) is in core/esp8266 directory so it should be accessible from UpdaterClass.

@mcspr
Copy link
Collaborator

mcspr commented Feb 12, 2023

Out of scope here, but I see #7979 as a possible solution to that
(it was neglected for some time, though, but overall idea of moving sleep modes into ESP should be merged at some point)

3.1.2 is slightly stretched, so holding for a bit.

@d-a-v d-a-v added the merge-conflict PR has a merge conflict that needs manual correction label Mar 30, 2023
@d-a-v d-a-v removed the merge-conflict PR has a merge conflict that needs manual correction label Mar 30, 2023
Copy link
Collaborator

@mcspr mcspr left a 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

libraries/ArduinoOTA/ArduinoOTA.h Outdated Show resolved Hide resolved
cores/esp8266/Esp.h Outdated Show resolved Hide resolved
cores/esp8266/hardware_reset.h Outdated Show resolved Hide resolved
mhightower83 and others added 2 commits July 27, 2023 18:44
Avoid using "[[noreturn]]" - not accepted for a .c file function
Updated to use __attribute__((noreturn)) to handle both .cpp and .c
file functions.
@d-a-v d-a-v merged commit a348833 into esp8266:master Aug 29, 2023
56 checks passed
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha included in alpha release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants