Regenerate displayconfig.json if erroneous #575
Conversation
|
Closes #563 |
tyeth
force-pushed
the
funhouse-display-config-recreate
branch
from
d4017ab to
989f666
Compare
|
@brentru this one is also ready for review |
|
@tyeth Can we hold on to this PR until we merge and release what's required for NO-OTA? |
|
We definitely can, although testing the noota was when I encountered the
issue.
The partition was corrupt but valid, and displayconfig was corrupt so board
stalled (no visual output too obviously)
…On Mon, 22 Apr 2024, 16:43 Brent Rubell, ***@***.***> wrote:
@tyeth <https://github.com/tyeth> Can we hold on to this PR until we
merge and release what's required for NO-OTA?
—
Reply to this email directly, view it on GitHub
<#575 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTBZ43D5AORF5WTZFJIXZDY6UV3FAVCNFSM6AAAAABGTAH7BSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRZHE2TENJXGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@tyeth Should we look at this again? |
|
Ah yes, also when secrets exists but is corrupt (usually 0bytes). I've been seeing some weird behaviour regarding first boot after installation (second boot gives more stable filesystem), but not pinned down anything definitive. |
|
Leaving this one open until file system corruption issue is resolved hopefully this week. It could go in as is, but there will be movement in same code areas as we try to avoid unnecessary file writes at boot. |
|
Note to add this when I do #655 |
tyeth
force-pushed
the
funhouse-display-config-recreate
branch
from
db2af9c to
b7a915a
Compare
|
@tyeth Do we still want this? |
|
@tyeth Should I take another look at this, this week? |
|
Yes please |
| @@ -529,7 +529,16 @@ void Wippersnapper_FS::createDisplayConfig() { | |||
| delay(2500); // give FS some time to write the file | |||
| } | |||
|
|
|||
| void Wippersnapper_FS::parseDisplayConfig(displayConfig &dispCfg) { | |||
| bool Wippersnapper_FS::parseDisplayConfig(displayConfig &dispCfg, bool forceRecreate) { | |||
There was a problem hiding this comment.
Change forceRecreate to force_recreateand give it a default param here:bool force_recreate=false`
| @@ -59,7 +59,7 @@ class Wippersnapper_FS { | |||
| void parseSecrets(); | |||
|
|
|||
| #ifdef ARDUINO_FUNHOUSE_ESP32S2 | |||
| void parseDisplayConfig(displayConfig &displayFile); | |||
| bool parseDisplayConfig(displayConfig &displayFile, bool forceRecreate = false); | |||
There was a problem hiding this comment.
Do not set default arg in header, set in .cpp
| @@ -542,13 +551,19 @@ void Wippersnapper_FS::parseDisplayConfig(displayConfig &dispCfg) { | |||
| // Attempt to open file for JSON parsing | |||
| File32 file = wipperFatFs.open("/display_config.json", FILE_READ); | |||
| if (!file) { | |||
| if (!forceRecreate && parseDisplayConfig(dispCfg, true)) { | |||
There was a problem hiding this comment.
This call to parseDisplayConfig() within parseDisplayConfig() potentially causes recursion.
Instead, I'd like to see this segment (and the segment on L564, which has the same logic) returning false and whatever calls this should check the return and then attempt to call parseDisplayConfig(dispCfg, true).
| fsHalt("FATAL ERROR: Unable to open display_config.json for parsing"); | ||
| } | ||
|
|
||
| // Attempt to deserialize the file's json document | ||
| JsonDocument doc; | ||
| DeserializationError error = deserializeJson(doc, file); | ||
| if (error) { | ||
| if (!forceRecreate && parseDisplayConfig(dispCfg, true)) { |
There was a problem hiding this comment.
See my comment about L554, this is the same potential recursion issue
It's possible for the display config file to be corrupt, at which point the funhouse fails to continue.
It's a standard file we generate so this commit regenerates the file if erroneous