TTGO-T-Display usermod fixup#5479
Conversation
details, and force an older version of dependent library TFT_eSPI.
Revised WLED_RELEASE_NAME to "WLED_T-Display"
Removed Serial.begin statement from userSetup
This allows a v1 usermod to be linked as a library, too. Of course, only one such usermod can be included, or a linker error will result.
Avoids any collisions with other source files.
Convert to a library-based usermod, allowing easy inclusion with the custom_usermods feature. The BTNPIN override and display model selection are handled via a build python script, so it is no longer necessary modify the TFT_eSPI library install.
Allow specifying the display setup file as an environment parameter. Files are looked for anywhere in the include path of WLED itself, and the direct dependencies of the TTGO-T-Display module (ie. TFT_eSPI).
Adds better support for newer CPUs.
Check if the path is fully qualified before searching; this allows using files outside of the WLED folder.
WalkthroughAdds a reusable ChangesTFT_eSPI_compat build library
TTGO-T-Display usermod refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
willmmiles
added
keep
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/TTGO-T-Display/usermod.cpp (1)
96-144:⚠️ Potential issue | 🟠 MajorAP mode never settles on ESP32.
The change detector uses
String(apSSID)whenapActiveis true, but the cached value is updated withWiFi.SSID()on ESP32. In AP mode those values differ, soneedRedrawstays true forever and the idle backlight timeout never kicks in.Suggested fix
`#if` defined(ESP8266) knownSsid = apActive ? WiFi.softAPSSID() : WiFi.SSID(); `#else` - knownSsid = WiFi.SSID(); + knownSsid = apActive ? String(apSSID) : WiFi.SSID(); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/TTGO-T-Display/usermod.cpp` around lines 96 - 144, The SSID change check in loop() uses String(apSSID) when apActive while knownSsid is later set via WiFi.SSID()/WiFi.softAPSSID(), causing AP mode to never settle; fix by making the initial SSID comparison use the same API as the update path (use WiFi.softAPSSID() when apActive and WiFi.SSID() otherwise) so ((apActive) ? WiFi.softAPSSID() : WiFi.SSID()) is used in the change detector (and ensure the knownSsid assignment in the update block matches that same conditional across ESP8266/ESP32).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/TTGO-T-Display/library.json`:
- Line 2: The manifest has a typo in the JSON key "name:" which violates
PlatformIO's schema; open the library.json and rename the property key "name:"
to "name" (keep the existing value "TTGO-T-Display") so the package metadata
conforms to PlatformIO's expected schema and is correctly recognized by
functions that read library.json.
In `@usermods/TTGO-T-Display/platformio_override.ini`:
- Around line 36-37: The platformio_override.ini currently enables both usermods
via the custom_usermods setting ("audioreactive TTGO-T-Display"), which
unintentionally adds the audioreactive module and its deps to the default TTGO
environment; update the custom_usermods value in platformio_override.ini to
include only the TTGO-T-Display usermod (or remove the combined entry and
document how to enable audioreactive separately), ensuring the symbol
"custom_usermods" no longer references "audioreactive" so only the intended
"TTGO-T-Display" usermod is enabled by default.
In `@usermods/TTGO-T-Display/README.md`:
- Around line 43-52: The README mixes two integration models: it still instructs
copying usermod.cpp into wled00 (old flow) even though this repo uses the
override-based workflow; update the README to remove the instruction to copy
usermod.cpp into wled00 and instead show where the custom_display_setup is
configured in platformio_override.ini (point to the custom_display_setup entry
and explain copying platformio_override.ini into the project root). Also add a
note that this usermod lives under usermods/TTGO-T-Display and follows the
library-based approach, and include that the usermod has a dedicated
library.json to manage dependencies (so users don’t attempt manual file copies).
In `@usermods/TTGO-T-Display/set_build_flags.py`:
- Around line 41-43: The forced-include flag is being passed as a single
whitespace-containing string in add_display_setup
(tgt_env.Append(CCFLAGS=f"-include {display_setup_path}")), which SCons will
split and thus break when display_setup_path contains spaces; change the CCFLAGS
Append to pass the flag and the path as separate argv entries (e.g., provide a
list/sequence containing the "-include" token and display_setup_path) so SCons
treats them as two distinct arguments and handles paths with spaces correctly.
---
Outside diff comments:
In `@usermods/TTGO-T-Display/usermod.cpp`:
- Around line 96-144: The SSID change check in loop() uses String(apSSID) when
apActive while knownSsid is later set via WiFi.SSID()/WiFi.softAPSSID(), causing
AP mode to never settle; fix by making the initial SSID comparison use the same
API as the update path (use WiFi.softAPSSID() when apActive and WiFi.SSID()
otherwise) so ((apActive) ? WiFi.softAPSSID() : WiFi.SSID()) is used in the
change detector (and ensure the knownSsid assignment in the update block matches
that same conditional across ESP8266/ESP32).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bec40c3-8c17-4246-8390-93bb0c5dacc5
📒 Files selected for processing (6)
usermods/TTGO-T-Display/README.mdusermods/TTGO-T-Display/library.jsonusermods/TTGO-T-Display/platformio_override.iniusermods/TTGO-T-Display/set_build_flags.pyusermods/TTGO-T-Display/usermod.cppwled00/usermod.cpp
| def add_display_setup(tgt_env): | ||
| tgt_env.Append(CPPDEFINES=[("USER_SETUP_LOADED", "1")]) | ||
| tgt_env.Append(CCFLAGS=f"-include {display_setup_path}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "set_build_flags.py" -path "*/usermods/*" 2>/dev/nullRepository: wled/WLED
Length of output: 97
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "./usermods/TTGO-T-Display/set_build_flags.py" ]; then
echo "=== File found, reading lines 35-50 ===";
sed -n '35,50p' "./usermods/TTGO-T-Display/set_build_flags.py" | cat -n;
else
echo "File not found at expected path. Searching for any set_build_flags.py...";
find . -name "set_build_flags.py" 2>/dev/null;
fiRepository: wled/WLED
Length of output: 884
🌐 Web query:
PlatformIO SCons CCFLAGS string vs array argument handling spaces
💡 Result:
In PlatformIO’s SCons environment, env["CCFLAGS"] is a SCons CLVar (command-line variable). That affects how strings vs lists are handled:
- If you pass a string (e.g.
env.Append(CCFLAGS='-DNAME="A B" -O2')), SCons will run itsSplit()logic and split on whitespace into multiple arguments (with some quote-aware behavior). As a side effect, “extra” spaces in string inputs get stripped. (scons.org) - If you pass a list/array (e.g.
env.Append(CCFLAGS=['-DNAME="A B"', '-O2'])), each list element is treated as one argument, so an element that contains spaces is preserved as a single flag. SCons explicitly recommends this when you need spaces preserved. (scons.org)
Rule of thumb:
- Use a string for normal flags where spaces just separate flags.
- Use a list when any single flag contains spaces (or you want to guarantee “one item = one argument”). (scons.org)
Citations:
- 1: https://scons.org/doc/4.5.2/PDF/scons-api.pdf
- 2: https://scons.org/doc/4.5.2/PDF/scons-api.pdf
- 3: https://scons.org/doc/4.5.2/PDF/scons-api.pdf
Pass the forced include as separate argv entries.
CCFLAGS=f"-include {display_setup_path}" breaks as soon as custom_display_setup resolves to a path containing spaces. SCons splits string arguments on whitespace, so paths with spaces (common on Windows) will be split into multiple arguments, breaking the -include flag and undermining the new full-path workflow.
Suggested fix
def add_display_setup(tgt_env):
tgt_env.Append(CPPDEFINES=[("USER_SETUP_LOADED", "1")])
- tgt_env.Append(CCFLAGS=f"-include {display_setup_path}")
+ tgt_env.Append(CCFLAGS=["-include", display_setup_path])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/TTGO-T-Display/set_build_flags.py` around lines 41 - 43, The
forced-include flag is being passed as a single whitespace-containing string in
add_display_setup (tgt_env.Append(CCFLAGS=f"-include {display_setup_path}")),
which SCons will split and thus break when display_setup_path contains spaces;
change the CCFLAGS Append to pass the flag and the path as separate argv entries
(e.g., provide a list/sequence containing the "-include" token and
display_setup_path) so SCons treats them as two distinct arguments and handles
paths with spaces correctly.
I think the reason for the build failure is crazy simple: tasmota has removed SPIFFS 😢 , so the display driver lib fails to compile (even when SPIFFs is not used, the lib needs SPIFFS.h) You can still build this usermod with the original arduino-esp32 framework from espressif. Lines 305 to 308 in bf948fa btw, same build failure in another usermods that use |
|
Re 18d4a41: Arg, I had unpushed commits in this branch, now I have to play rebase games. :( Would you mind not doing this in the future?
Yes, I know. There are better workarounds via the custom script. (This branch is labeled draft for a reason!) |
@willmmiles sorry and your're right - I should have thought for a moment about the "why DRAFT?" thing 😅 Good to know that you see a way via custom script, i was a bit over-nervous maybe when I found out that tasmota simply does not have "SPIFFS.h", so all our TFT_eSPI based usermods will suffer from the same problem. |
Yes. It's complicated by the fact that the TFT_eSPI library seemed to be unmaintained for a year or so there. I think it's faced stiff competition with Espressif's well optimized I've shimmed it in my WIP build with |
- Use latest TFT-ESPI - Fix include in script - Fix name
Ensure pins are registered together to avoid partial success.
Add a shim layer for TFT_espi that works around the missing SPIFFS.h dependency. Integrate some logic for accepting display definitions via a PlatformIO custom property as an alternative to a dozen command line switches; this improves cache viability, too.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/TFT_eSPI_compat/configure.py (1)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: avoid computing
os.path.join(...)twice per candidate.The generator evaluates
os.path.join(str(p), display_setup)in both the predicate and the value expression. A small comprehension over precomputed candidates reads cleaner and halves the joins.♻️ Suggested refactor
- setup_path = next( - ( - os.path.join(str(p), display_setup) - for p in search_paths - if os.path.isfile(os.path.join(str(p), display_setup)) - ), - None, - ) + candidates = (os.path.join(str(p), display_setup) for p in search_paths) + setup_path = next((c for c in candidates if os.path.isfile(c)), None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/TFT_eSPI_compat/configure.py` around lines 76 - 83, The setup path search in configure.py recomputes os.path.join(str(p), display_setup) twice for each candidate, so refactor the generator around next() to precompute each joined path once and reuse it for both the file check and the returned value. Update the logic near the setup_path assignment to iterate over prebuilt candidate paths from search_paths, keeping the same behavior while avoiding duplicate joins.usermods/TTGO-T-Display/usermod.cpp (1)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover dead code introduced in the refactor.
// tft.setRotation(3);is a commented-out duplicate of line 102 within the newly-writtensetup(). Per coding guidelines, dead/unused code should be removed or justified with a comment.🧹 Proposed cleanup
- - // tft.setRotation(3); ready = true;As per coding guidelines, "Remove dead/unused code or justify it with comments" for
**/{wled00,usermods}/**/*.{cpp,h,js,css}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/TTGO-T-Display/usermod.cpp` at line 121, Remove the leftover dead code in setup() for the TTGO-T-Display usermod: the commented-out tft.setRotation(3) duplicate should be deleted or replaced with a clear justification comment. Check the setup() logic in usermod.cpp and ensure only the active rotation call remains, with no unused refactor remnants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/TTGO-T-Display/usermod.cpp`:
- Around line 130-138: The config re-enable path in readFromConfig updates
enabled but leaves needRedraw unchanged, so loop exits the !enabled branch and
then skips the redraw/backlight-on logic. Update the enabled transition handling
in readFromConfig (and any shared state update path used by the UI) to set
needRedraw when the usermod changes from disabled to enabled, so loop forces the
display/backlight to turn back on immediately. Use the existing enabled,
needRedraw, and displayTurnedOff flags in usermod.cpp to keep the re-enable
behavior consistent with the disable path.
---
Nitpick comments:
In `@lib/TFT_eSPI_compat/configure.py`:
- Around line 76-83: The setup path search in configure.py recomputes
os.path.join(str(p), display_setup) twice for each candidate, so refactor the
generator around next() to precompute each joined path once and reuse it for
both the file check and the returned value. Update the logic near the setup_path
assignment to iterate over prebuilt candidate paths from search_paths, keeping
the same behavior while avoiding duplicate joins.
In `@usermods/TTGO-T-Display/usermod.cpp`:
- Line 121: Remove the leftover dead code in setup() for the TTGO-T-Display
usermod: the commented-out tft.setRotation(3) duplicate should be deleted or
replaced with a clear justification comment. Check the setup() logic in
usermod.cpp and ensure only the active rotation call remains, with no unused
refactor remnants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d22afe1d-29dd-4ce1-b92f-eb4f0f0580c7
📒 Files selected for processing (10)
lib/TFT_eSPI_compat/README.mdlib/TFT_eSPI_compat/configure.pylib/TFT_eSPI_compat/library.jsonlib/TFT_eSPI_compat/shims/SPIFFS.hlib/TFT_eSPI_compat/shims/User_Setup.husermods/TTGO-T-Display/README.mdusermods/TTGO-T-Display/library.jsonusermods/TTGO-T-Display/platformio_override.iniusermods/TTGO-T-Display/set_build_flags.pyusermods/TTGO-T-Display/usermod.cpp
✅ Files skipped from review due to trivial changes (2)
- lib/TFT_eSPI_compat/shims/User_Setup.h
- lib/TFT_eSPI_compat/library.json
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/TTGO-T-Display/library.json
| if (!enabled) { | ||
| #if DISPLAY_HAS_BACKLIGHT | ||
| if (!displayTurnedOff) { | ||
| digitalWrite(TFT_BL, !TFT_BACKLIGHT_ON); // Turn backlight off. | ||
| displayTurnedOff = true; | ||
| } | ||
| #endif | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Re-enabling via config doesn't force a redraw/backlight-on.
readFromConfig() updates enabled but never sets needRedraw. When the usermod is disabled then re-enabled through the WLED UI, loop() clears the !enabled early-return but needRedraw remains false (line 159's if (!needRedraw) branch is taken), so the backlight/display stays off until an unrelated tracked value (SSID, IP, brightness, mode, or palette) happens to change. Contrast with the disable path, which turns the backlight off immediately (lines 132-135).
🐛 Proposed fix to force a redraw on re-enable
bool readFromConfig(JsonObject& root) override
{
// default settings values could be set here (or below using the 3-argument getJsonValue()) instead of in the class definition or constructor
// setting them inside readFromConfig() is slightly more robust, handling the rare but plausible use case of single value being missing after boot (e.g. if the cfg.json was manually edited and a value was removed)
JsonObject top = root[FPSTR(_name)];
bool configComplete = !top.isNull();
- configComplete &= getJsonValue(top[FPSTR(_enabled)], enabled);
+ bool wasEnabled = enabled;
+ configComplete &= getJsonValue(top[FPSTR(_enabled)], enabled);
+ if (!wasEnabled && enabled) needRedraw = true; // force refresh/backlight-on when re-enabled
return configComplete;
}Also applies to: 273-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/TTGO-T-Display/usermod.cpp` around lines 130 - 138, The config
re-enable path in readFromConfig updates enabled but leaves needRedraw
unchanged, so loop exits the !enabled branch and then skips the
redraw/backlight-on logic. Update the enabled transition handling in
readFromConfig (and any shared state update path used by the UI) to set
needRedraw when the usermod changes from disabled to enabled, so loop forces the
display/backlight to turn back on immediately. Use the existing enabled,
needRedraw, and displayTurnedOff flags in usermod.cpp to keep the re-enable
behavior consistent with the disable path.
3 participants
Import all the latest fixes for this usermod, and upgrade it to v2. Also includes the ability to specify the display setup header as a custom property in
platformio_override.ini.Replaces #4600.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes