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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "name:": "TTGO-T-Display", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file in question
fd -i "library.json" | grep -i "ttgo"Repository: wled/WLED
Length of output: 89
🏁 Script executed:
# If found, check the content
cat usermods/TTGO-T-Display/library.jsonRepository: wled/WLED
Length of output: 267
🌐 Web query:
PlatformIO library.json manifest schema specification format
💡 Result:
PlatformIO’s library.json manifest is a JSON object whose spec is defined by an official JSON Schema (JSON Schema draft 2020-12) published in the PlatformIO Core repo. (raw.githubusercontent.com)
Schema URL (use in $schema)
{
"$schema": "https://raw.githubusercontent.com/platformio/platformio-core/develop/platformio/assets/schema/library.json"
}What the schema covers (high level)
The schema defines (among others) these top-level fields and their types/constraints: name, version, description, keywords, homepage, repository, authors, license (SPDX ID/expression), frameworks, platforms, headers, examples, dependencies, export, scripts, build (e.g., flags, includeDir, srcDir, srcFilter, libArchive, libLDFMode, etc.). (raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/platformio/platformio-core/develop/platformio/assets/schema/library.json
- 2: https://docs.platformio.org/en/stable/manifests/library-json/fields/schema.html
- 3: https://raw.githubusercontent.com/platformio/platformio-core/develop/platformio/assets/schema/library.json
Fix the manifest key typo.
PlatformIO's library.json schema expects name, not name:. The colon makes this an invalid key per the schema specification, preventing proper library packaging metadata recognition.
Suggested fix
- "name:": "TTGO-T-Display",
+ "name": "TTGO-T-Display",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name:": "TTGO-T-Display", | |
| "name": "TTGO-T-Display", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/TTGO-T-Display/library.json` at 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.
| lib_deps = ${esp32.lib_deps} | ||
| custom_usermods = audioreactive TTGO-T-Display |
There was a problem hiding this comment.
Don't ship an unrelated usermod in the default TTGO env unless it's required.
custom_usermods = audioreactive TTGO-T-Display enables both modules, not just the display one. For a drop-in TTGO-T-Display environment, that unexpectedly changes the build surface and dependency set for everyone following the README.
Suggested fix
-custom_usermods = audioreactive TTGO-T-Display
+custom_usermods = TTGO-T-Display📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lib_deps = ${esp32.lib_deps} | |
| custom_usermods = audioreactive TTGO-T-Display | |
| lib_deps = ${esp32.lib_deps} | |
| custom_usermods = TTGO-T-Display |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/TTGO-T-Display/platformio_override.ini` around lines 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.
| ### Platformio_override.ini (added) | ||
| Copy the `platformio_override.ini` file which is contained in the `usermods/TTGO-T-Display/` folder into the root of your project folder. This file contains an override that remaps the button pin of WLED to use the on-board button to the right of the USB-C connector (when viewed with the port oriented downward - see hardware photo). | ||
|
|
||
| ### Platformio_overrides.ini (added) | ||
| Copy the `platformio_overrides.ini` file which is contained in the `usermods/TTGO-T-Display/` folder into the root of your project folder. This file contains an override that remaps the button pin of WLED to use the on-board button to the right of the USB-C connector (when viewed with the port oriented downward - see hardware photo). | ||
|
|
||
| ### TFT_eSPI Library Adjustments (board selection) | ||
| You need to modify a file in the `TFT_eSPI` library to select the correct board. If you followed the directions to modify and save the `platformio.ini` file above, the `User_Setup_Select.h` file can be found in the `/.pio/libdeps/esp32dev/TFT_eSPI_ID1559` folder. | ||
| ### Platformio.ini changes | ||
| Once the platformio_override.ini file has been copied as described above, the platformio.ini file isn't actually changed, but it is helpful to save the platformio.ini file in the VS Code application. This should trigger the download of the library dependencies. | ||
|
|
||
| Modify the `User_Setup_Select.h` file as follows: | ||
| * Comment out the following line (which is the 'default' setup file): | ||
| ```ini | ||
| //#include <User_Setup.h> // Default setup is root library folder | ||
| ``` | ||
| * Uncomment the following line (which points to the setup file for the TTGO T-Display): | ||
| ```ini | ||
| #include <User_Setups/Setup25_TTGO_T_Display.h> // Setup file for ESP32 and TTGO T-Display ST7789V SPI bus TFT | ||
| ``` | ||
| ### Change to the WLED_T-Display environment | ||
| This should appear as an option in the bottom toolbar. | ||
|
|
||
| Build the file. If you see a failure like this: | ||
| ```ini | ||
| xtensa-esp32-elf-g++: error: wled00\wled00.ino.cpp: No such file or directory | ||
| xtensa-esp32-elf-g++: fatal error: no input files | ||
| ``` | ||
| try building again. Sometimes this happens on the first build attempt and subsequent attempts build correctly. | ||
| Once the compilation is done and loaded onto the TTGO T-Display module, the display should show "Loading...", and then it will show the IP of the WLED access point. |
There was a problem hiding this comment.
These setup steps still describe two different integration models.
The new override-based workflow is correct, but Line 39 still tells users to copy usermod.cpp into wled00. That is now the wrong path for this library-based usermod, and it will confuse anyone following the README. Please replace the old step and show where custom_display_setup is configured in platformio_override.ini.
As per coding guidelines, usermods/** should live in their own directory and use a dedicated library.json to manage dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/TTGO-T-Display/README.md` around lines 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).
| 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.
2 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
Documentation
platformio_override.iniworkflow, eliminating manual configuration steps.Chores