No fs format nor file recreation and boot reason changes #656
Conversation
tyeth
force-pushed
the
no-fs-format-nor-file-recreation
branch
2 times, most recently
from
ff8498e to
4b911bf
Compare
|
Add getResetReason for rp2040/2350 (needs latest earle core) |
tyeth
force-pushed
the
no-fs-format-nor-file-recreation
branch
2 times, most recently
from
86121f1 to
3fc98a2
Compare
| bootFile.flush(); | ||
| bootFile.close(); | ||
| is_success = true; | ||
| } else { | ||
| bootFile.close(); | ||
| WS_DEBUG_PRINTLN("ERROR: Unable to open wipper_boot_out.txt for writing!"); |
There was a problem hiding this comment.
Don't think we'll see this except in debug builds with Tx monitored. The next PR (#655) will resolve this.
tyeth
changed the title
|
@brentru this is ready for review |
| if (!initFilesystem() && !initFilesystem(true)) { | ||
| setStatusLEDColor(RED); | ||
| fsHalt("ERROR Initializing Filesystem"); | ||
| if (!initFilesystem()) { |
There was a problem hiding this comment.
I am confused about tracing the brownout detection throughout the FS class.
Could you help clarify the following:
- I see
brownOutCausedResetis set withinget_and_print_reset_reason_for_cpu(). - When does this function get called within FS? I don't see any calls outwards to this function, only the boolean
brownOutCausedResetwhich isn't a function
| delay(10); // let power stablise after failure | ||
| if (!initFilesystem()) { | ||
| // no lights, save power as we're probably on a low battery | ||
| fsHalt("Brownout detected. Couldn't initialise filesystem."); |
There was a problem hiding this comment.
Could you please add some descriptive text/instructions on what to do if we couldn't recreate the FS after a brownout?
| // no lights, save power as we're probably on a low battery | ||
| fsHalt("Brownout detected. Couldn't initialise filesystem."); | ||
| } | ||
| } else if (!WS.brownOutCausedReset && !initFilesystem(true)) { |
There was a problem hiding this comment.
If a brownout was detected, all the handling for this was performed inside the if condition above and we don't need to work with/handle it anymore.
Consider changing this to an if and only handle the initFileSystem(true case.
| while (1) | ||
| { | ||
| delay(1000); | ||
| } |
There was a problem hiding this comment.
This is great, I like the idea of resetting instead of instructing the user to reset!
Shorten this by doing:
for (;;) delay(1000);
| // If WipperSnapper was previously installed - remove the | ||
| // wippersnapper_boot_out.txt file | ||
| eraseBootFile(); | ||
| // Also, should probably relabel drive to WIPPER if CIRCUITPY was there |
There was a problem hiding this comment.
Would it be possible to do this within eraseCPFS, as the vlume was likely named CIRCUITPY?
|
|
||
| // Create wippersnapper_boot_out.txt file | ||
| if (!createBootFile()) | ||
| if (!createBootFile() && !WS.brownOutCausedReset) |
There was a problem hiding this comment.
Do we need to check if a brownout happened here?
There was a problem hiding this comment.
We still check the bootlog contents in createBootfile, and print if there is a discrepency. In the event of a failure to write bootlog, assuming the secrets are still valid, we don't want to call it a failure by returning false
| bootFile.print("ESP32 Core Version: "); | ||
| bootFile.println(ESP.getCoreVersion()); | ||
| #endif | ||
| if (WS.brownOutCausedReset){ |
There was a problem hiding this comment.
Why are we checking for a brownout here, again?
There was a problem hiding this comment.
We wanted to inform that the bootlog isn't correct but we can't update it. I guess I could return true instead, or comment this as to why
| @@ -349,6 +397,8 @@ void Wippersnapper_FS::createSecretsFile() { | |||
| "Please edit it to reflect your Adafruit IO and network credentials. " | |||
| "When you're done, press RESET on the board."); | |||
| #endif | |||
| delay(500); // previously 2500 | |||
There was a problem hiding this comment.
Why are we decreasing this delay? It was from TinyUSB Arduino
| @@ -349,6 +397,8 @@ void Wippersnapper_FS::createSecretsFile() { | |||
| "Please edit it to reflect your Adafruit IO and network credentials. " | |||
| "When you're done, press RESET on the board."); | |||
| #endif | |||
| delay(500); // previously 2500 | |||
| initUSBMSC(); // re-init USB MSC to show new file to user for editing | |||
There was a problem hiding this comment.
- Where did we detach USB MSC?
- Is this called twice? We shouldn't be.
- Do we call this again, outside of this function, after this is called?
| if (error) { | ||
| if (error == DeserializationError::EmptyInput) | ||
| { | ||
| if (WS.brownOutCausedReset) |
There was a problem hiding this comment.
A special case to handle the brownout detection shouldn't be located here.
We should only handle the brownout detection/errors within the class constructor/init.
|
Recreation Steps: Secondly pull this repository (and read the readme) https://github.com/tyeth/python_nordic_ppk2_wipper_secrets_brownout |
|
Note - I will have more bandwidth to reproduce and trace this error after November, we'll re-look into it then. |
tyeth
force-pushed
the
no-fs-format-nor-file-recreation
branch
from
2f1cce7 to
3403744
Compare
This resolves a couple of things.
Firstly we now reattach USB Mass Storage Device after recreating the filesystem, this means that the drive now shows up on first boot after an initial firmware installation (tested with erased device). Previously the first boot would not show a drive, but subsequent boots would.
Secondly, in relation to brown-out situations, we now avoid erasing files unnecessarily, and if we last reset due to brownout we do not attempt to write any files like boot log nor erase the circuitpython files. There turns out to be little reason to write to the filesystem every boot. Mostly a load of
.existsguards to avoid unnecessary erases, and checking the boot reason for some operations.There's a minor tweak to the print_reset_reason method as it was being used incorrectly. It was being passed CPU core number instead of passing in the result of rtc_get_reset_reason(cpuCore).
Finally the pico now has a boot reason adding in the latest Earle core, so included that too.