ramapcsx2 / gbs-control

GNU General Public License v3.0
773 stars 110 forks source link

Cleanup code, add improved logging for debugging #406

Closed nyanpasu64 closed 1 year ago

nyanpasu64 commented 1 year ago

Changes I've made to my local GBS-C tree to help me understand and debug the code. I'm planning to rebase #405 on this branch (dropping messy commits) and force push it.

I'm not sure if "Make web UI log new messages" is a good change. All it does is make the client JS log a fixed string to the browser console whenever a message is received from the server. This allows you to tell when the GBS-C is sending new information to the web UI (and when it's stalled doing work or the Wi-Fi connection is flaking out), but doesn't say what that information is, and I found myself rarely using this "feature". And sending too many console.log operations to a browser might result in a memory leak (I don't know if that's the case in Firefox or Chrome or not), even though they're merged together into a single message in F12 because the text is identical.

Otherwise I think my code changes are generally improvements.

To verify my OutputMode implementation was correct, I have replaced each usage of the enum cases with the underlying number, and verified that the resulting file has no altered numbers compared to the commit before I added OutputMode (only comments and types/casts changed):

Details ```diff diff --git a/gbs-control.ino b/gbs-control.ino index 28eb783..ab5e253 100644 --- a/gbs-control.ino +++ b/gbs-control.ino @@ -237,7 +237,7 @@ struct userOptions { // 0 - normal, 1 - x480/x576, 2 - customized, 3 - 1280x720, 4 - 1280x1024, 5 - 1920x1080, // 6 - downscale, 10 - bypass - uint8_t presetPreference; + OutputMode presetPreference; uint8_t presetSlot; uint8_t enableFrameTimeLock; uint8_t frameTimeLockMethod; @@ -4108,6 +4108,7 @@ void doPostPresetLoadSteps() SerialM.println("\n"); } +// TODO replace result with VideoStandardInput enum void applyPresets(uint8_t result) { if (!rto->boardHasPower) { @@ -4151,6 +4152,7 @@ void applyPresets(uint8_t result) } if (result == 0) { + // Unknown SerialM.println(F("Source format not properly recognized, using fallback preset!")); result = 3; // in case of success: override to 480p60 GBS::ADC_INPUT_SEL::write(1); // RGB @@ -4202,6 +4204,7 @@ void applyPresets(uint8_t result) } if (result == 1 || result == 3 || result == 8 || result == 9 || result == 14) { + // NTSC input if (uopt->presetPreference == 0) { writeProgramArrayNew(ntsc_240p, false); } else if (uopt->presetPreference == 1) { @@ -4228,6 +4231,7 @@ void applyPresets(uint8_t result) writeProgramArrayNew(ntsc_downscale, false); } } else if (result == 2 || result == 4) { + // PAL input if (uopt->presetPreference == 0) { if (uopt->matchPresetSource) { SerialM.println(F("matched preset override > 1280x1024")); @@ -7237,7 +7241,7 @@ void setup() saveUserPrefs(); // if this fails, there must be a spiffs problem } else { //on a fresh / spiffs not formatted yet MCU: userprefs.txt open ok //result = 207 - uopt->presetPreference = (uint8_t)(f.read() - '0'); // #1 + uopt->presetPreference = (OutputMode)(f.read() - '0'); // #1 if (uopt->presetPreference > 10) uopt->presetPreference = 0; // fresh spiffs ? @@ -8072,7 +8076,7 @@ void loop() uint8_t videoMode = getVideoMode(); if (videoMode == 0) videoMode = rto->videoStandardInput; - uint8_t backup = uopt->presetPreference; + OutputMode backup = uopt->presetPreference; uopt->presetPreference = 3; rto->videoStandardInput = 0; // force hard reset applyPresets(videoMode); ```
ramapcsx2 commented 1 year ago

Please feel free to pick whether you want your debug messages, or drop them for a more release ready status. It's always possible to have an enable flag that you can set when developing, too :)

nyanpasu64 commented 1 year ago

Do you want me to drop "Make web UI log new messages" through console.log, or all code that logs to "Toggle Console" and labeling slot IDs? I do not want to do the latter, since these messages have proven useful in probing what happened "in the wild" (eg. mapping buttons to code, understanding logs, and even finding out why debug mode got enabled when I saw serial command D in the WebSocket console as if I had clicked the button even though I hadn't... I really don't like how GBS-C switches to 1280x960 and enables debug view if you switch output resolutions with no input present).

nyanpasu64 commented 1 year ago

I've removed "Make web UI log new messages" (console.log) while keeping slot names/IDs in the web GUI, regenerated the website webui_html.h, and force-pushed.

ramapcsx2 commented 1 year ago

Awesome. Those enums were really necessary. That has always been just me being lazy / rather doing other work that cleanup :)

nyanpasu64 commented 1 year ago

TBH it's odd that the source code refers to OutputBypass = 10 as HdBypass and similar, but it's named "Pass Through" on the GUI. I think this passes through the source signal's horizontal and vertical video timings to the output?

It's not worth changing and breaking consistency with the rest of the source code, though maybe I should add a comment OutputBypass = 10, // Pass Through in a subsequent PR? Not sure.

ramapcsx2 commented 1 year ago

Bypass / Pass through, yeah, iirc they mean the same. It is a drastically different mode, where the slightly processed source syncs are passed to the output timing generator as is.