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.

Sign up for GitHub

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

Jump to bottom

No fs format nor file recreation and boot reason changes #656

Open

tyeth wants to merge 15 commits into adafruit:main from tyeth:no-fs-format-nor-file-recreation

Contributor

tyeth commented Oct 25, 2024 •

edited

Loading

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 .exists guards 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.


          Change createBootFile to only if needed

9b52d98

tyeth force-pushed the no-fs-format-nor-file-recreation branch from 7d9a872 to 2e06600 Compare

October 25, 2024 14:50

tyeth added 8 commits

October 25, 2024 18:05


          Only create "don't index" files if not there

e6bafe8


          dont erase bootlog unnecessarily, skip erase Circuitpython stuff

09c5b8b


          Fix printing of ESP reset reason

7f02707


          Store if brownout boot

eb966ff


          Minimum change if brownout (don't erase CPYFS)

1d6fb07


          Doxgen + reorder methods

2ea44c6


          Reinitialise Mass Storage Device after createSecrets

d0b9a59


          Clang format and remove ESP32-H2 ifdef

4b911bf

tyeth force-pushed the no-fs-format-nor-file-recreation branch from ff8498e to 4b911bf Compare

October 25, 2024 17:06

Contributor Author

tyeth commented Oct 30, 2024 •

edited

Loading

Add getResetReason for rp2040/2350 (needs latest earle core)
See https://github.com/earlephilhower/arduino-pico/pull/2516/files

tyeth added 3 commits

November 8, 2024 16:59


          Disallow format file system if brownout, allow otherwise

b18fcf8


          Change _freshFS to self reboot

f454535


          Add RP2040/2350 reset reason / brownout detection

293c8ba

tyeth force-pushed the no-fs-format-nor-file-recreation branch from bfb100d to 86121f1 Compare

November 8, 2024 18:39


          Reinitialise secrets if empty, otherwise leave as-is

3fc98a2

tyeth force-pushed the no-fs-format-nor-file-recreation branch from 86121f1 to 3fc98a2 Compare

November 8, 2024 18:46


          clang format

2c0b752

tyeth commented

View reviewed changes

src/provisioning/tinyusb/Wippersnapper_FS.cpp Outdated Show resolved Hide resolved

src/provisioning/tinyusb/Wippersnapper_FS.cpp Show resolved Hide resolved

src/provisioning/tinyusb/Wippersnapper_FS.cpp Outdated Show resolved Hide resolved

src/provisioning/tinyusb/Wippersnapper_FS.cpp

                   bootFile.flush();
                   bootFile.close();
                   is_success = true;
                 } else {
-                  bootFile.close();
+                  WS_DEBUG_PRINTLN("ERROR: Unable to open wipper_boot_out.txt for writing!");

Contributor Author

tyeth Oct 25, 2024

Don't think we'll see this except in debug builds with Tx monitored. The next PR (#655) will resolve this.

src/provisioning/tinyusb/Wippersnapper_FS.cpp Outdated Show resolved Hide resolved

src/provisioning/tinyusb/Wippersnapper_FS.cpp Outdated Show resolved Hide resolved

src/provisioning/tinyusb/Wippersnapper_FS.cpp Outdated Show resolved Hide resolved


          Reduce delay + don't reinit USB MSC until after writeToBootOut

2f1cce7

tyeth changed the title ~~Draft: CI-Assets - No fs format nor file recreation~~ No fs format nor file recreation and boot reason changes

tyeth marked this pull request as ready for review

November 15, 2024 15:10

tyeth requested a review from brentru

November 15, 2024 15:11

Contributor Author

tyeth commented Nov 15, 2024

@brentru this is ready for review

brentru requested changes

View reviewed changes

src/provisioning/tinyusb/Wippersnapper_FS.cpp

-                if (!initFilesystem() && !initFilesystem(true)) {
-                  setStatusLEDColor(RED);
-                  fsHalt("ERROR Initializing Filesystem");
+                if (!initFilesystem()) {

Member

brentru Nov 15, 2024

I am confused about tracing the brownout detection throughout the FS class.

Could you help clarify the following:

I see brownOutCausedReset is set within get_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 brownOutCausedReset which isn't a function

src/provisioning/tinyusb/Wippersnapper_FS.cpp

+                    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.");

Member

brentru Nov 15, 2024

Could you please add some descriptive text/instructions on what to do if we couldn't recreate the FS after a brownout?

src/provisioning/tinyusb/Wippersnapper_FS.cpp

+                      // 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)) {

Member

brentru Nov 15, 2024

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.

src/provisioning/tinyusb/Wippersnapper_FS.cpp

Comment on lines +131 to +134

+                  while (1)
+                  {
+                    delay(1000);
+                  }

Member

brentru Nov 15, 2024

This is great, I like the idea of resetting instead of instructing the user to reset!

Shorten this by doing:

for (;;) delay(1000);

src/provisioning/tinyusb/Wippersnapper_FS.cpp

-                // If WipperSnapper was previously installed - remove the
-                // wippersnapper_boot_out.txt file
-                eraseBootFile();
+                // Also, should probably relabel drive to WIPPER if CIRCUITPY was there

Member

brentru Nov 15, 2024

Would it be possible to do this within eraseCPFS, as the vlume was likely named CIRCUITPY?

src/provisioning/tinyusb/Wippersnapper_FS.cpp

                 // Create wippersnapper_boot_out.txt file
-                if (!createBootFile())
+                if (!createBootFile() && !WS.brownOutCausedReset)

Member

brentru Nov 15, 2024

Do we need to check if a brownout happened here?

Contributor Author

tyeth Nov 15, 2024

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

src/provisioning/tinyusb/Wippersnapper_FS.cpp

-                  bootFile.print("ESP32 Core Version: ");
-                  bootFile.println(ESP.getCoreVersion());
-                  #endif
+                if (WS.brownOutCausedReset){

Member

brentru Nov 15, 2024

Why are we checking for a brownout here, again?

Contributor Author

tyeth Nov 15, 2024 •

edited

Loading

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

src/provisioning/tinyusb/Wippersnapper_FS.cpp

@@ @@ -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

Member

brentru Nov 15, 2024

Why are we decreasing this delay? It was from TinyUSB Arduino

src/provisioning/tinyusb/Wippersnapper_FS.cpp

@@ @@ -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

Member

brentru Nov 15, 2024

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?

src/provisioning/tinyusb/Wippersnapper_FS.cpp

-                if (error) {
+                if (error == DeserializationError::EmptyInput)
+                {
+                  if (WS.brownOutCausedReset)

Member

brentru Nov 15, 2024

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.

Contributor Author

tyeth commented Nov 15, 2024

Recreation Steps:
Firstly if you want to keep the corruption you'll need to change the code to not call initialised filesystem the second time (with forceFormat true), also disabling the check inside the initFilesystem function on failure. The same for the file.open with bootlog+secrets. I think I totally disabled the macOS file writing + eraseCPFS.

Secondly pull this repository (and read the readme) https://github.com/tyeth/python_nordic_ppk2_wipper_secrets_brownout
The hardware setup and test process is described there, but in short it powers the device and waits for wifi related serial messages (successful boot), or corruption serial messages (test stops), it totally ignores anything else and continues (timing out that test run at X seconds).

brentru self-assigned this

Member

brentru commented Nov 15, 2024

Note - I will have more bandwidth to reproduce and trace this error after November, we'll re-look into it then.

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

Labels

None yet