Preset slot system confuses integers and characters, breaking slots 'H' and higher

[nyanpasu64]

This bug report includes multiple symptoms caused by the same central type confusion over uopt->presetSlot. All issues were confirmed to occur on master c4babdd49da0a7d299b3e246d89fb9bb9a5efaaa.

After factory reset, all settings are grayed out

Upon first boot, or after a factory reset or SPIFFS format, all toggle switches on the Settings page are turned off (gray). Clicking on them still works (affects VGA output and appears in the developer console), but does not change the web UI switches. This bug persists until you click a slot on the Presets panel.

Looking in Network debugger, I see malformed status messages consisting of #1 with characters 2-5 missing. Why does this happen?

void loadDefaultUserOptions()
{
    uopt->presetPreference = Output960P;    // #1
    uopt->enableFrameTimeLock = 0; // permanently adjust frame timing to avoid glitch vertical bar. does not work on all displays!
    uopt->presetSlot = 0;          //
    ...
}

void updateWebSocketData()
{
    if (rto->webServerEnabled && rto->webServerStarted) {
        if (webSocket.connectedClients() > 0) {

            char toSend[7] = {0};
            ...
            toSend[2] = (char)uopt->presetSlot;

            // '@' = 0x40, used for "byte is present" detection; 0x80 not in ascii table
            toSend[3] = '@';
            toSend[4] = '@';
            toSend[5] = '@';
            ...

            // send ping and stats
            if (ESP.getFreeHeap() > 14000) {
                webSocket.broadcastTXT(toSend);
            } else {
                webSocket.disconnect();
            }
        }
    }
}

After a factory reset, uopt->presetSlot is initialized to 0, gets written to toSend[2], and webSocket.broadcastTXT(toSend); treats it as a null terminator.

This can be fixed easily by passing length 6 as the second argument of webSocket.broadcastTXT(toSend), to avoid truncating the message. The null byte survives being sent through WebSockets and processed by the browser. Now that you're passing length explicitly and webSocket isn't calling strlen(), you can also turn toSend into a 6-character array without a null terminator (I never was a fan of null terminated strings anyway).

Mixing up ASCII characters and 8-bit integer indexes

If you create 8 or more preset slots, picking the 8th or above slot (technically 'H' or above) does not work:

• Saving is successful, but loading will try to load from an invalid slot (named after a null byte) instead. The console shows loading from preset slot \0: no preset file for this slot and source (with an actual null byte, shown in Firefox as a box containing [0000]).
• Additionally, after rebooting the GBS-C, uopt->presetSlot is set to 0, and the "all toggle switches are grayed out" bug appears.

Why does this happen?

Types

struct userOptions
{
    ...
    uint8_t presetSlot;

Additionally, saveUserPrefs() writes f.write(uopt->presetSlot); to char 2 of "/preferencesv2.txt".

Control flow

Most of the code currently treats uopt->presetSlot as an ASCII character:

• updateWebSocketData() sends presetSlot directly to the client in an ASCII string: toSend[2] = (char)uopt->presetSlot;
• server.on("/slot/set") sets presetSlot = an ASCII character taken directly from a HTTP parameter.
• The OLED menu sets it to 'A' through 'G', in a glorious festival of copy-pasted code.
• savePresetToSPIFFS() reads "/preferencesv2.txt" to result, and sets uint8_t slot = result[2].
  • It prints slot as a character: SerialM.println(String((char)slot));
  • Then it opens file "/preset_ntsc." + String((char)slot).

Other code treats uopt->presetSlot as a number or array index:

• loadDefaultUserOptions() sets it to 0, which is neither a printable ASCII character, nor the first slot visible on the web UI.
  • It's an acceptable stand-in for "no slot", if the rest of the code is built to accommodate "no slot selected" and NUL characters (updateWebSocketData() was not).
  • I think initializing presetSlot to slot 'A' is a better option, if we decide to keep presetSlot as ASCII.
• User command 'e' prints byte 2 of "/preferencesv2.txt" as a number:
  • SerialM.print(F("preset slot = "));
  • SerialM.println((uint8_t)(f.read()));
• setup() bounds-checks uopt->presetSlot:
  • uopt->presetSlot = lowByte(f.read());
  • if (uopt->presetSlot >= SLOTS_TOTAL)\n uopt->presetSlot = 0;

Other code is internally confused, even within a single function:

• server.on("/gbs/restore-filters"):
  • sets uint8_t currentSlot = the index of uopt->presetSlot in a list of ASCII characters.
  • It then calls SerialM.println(uopt->presetSlot), which converts uopt->presetSlot (not currentSlot!) from an 8-bit integer to an ASCII string.
    • I assume treating uopt->presetSlot as an integer was incidental behavior, caused by the overload set of SerialM.println. If presetSlot was a char rather than uint8_t (unsigned char), then SerialM.println would print it as an ASCII character.
• loadPresetFromSPIFFS() reads "/preferencesv2.txt" to result.
  • uint8_t slot = 0;
  • It bounds-checks result as an integer: if (result[2] < SLOTS_TOTAL) { slot = result[2];
  • Then it prints slot as a character: SerialM.print((char)slot);
  • Then it opens file "/preset_ntsc." + String((char)slot), treating it as a character.

Some code ignores presetSlot altogether:

• server.on("/slot/save") takes a multi-character string of digits from the URL query parameter index=, and converts it to an integer uint8_t slotIndex to index into slotsObject.slot[slotIndex].
  • It also takes a name from URL parameter name=.

Solutions

• Make uopt->presetSlot uniformly ASCII, keep the current mix of ASCII characters and stringified numbers over the network.
  • Initialize uopt->presetSlot to 'A' on factory reset, so we highlight the first slot after a factory reset.
    • My concern is that the list of slots is separately coded in index.ts getSlotsHTML, and C++ server.on("/gbs/restore-filters")#slotIndexMap. And now we'll be hard-coding the initial slot value in a third location. This violates DRY, since different parts of the same codebase need to agree on the names of slots.
    • Interestingly, SlotMeta doesn't actually store slot characters. SlotMeta::slot simply replicates the integer index.
      • Worse yet, as far as I can tell SlotMeta::slot serves no purpose at all. It appears to be unused on the server side (only ever written to, never read). And it seems unused by the client in index.ts (reading /bin/slots.bin) as well, since getSlotsHTML() generates sequential gbs-slot-id client-side, and updateSlotNames() writes to GBSControl.structs by index not lookup.
  • Remove bounds checks on ASCII characters, and add to /slot/save.
  • (Less important) perhaps change server.on("/gbs/restore-filters") and user command 'e' to print uopt->presetSlot as a char.
  • + Easiest to implement.
• Make uopt->presetSlot and preferencesv2.txt uniformly numeric, send values over the network as stringified integers, use slotIndexMap on the server to convert from numeric indexes to filenames.
  • - A lot of work for little gain.
  • - Breaks the on-disk format.
  • We may be able to remove the stringified number from the /slot/save endpoint.
• Store separate ASCII and numeric preset indexes in uopt.
  • Do we make the client send over both in each request? What if the client sends non-matching values? Or just send one, and we translate to the other on the server?
  • They can go out of sync on the server, if we don't update both correctly.
  • I do not enjoy