Add ESP32-S3 2.8 inch display board support#2500
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR appears to revert or replace large portions of the codebase across many modules (WiFi, BLE, RFID, IR, NRF24, display core, board interfaces, build config, etc.). The net effect is the removal of many recently-added features (e.g., the structured ibutton menu, ducky_typer key queue, IR emulate mode, evil portal gateway IP setting, BLE stack teardown helpers, YModem support, WDGoWars upload, NetCut menu, AppsMenu, etc.) and the reintroduction of older, less-safe implementations. It also adds a new smoochiee-s3-2.8inch-ili9341 board variant and switches the default PlatformIO environment.
Changes:
- Wide-scale revert of recent feature work across WiFi, BLE, RFID, IR, NRF24, audio, display, settings and serial commands modules.
- Replaces many bounded-string APIs (
snprintf,strlcpy) with unbounded equivalents (sprintf,strcpy) and removes input validation/escape handling in many keyboard prompts. - Adds a new Smoochiee S3 2.8" ILI9341 board variant and tweaks build configuration (default env, lib pinning, PlatformIO flags).
Reviewed changes
Copilot reviewed 107 out of 110 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/bjs_interpreter/interpreter.h/.cpp | Changes FS& parameter to FS by value. |
| src/core/startup_app.cpp | Drops null-check on fs pointer returned from getScriptsFolder. |
| src/core/serial_commands/storage_commands.cpp | Removes YModem support and reworks writeCallback argument handling. |
| src/core/type_convertion.cpp | Shrinks temp buffer in decimalToHexString. |
| src/core/display.cpp | Reworks status bar, removes Gif null guards, reverts playFrame logic, removes enabled option support, removes read16/32 initialization. |
| src/core/wifi/wg.cpp | Replaces structured UI rendering with raw tft calls and a fixed 7s delay. |
| src/core/wifi/webInterface.cpp | Reverts drawWebUiScreen to old layout, switches snprintf→sprintf. |
| src/core/configPins.* / src/core/config.* / src/core/settings.* | Removes PN532 bus config, evil portal gateway IP config, WDGoWars API key, TerminalLog flag. |
| src/core/sd_functions.* | Removes folderExists, WDG upload entries, escape-cancel checks. |
| src/core/menu_items/* | Adds AppsMenu, restructures MainMenu, reverts WifiMenu/FileMenu/EthernetMenu changes. |
| src/core/led_control.* | Reorders LED effect enum values (renumbering breaks persisted configs). |
| src/core/app_launcher.* | New JS app discovery/launcher. |
| src/modules/wifi/* | Reverts evil portal DNS server lifetime, responder UI, tcp_utils UI, sniffer hardening, wifi_recover output, socks4 layout. |
| src/modules/ble/* | Removes stopBLEStack and BLE↔WiFi mutual teardown logic. |
| src/modules/ir/ir_read.* | Removes IR emulate mode and protocol parsing. |
| src/modules/NRF24/nrf_spectrum.* | Rewrites spectrum analyzer with new modes and layout. |
| src/modules/NRF24/nrf_jammer.h | Replaces jam-strategy struct fields with single useFlooding flag. |
| src/modules/others/ibutton.* | Reverts structured ibutton menu and file I/O to older procedural version. |
| src/modules/others/audio.cpp | Removes M5Unified path in _tone and frequency=0 delay handling in playTone. |
| src/modules/others/u2f.cpp | Replaces virtual press helpers with direct digitalRead. |
| src/modules/badusb_ble/ducky_typer.cpp | Removes the key-queue keyboard UI. |
| src/modules/bjs_interpreter/keyboard_js.cpp | Removes ESC→empty-string handling. |
| boards/m5stack-cardputer/interface.cpp | Reverts TCA8418 input handler to non-FIFO version, drops volatile from kb_interrupt. |
| boards/m5stack-sticks3/* | Removes PSRAM/OPI config, simplifies codec setup. |
| boards/smoochiee-s3-2.8inch-ili9341/* | Adds new board variant. |
| boards/lilygo-t-lora-pager/* | Reverts to single-event input handler. |
| boards/lilygo-t-embed-cc1101/interface.cpp | Removes encoder stale-step cleanup. |
| boards/marauder-touch/interface.cpp | Removes encoder stale-step cleanup. |
| boards/pinouts/pins_arduino.h | Adds new board and removes several existing ones. |
| platformio.ini | Switches default env, downgrades NimBLE pin, removes LTO flags. |
| patch.py | Removes HTTP request timeouts. |
| .gitignore | Removes .idea and .ai entries. |
| .github/workflows/buil_parallel.yml | Removes several boards from the matrix. |
Comments suppressed due to low confidence (19)
src/modules/bjs_interpreter/interpreter.h:1
FSis an abstract base class (e.g.SD,LittleFSinherit from it), so passing it by value causes object slicing and removes the polymorphic behavior — calls intofswill not dispatch to the concrete filesystem. The originalFS &fsreference signature should be preserved here and ininterpreter.cpp.
src/core/startup_app.cpp:1fsis left uninitialized and the previous null-check aftergetScriptsFolderis removed. IfgetScriptsFolderdoes not setfs(e.g. when no filesystem is available) this dereferences an indeterminate pointer. Initializefs = nullptr;and keep theif (fs == nullptr) return;guard.
src/core/serial_commands/storage_commands.cpp:1- Both
filepathandsizeStrare read from the same variablearg, andargis no longer declared in this scope after the rename. This will not compile, and the intent is clearly to readfilepathArg.getValue()andsizeArg.getValue()respectively.
src/core/type_convertion.cpp:1 - The previous comment explicitly noted the buffer was sized to 66 to accommodate the null terminator at index 65. Shrinking back to 65 reintroduces a one-byte out-of-bounds write when the routine null-terminates at the maximum index. Keep the size at 66.
src/modules/wifi/sniffer.cpp:1 - Replacing
strlcpy(..., sizeof(...))with unboundedstrcpyremoves bounds protection on the fixed-sizewifi_config.ap.ssid/passwordbuffers. Although the literals fit today, this regresses a defensive pattern; please retainstrlcpywith the destination size.
src/modules/wifi/sniffer.cpp:1 - Replacing
snprintf(buffer, sizeof(buffer), ...)withsprintfremoves the size guard. The same pattern is being reintroduced in several other files in this PR (core/wifi/wifi_mac.cpp,core/net_utils.cpp,core/configPins.cpp,core/utils.cpp,core/connect/esp_connection.cpp,core/serial_commands/settings_commands.cpp,core/wifi/webInterface.cpp). Please keepsnprintfto maintain bounds checking.
src/core/display.cpp:1 gifis not initialized in the constructor (thegifmember has no in-class initializer shown) andopenGIFmay fail beforegifis assigned. The previous version null-guarded bothclose()anddelete. Restore the null check, e.g.if (gif) { gif->close(); delete gif; gif = nullptr; }.
src/core/display.cpp:1- When
bSyncisfalse(the documented “play next frame regardless of timing” path) this implementation returns2without ever advancing the GIF, andgifis not null-checked. AdditionallydelayMillisecondsis initialized to&zero, so the timing branch only fires whenbSyncis true andmillis()-lTime >= 0, which makes the gating ineffective. Please null-checkgifand callplayFramewhenbSyncis false as well.
src/core/display.cpp:1 - Removing the zero-initialization here (and in
read32below) means partial-read failures (e.g. EOF on the SPI file) leaveresultwith indeterminate bytes that are still returned to the caller. Keepuint16_t result = 0;/uint32_t result = 0;.
src/modules/wifi/wifi_recover.cpp:1 total_timeandsecondsare computed but their only consumers are commented out, leaving dead code and unused-variable warnings. Either uncomment thepadprintfcalls or remove the computation.
src/core/wifi/wg.cpp:1local_ipis anIPAddress;Print::printlndoes not have a direct overload that prints it as dotted-quad, so this either fails to compile or prints an unexpected representation. The previous code usedlocal_ip.toString(). Please call.toString()explicitly.
src/core/menu_items/AppsMenu.cpp:1File::getNextFileName(bool*)is only available on recent arduino-esp32 cores; older targets in this repo (whichplatformio.inistill supports via multiple environments) may not have this overload. Consider using the portableopenNextFile()iteration to avoid breaking builds on those board envs.
src/core/led_control.h:1- Renumbering these macros silently changes the meaning of values already persisted in user configs (
bruceConfig.ledEffect). Users upgrading will see the wrong effect activate (e.g. a stored7previously meantRAINBOW_BREATHEand will now meanDISCO). Either keep the original numbering or add a migration step in config load.
src/modules/others/audio.cpp:1 - Removing the
frequency == 0 && duration > 0delay branch breaks the common “silent rest” usage ofplayTone(0, duration)— callers now get an immediate return instead of waitingdurationms, which will desync any sequence of notes that uses rests. Keep thedelay(duration)for the zero-frequency case.
src/modules/wifi/evil_portal.cpp:1 - This drops the user-configurable
bruceConfig.evilPortalGatewayIplookup and hardcodes192.168.4.1, while the matching “Default” menu entry that re-applied the configured value is also removed. Users who previously set a custom gateway IP will silently have their configuration ignored. Restore theapGateway.fromString(bruceConfig.evilPortalGatewayIp)fallback.
src/modules/ble/ble_common.cpp:1 is_ble_initedis declared at file scope here but is never referenced after the removal ofstopBLEStack, and several call sites incore/wifi/wifi_common.cpppreviously relied onstopBLEStack()for safe WiFi/BLE coexistence on no-PSRAM boards. Removing the teardown without an equivalent will cause radio init failures on those boards. Please either keepstopBLEStackand theFORCE_RADIO_TEARDOWN_ON_SWITCHpaths or document the replacement.
src/modules/wifi/tcp_utils.cpp:1- Removing the
serverIP == \"\\x1B\"/portString == \"\\x1B\"checks means pressing ESC at either prompt now falls through toclient.connectwith whatever string is currently buffered, instead of cancelling. Many other files in this PR have the same regression (wifi_recover.cpp,wifi_atks.cpp,responder.cpp, RFID modules, IR module, qrcode_menu, ble_spam, wifi_mac, passwords, etc.). Please retain the ESC-cancel guard.
src/core/menu_items/EthernetMenu.cpp:1 - The icon-drawing function has been fully rewritten with magic numbers (e.g.
iconCenterY - 25,iconH * 2, hardcoded15pin spacing) and no longer respects thescaleparameter consistently (e.g.Y = iconCenterY - 25ignoresscale). The previous version derived all positions fromscale, which made it work across screen sizes. Please scale the new layout the same way.
src/main.cpp:1 - Removing the
setBrightness(bruceConfig.bright, false)re-apply after_post_setup_gpio()regresses the fix for boards whose backlight is reset inside post-setup (per the deleted comment). On those boards the configured brightness will be lost on boot. Please retain the re-apply.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| digitalWrite(5, HIGH); | ||
| } | ||
| volatile bool kb_interrupt = false; | ||
| bool kb_interrupt = false; |
| {'input': js.read().decode('utf-8')}, | ||
| timeout=10 | ||
| ) |
| bool selected = false; | ||
| bool enabled = true; | ||
| bool (*hover)(void *hoverPointer, bool shouldRender); |
| board_build.flash_mode = qio | ||
| board_build.flash_size = 8MB | ||
| board_build.psram_type = opi | ||
| board_build.partitions = custom_8Mb.csv |
| mikalhart/TinyGPSPlus | ||
| tinyu-zhao/FFT@^0.0.1 | ||
| h2zero/NimBLE-Arduino@2.5 | ||
| h2zero/NimBLE-Arduino@^2.3.9 |
| if (n > VECTOR_DISPLAY_MAX_STRING) n = VECTOR_DISPLAY_MAX_STRING; | ||
| strncpy(args.xyText.text, str, n); |
Proposed Changes
This pull request adds support for an ESP32-S3 based board equipped with a 2.8" SPI TFT display. The implementation introduces a dedicated board configuration, display initialization, pin mappings, and PlatformIO environment while keeping existing board support unchanged.
The goal of this contribution is to expand Bruce Firmware compatibility to additional ESP32-S3 hardware platforms and provide a clean, maintainable implementation that follows the project's existing board architecture.
Types of Changes
Verification
The following verification steps were performed:
Testing
No automated tests were added because this change primarily introduces hardware-specific support.
Linked Issues
None.
User-Facing Change
Further Comments
This contribution was developed and tested on real hardware. The implementation is isolated to a dedicated board configuration to avoid affecting existing supported devices and to simplify future maintenance and enhancements.