No fs format nor file recreation and boot reason changes #656
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,7 @@ Wippersnapper_FS::Wippersnapper_FS() { | |
| WS_DEBUG_PRINTLN(project_dependencies); | ||
| WS_DEBUG_PRINTLN("*********************"); | ||
| WS_PRINTER.flush(); | ||
| delay(50); // give host a chance to finish reading serial buffer | ||
| #endif | ||
| // Detach USB device during init. | ||
| TinyUSBDevice.detach(); | ||
|
|
@@ -105,17 +106,34 @@ Wippersnapper_FS::Wippersnapper_FS() { | |
|
|
||
| // If a filesystem does not already exist - attempt to initialize a new | ||
| // filesystem | ||
| if (!initFilesystem()) { | ||
| if (WS.brownOutCausedReset) { | ||
| // try once more for good measure | ||
| 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? |
||
| } | ||
| } 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 Consider changing this to an |
||
| setStatusLEDColor(RED); | ||
| fsHalt("ERROR Initializing Filesystem"); | ||
| } | ||
| } | ||
|
|
||
| // Initialize USB-MSD | ||
| initUSBMSC(); | ||
|
|
||
| // If we created a new filesystem, halt until user RESETs device. | ||
tyeth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (_freshFS) | ||
| { | ||
| WS_DEBUG_PRINTLN("New filesystem created! Resetting the board shortly..."); | ||
| WS_PRINTER.flush(); | ||
tyeth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| WS.enableWDT(500); | ||
| while (1) | ||
| { | ||
| delay(1000); | ||
| } | ||
|
Comment on lines
+131
to
+134
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: |
||
| } | ||
| } | ||
|
|
||
| /************************************************************/ | ||
|
|
@@ -159,32 +177,39 @@ bool Wippersnapper_FS::initFilesystem(bool force_format) { | |
| if (!wipperFatFs.begin(&flash)) | ||
| return false; | ||
|
|
||
| //TODO: Don't do this unless we need the space and createSecrets fails | ||
| // If CircuitPython was previously installed - erase CPY FS | ||
| eraseCPFS(); | ||
| // 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 |
||
| // using setVolumeLabel(), but note the FS must be unmounted first | ||
|
|
||
| // No file indexing on macOS | ||
| if (!wipperFatFs.exists("/.fseventsd/no_log")) | ||
| { | ||
| wipperFatFs.mkdir("/.fseventsd/"); | ||
|
There was a problem hiding this comment. While we're in it, let's refactor out the macOS-specific handling to a new function, |
||
| File32 writeFile = wipperFatFs.open("/.fseventsd/no_log", FILE_WRITE); | ||
| if (!writeFile) | ||
| return false; | ||
| writeFile.close(); | ||
| } | ||
|
|
||
| if (!wipperFatFs.exists("/.metadata_never_index")) | ||
| { | ||
| File32 writeFile = wipperFatFs.open("/.metadata_never_index", FILE_WRITE); | ||
| if (!writeFile) | ||
| return false; | ||
| writeFile.close(); | ||
| } | ||
| if (!wipperFatFs.exists("/.Trashes")) | ||
| { | ||
| File32 writeFile = wipperFatFs.open("/.Trashes", FILE_WRITE); | ||
| if (!writeFile) | ||
| return false; | ||
| writeFile.close(); | ||
| } | ||
|
|
||
| // Create wippersnapper_boot_out.txt file | ||
| 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 |
||
| return false; | ||
|
|
||
| // Check if secrets.json file already exists | ||
|
|
@@ -241,6 +266,9 @@ bool Wippersnapper_FS::configFileExists() { | |
| */ | ||
| /**************************************************************************/ | ||
| void Wippersnapper_FS::eraseCPFS() { | ||
| if (WS.brownOutCausedReset){ | ||
| return; // Can't serial print here, in next PR check we're not out of space | ||
| } | ||
| if (wipperFatFs.exists("/boot_out.txt")) { | ||
| wipperFatFs.remove("/boot_out.txt"); | ||
| wipperFatFs.remove("/code.py"); | ||
|
|
@@ -269,44 +297,66 @@ void Wippersnapper_FS::eraseBootFile() { | |
| bool Wippersnapper_FS::createBootFile() { | ||
| bool is_success = false; | ||
| char sMAC[18] = {0}; | ||
| String newContent; | ||
|
|
||
| // Generate new content | ||
| newContent += "Adafruit.io WipperSnapper\n"; | ||
| newContent += "Firmware Version: " + String(WS_VERSION) + "\n"; | ||
| newContent += "Board ID: " + String(BOARD_ID) + "\n"; | ||
| sprintf(sMAC, "%02X:%02X:%02X:%02X:%02X:%02X", WS._macAddr[0], | ||
| WS._macAddr[1], WS._macAddr[2], WS._macAddr[3], WS._macAddr[4], | ||
| WS._macAddr[5]); | ||
| newContent += "MAC Address: " + String(sMAC) + "\n"; | ||
|
|
||
| #if PRINT_DEPENDENCIES | ||
| newContent += ("Build dependencies:\n"); | ||
| newContent += (project_dependencies); | ||
| newContent += ("\n"); | ||
| #endif | ||
|
|
||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| newContent += "ESP-IDF Version: " + String(ESP.getSdkVersion()) + "\n"; | ||
| newContent += "ESP32 Core Version: " + String(ESP.getCoreVersion()) + "\n"; | ||
| #endif | ||
|
|
||
| // Check if the file exists and read its content | ||
| File32 bootFile = wipperFatFs.open("/wipper_boot_out.txt", FILE_READ); | ||
| if (bootFile) { | ||
| String existingContent; | ||
| while (bootFile.available()) { | ||
| existingContent += char(bootFile.read()); | ||
| } | ||
| bootFile.close(); | ||
|
|
||
| // Compare existing content with new content | ||
| if (existingContent == newContent) { | ||
| WS_DEBUG_PRINTLN("INFO: wipper_boot_out.txt already exists with the " | ||
| "same content."); | ||
| return true; // No need to overwrite | ||
| } else { | ||
| WS_DEBUG_PRINTLN("INFO: wipper_boot_out.txt exists but with different " | ||
| "content. Overwriting..."); | ||
| } | ||
| } else { | ||
| WS_DEBUG_PRINTLN("INFO: could not open wipper_boot_out.txt for reading. " | ||
| "Creating new file..."); | ||
| } | ||
|
|
||
| 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 |
||
| return false; | ||
| } | ||
|
|
||
| // Overwrite the file with new content | ||
| bootFile = wipperFatFs.open("/wipper_boot_out.txt", FILE_WRITE); | ||
| if (bootFile) { | ||
| bootFile.print(newContent); | ||
| bootFile.flush(); | ||
| bootFile.close(); | ||
| is_success = true; | ||
| } else { | ||
| 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. |
||
| } | ||
|
|
||
| return is_success; | ||
| } | ||
|
|
||
|
|
@@ -338,6 +388,7 @@ void Wippersnapper_FS::createSecretsFile() { | |
| secretsFile.flush(); | ||
| secretsFile.close(); | ||
| delay(2500); | ||
tyeth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| initUSBMSC(); // re-init USB MSC to show new file to user for editing | ||
tyeth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Signal to user that action must be taken (edit secrets.json) | ||
| writeToBootOut( | ||
|
|
@@ -367,7 +418,23 @@ void Wippersnapper_FS::parseSecrets() { | |
| // Attempt to deserialize the file's JSON document | ||
| JsonDocument doc; | ||
| DeserializationError error = deserializeJson(doc, secretsFile); | ||
| 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. |
||
| { | ||
| fsHalt("ERROR: Empty secrets.json file, can't recreate due to brownout - recharge or must be fixed manually."); | ||
| } | ||
| else | ||
| { | ||
| // TODO: Can't serial print here, in next PR check we're not out of space | ||
| WS_DEBUG_PRINTLN("ERROR: Empty secrets.json file, recreating..."); | ||
| secretsFile.close(); | ||
| wipperFatFs.remove("/secrets.json"); | ||
| createSecretsFile(); // calls fsHalt | ||
| } | ||
| } | ||
| else if (error) | ||
| { | ||
| fsHalt(String("ERROR: Unable to parse secrets.json file - " | ||
| "deserializeJson() failed with code") + | ||
| error.c_str()); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 am confused about tracing the brownout detection throughout the
FSclass.Could you help clarify the following:
brownOutCausedResetis set withinget_and_print_reset_reason_for_cpu().brownOutCausedResetwhich isn't a function