Claude/i2c-rotary-encoder-ui-AJq6U#5653
Closed
Mdevlin4 wants to merge 3 commits into
Closed
Conversation
Adds a new usermod at usermods/usermod_v2_i2c_encoder_ui that delivers the same WLED UI controls as the existing Rotary Encoder UI (brightness, effect, speed, intensity, palette, hue, saturation, CCT, presets, and custom sliders), but reads from an I2C rotary encoder instead of GPIO pins. Targets the M5Stack Encoder Unit (U135, I2C address 0x40) using the device's V1 register protocol directly over the globally-configured WLED I2C bus — no encoder-specific GPIO pins, no external library. Configurable settings: I2C address, preset range, apply-to-all-segments. FourLineDisplay integration is preserved. Usermod ID 59 added to const.h.
Adds a new usermod at usermods/usermod_v2_gc9a01_display for the Waveshare 1.28" Round LCD Module (240x240 IPS, GC9A01 driver, SPI). Shows a circular watch-face layout: outer arc for brightness (0-360° yellow progress), colour disc filled with the primary segment colour, effect name, brightness %, and IP address. Screen blanks after a configurable timeout. Uses TFT_eSPI (Bodmer) which requires compile-time pin configuration via build flags — a ready-to-use platformio_override.sample.ini is included. Pins are registered with PinManager for conflict detection. Exposes wakeDisplay(), showOverlay(), and forceRedraw() for pairing with the I2C encoder usermod in a later pass. USERMOD_ID_GC9A01_DISPLAY (60) added to const.h. UM_GC9A01Display and UM_I2CEncoderUI added to pin_manager.h.
Contributor
WalkthroughAdds two new usermods: a GC9A01 round TFT display (watch-face UI, backlight timeout, overlays, config) and an I2C rotary-encoder UI (encoder/button polling, parameter state machine, optional FourLineDisplay integration). Updates core headers with two new USERMOD_IDs and PinOwner entries, and includes manifests, PlatformIO sample, and docs. ChangesUsermod Registry Infrastructure
GC9A01 Round Display Usermod
I2C Encoder UI Usermod
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
softhack007
added
the
AI
coderabbitai
Bot
added
enhancement
usermod
softhack007
added
the
AI
Member
|
@coderabbitai only the user "claude" has pushed to this PR, so we should assume it's largely AI-generated code (see your specific additional checks in .coderabbit.yaml). |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the coderabbit configuration file
cat .coderabbit.yaml 2>/dev/null || cat .coderabbit.yml 2>/dev/null || echo "No .coderabbit.yaml found"
# Also check PR files for scope
gh pr view 5653 --repo wled/WLED --json files --jq '.files[].path' 2>/dev/null | head -60Length of output: 7068
Triggering a full re-review now with these checks in mind. Please apply all six AI-generated-code checks from the ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (1)
152-191: ⚡ Quick winRemove or use currently dead helper methods.
fillDisplay()anddrawCenteredText()are unused in the current implementation. Please either delete them now or add a short TODO explaining the planned usage.As per coding guidelines, AI-generated contributions should be checked for dead/disabled code and cleaned up.
🤖 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/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around lines 152 - 191, The two helper functions fillDisplay() and drawCenteredText() are currently unused; either remove them or mark them for future use: delete the functions fillDisplay(uint32_t) and drawCenteredText(const char*, int16_t, uint8_t, uint32_t) from usermod_v2_gc9a01_display.cpp if they're not needed, or add a one-line TODO comment above each (e.g. "// TODO: retained for future use in X feature") so their presence is intentional and documented; ensure you update any include/forward declarations if present and run a build to confirm no references remain.usermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp (1)
1-727: ⚡ Quick winWrap this contiguous AI-generated C++ block with AI boundary comments.
Given the PR context, this file should include the required
// AI: below section was generated by an AI…// AI: endmarkers.As per coding guidelines: “When AI-generated code blocks are large/contiguous, mark them with
// AI: below section was generated by an AI ... // AI: end.”🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` around lines 1 - 727, The file lacks the required AI-generated block markers; wrap the contiguous AI-generated C++ section with the exact comments "// AI: below section was generated by an AI" at the start and "// AI: end" at the end. Place the opening marker immediately before the AI-generated block (for example just above the I2CEncoderUIUsermod class definition or right after the initial includes) and the closing marker immediately after the end of that block (for example after the REGISTER_USERMOD call), ensuring the markers enclose everything from the start of the generated code through the end of the class/registration so symbols like I2CEncoderUIUsermod, readEncoderDelta, writeReg, setup, loop, and REGISTER_USERMOD(usermod_v2_i2c_encoder_ui) are inside the marked region.
🤖 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/usermod_v2_gc9a01_display/library.json`:
- Around line 4-7: The library.json currently lists "SPI": "" under
"dependencies" which is incorrect because SPI is a core framework library;
remove the "SPI" entry from the "dependencies" object in
usermod_v2_gc9a01_display's library.json so dependency resolution relies on
framework includes (e.g., `#include` <SPI.h>) and only keep external entries like
"bodmer/TFT_eSPI": "~2.5.43".
In `@usermods/usermod_v2_gc9a01_display/readme.md`:
- Around line 8-19: The fenced ASCII-art code block in readme.md is missing a
language tag which triggers markdownlint MD040; update the triple-backtick fence
that surrounds the watch-face diagram to include a language (e.g., "text") so it
becomes ```text, ensuring the diagram is treated as plain text and the linter
warning is resolved; locate the diagram block in
usermod_v2_gc9a01_display/readme.md and add the language to that opening fence.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp`:
- Around line 1-459: The file contains a large AI-generated block (the
GC9A01DisplayUsermod class and related globals like TFT_eSPI tft and static
GC9A01DisplayUsermod gc9a01_display) that must be wrapped with the required AI
markers; add a comment line "// AI: below section was generated by an AI"
immediately before the start of the generated block (before the TFT_eSPI tft =
... line or before the class definition) and add "// AI: end" immediately after
the end of that block (after REGISTER_USERMOD(gc9a01_display);), ensuring both
markers are plain single-line comments and do not alter existing code or
spacing.
- Around line 383-385: The code treats screenTimeoutSec==0 as a 0ms timeout and
blanks immediately; modify the logic around screenTimeoutSec/timeoutMs so a
value of 0 means "never" — e.g. only compute timeoutMs and perform the blanking
check when screenTimeoutSec > 0 (or explicitly skip the (now - lastChange >
timeoutMs) check when timeoutMs == 0); update the block referencing
screenTimeoutSec, timeoutMs, displayOff, lastChange, now and setBacklight to
ensure no blanking occurs for screenTimeoutSec==0.
In `@usermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp`:
- Around line 157-173: The code in sortModesAndPalettes (and the nearby block at
175-183) uses allocations returned by re_initIndexArray and re_findModeStrings
and then dereferences them immediately; modify sortModesAndPalettes to check
each pointer returned (modes_qstrings, modes_alpha_indexes, palettes_qstrings,
palettes_alpha_indexes) for null before using them, and if any allocation fails,
perform a safe failure path (log an error via existing logger, free any
previously allocated arrays if needed, and abort/return from setup to avoid
dereferencing null). Ensure you also guard the customPalettes loop and the
pgm_read_byte_near access by confirming palettes_qstrings is valid and that
indexes are within bounds before writing. Use the helper names
re_initIndexArray, re_findModeStrings, and re_sortModes to locate changes.
- Around line 344-345: The menu allows entering "Preset" when both presetHigh
and presetLow are non-zero but changePreset() only acts when presetHigh >
presetLow, causing a no-op selectable item; update the eligibility check in the
menu (the case that writes "Preset" into lineBuffer and sets
changedState/newState) to require presetHigh > presetLow instead of merely
non-zero, and make the same analogous change for the duplicate block around the
changePreset() call (lines handling the same preset menu logic) so the menu
selection and changePreset() use the same condition.
- Around line 651-662: changeCCT() always updates every active segment; change
it to respect applyToAll like the other parameter handlers: after computing
currentCCT, if applyToAll is true keep the existing loop over
strip.getSegmentsNum() calling seg.setCCT(currentCCT), otherwise only update the
currently selected segment(s) (the same selected-segment index/pattern used by
the other handlers) — check and use the same symbol those handlers use to find
the selected segment (e.g., currentSegmentIndex / getSelectedSegmentIndex or
equivalent) and call seg.setCCT(currentCCT) only for that segment (keeping the
seg.isActive() guard), then call lampUdated().
---
Nitpick comments:
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp`:
- Around line 152-191: The two helper functions fillDisplay() and
drawCenteredText() are currently unused; either remove them or mark them for
future use: delete the functions fillDisplay(uint32_t) and
drawCenteredText(const char*, int16_t, uint8_t, uint32_t) from
usermod_v2_gc9a01_display.cpp if they're not needed, or add a one-line TODO
comment above each (e.g. "// TODO: retained for future use in X feature") so
their presence is intentional and documented; ensure you update any
include/forward declarations if present and run a build to confirm no references
remain.
In `@usermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp`:
- Around line 1-727: The file lacks the required AI-generated block markers;
wrap the contiguous AI-generated C++ section with the exact comments "// AI:
below section was generated by an AI" at the start and "// AI: end" at the end.
Place the opening marker immediately before the AI-generated block (for example
just above the I2CEncoderUIUsermod class definition or right after the initial
includes) and the closing marker immediately after the end of that block (for
example after the REGISTER_USERMOD call), ensuring the markers enclose
everything from the start of the generated code through the end of the
class/registration so symbols like I2CEncoderUIUsermod, readEncoderDelta,
writeReg, setup, loop, and REGISTER_USERMOD(usermod_v2_i2c_encoder_ui) are
inside the marked region.
🪄 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: 012a27a6-7c4a-4744-88b5-48816770cb5e
📒 Files selected for processing (8)
usermods/usermod_v2_gc9a01_display/library.jsonusermods/usermod_v2_gc9a01_display/platformio_override.sample.iniusermods/usermod_v2_gc9a01_display/readme.mdusermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cppusermods/usermod_v2_i2c_encoder_ui/readme.mdusermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cppwled00/const.hwled00/pin_manager.h
Comment on lines
+4
to
+7
| "dependencies": { | ||
| "bodmer/TFT_eSPI": "~2.5.43", | ||
| "SPI": "" | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all usermod library.json files
fd -a "library.json" usermods -t f > /tmp/usermod_library_jsons.txt
echo "Found $(wc -l < /tmp/usermod_library_jsons.txt) usermod library.json files."
# Count occurrences of "SPI" in dependency blocks
echo "=== Occurrences of \"SPI\" in usermods/library.json ==="
rg -n '"SPI"\s*:' usermods --glob '**/library.json' || true
# Show a few representative manifests that mention SPI (limit output)
echo
echo "=== Sample manifests containing \"SPI\" ==="
rg -n '"SPI"\s*:' usermods --glob '**/library.json' | head -n 20 | cut -d: -f1 | sort -u | head -n 20 > /tmp/spi_manifest_paths.txt || true
if [ -s /tmp/spi_manifest_paths.txt ]; then
while read -r p; do
echo "--- $p ---"
cat -n "$p" | sed -n '1,120p'
done < /tmp/spi_manifest_paths.txt
else
echo "No usermod manifests with \"SPI\" dependency found."
fi
# Also check non-usermod library.json in repo (if any)
echo
echo "=== Occurrences of \"SPI\" in all library.json files ==="
fd -a "library.json" -t f > /tmp/all_library_jsons.txt
rg -n '"SPI"\s*:' $(cat /tmp/all_library_jsons.txt) || trueRepository: wled/WLED
Length of output: 1114
🌐 Web query:
PlatformIO library.json dependencies Arduino core library SPI
💡 Result:
In PlatformIO, you generally do not need to (and should not) explicitly declare core Arduino libraries like SPI, Wire, or SoftwareSerial in your library.json dependencies [1]. These libraries are built-in components of the Arduino core platform [1]. PlatformIO's Library Dependency Finder (LDF) is designed to automatically detect when you include these headers in your code (e.g., #include <SPI.h>) and will link the necessary core library automatically during the build process [1]. If you are developing a library and want to ensure it works across different platforms that might handle built-in libraries differently, the recommended approach is: 1. Simply include the required header in your source code [1]. 2. Avoid adding built-in core libraries to the dependencies field in library.json, as this can lead to conflicts or unnecessary installation attempts for packages that are already provided by the core framework [1]. 3. The library.json dependencies field should be reserved for external, third-party libraries that need to be downloaded from the PlatformIO Registry or other repositories [2][3]. If you encounter issues where the compiler cannot find SPI or other core libraries, ensure that your platformio.ini file is correctly configured for the desired board and framework. The LDF is highly capable of resolving these dependencies without manual intervention provided the include statements are present in your source files [1].
Citations:
- 1: https://community.platformio.org/t/confusion-with-the-library-dependency-declarations-in-platformio-ini-targeting-arduino-framework/25861
- 2: https://docs.platformio.org/en/latest/manifests/library-json/fields/dependencies.html
- 3: https://docs.platformio.org/en/latest/manifests/library-json/index.html
🌐 Web query:
PlatformIO does library.json dependency "SPI" fetch from registry or uses framework
💡 Result:
In PlatformIO, "SPI" is typically a built-in library provided by the development framework (such as Arduino or ESP-IDF), not a package fetched from the PlatformIO Registry [1][2]. Because "SPI" is part of the framework, it does not need to be declared in the dependencies section of a library.json file as an external package [3][1]. If you encounter a compilation error stating "SPI.h: No such file or directory," it is often because the PlatformIO Library Dependency Finder (LDF) has not automatically detected the dependency [1][4]. To resolve this without needing to fetch a separate registry library, you can try the following: 1. Use an explicit include: Sometimes adding #include <SPI.h> directly in your project's main source file helps the LDF discover that the SPI library is required [4][2]. 2. Adjust LDF mode: If the library is nested or hidden, you can change the dependency finder mode in your platformio.ini file to force a deeper search [1][4]. For example: lib_ldf_mode = chain+ Do not attempt to add "SPI" to the library.json dependencies or lib_deps as if it were a third-party library, as this can lead to conflicts or redundant installations [1]. If your project absolutely requires a specific, non-default version of SPI, you must ensure the framework being used provides it [1].
Citations:
- 1: Compile breaks by unused library parts. platformio/platformio-core#4265
- 2: https://community.simplefoc.com/t/tutorial-getting-started-with-platformio-ide-and-the-simple-foc-library/97
- 3: https://docs.platformio.org/en/latest/manifests/library-json/index.html
- 4: Include path doesn't always contain the dependencies from a library.json file platformio/platformio-core#4900
Remove SPI from usermod_v2_gc9a01_display library.json dependencies
SPI is a core Arduino framework library; it shouldn’t be declared as a PlatformIO registry dependency in library.json (LDF/framework linkage should handle it based on #include <SPI.h>), and keeping it can add unnecessary dependency-resolution/build variability.
Suggested change
"dependencies": {
- "bodmer/TFT_eSPI": "~2.5.43",
- "SPI": ""
+ "bodmer/TFT_eSPI": "~2.5.43"
}📝 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.
Suggested change
| "dependencies": { | |
| "bodmer/TFT_eSPI": "~2.5.43", | |
| "SPI": "" | |
| } | |
| "dependencies": { | |
| "bodmer/TFT_eSPI": "~2.5.43" | |
| } |
🤖 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/usermod_v2_gc9a01_display/library.json` around lines 4 - 7, The
library.json currently lists "SPI": "" under "dependencies" which is incorrect
because SPI is a core framework library; remove the "SPI" entry from the
"dependencies" object in usermod_v2_gc9a01_display's library.json so dependency
resolution relies on framework includes (e.g., `#include` <SPI.h>) and only keep
external entries like "bodmer/TFT_eSPI": "~2.5.43".
Comment on lines
+8
to
+19
| ``` | ||
| ╭──────────────────╮ | ||
| ╱ │ ░░░░░░░░░░░░░░ │ ╲ ← brightness arc (yellow, 0–360°) | ||
| ╱ │ [Effect Name] │ ╲ | ||
| │ │ ╭────────────╮ │ │ | ||
| │ │ │ │ │ │ ← colour disc (primary segment colour) | ||
| │ │ │ 75 % │ │ │ | ||
| │ │ ╰────────────╯ │ │ | ||
| ╲ │ 192.168.1.10 │ ╱ | ||
| ╲ │ │ ╱ | ||
| ╰──────────────────╯ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced code block.
The watch-face diagram block is missing a fence language, which triggers markdownlint MD040.
Suggested change
-```
+```text
╭──────────────────╮
...
╰──────────────────╯</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/usermod_v2_gc9a01_display/readme.md around lines 8 - 19, The fenced
ASCII-art code block in readme.md is missing a language tag which triggers
markdownlint MD040; update the triple-backtick fence that surrounds the
watch-face diagram to include a language (e.g., "text") so it becomes ```text,
ensuring the diagram is treated as plain text and the linter warning is
resolved; locate the diagram block in usermod_v2_gc9a01_display/readme.md and
add the language to that opening fence.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+1
to
+459
| #include "wled.h" | ||
|
|
||
| // | ||
| // GC9A01 Round Display Usermod | ||
| // | ||
| // Adds support for the Waveshare 1.28" Round LCD Module (and compatible | ||
| // GC9A01-based 240×240 IPS displays) as a WLED status screen. | ||
| // | ||
| // Uses TFT_eSPI (Bodmer) which must be configured via build flags — | ||
| // see platformio_override.sample.ini for a ready-to-use example. | ||
| // | ||
| // Displays: | ||
| // • Outer ring : brightness progress arc | ||
| // • Centre fill : primary segment colour | ||
| // • Top text : current effect name | ||
| // • Centre text : brightness percentage | ||
| // • Bottom text : IP address (or "No WiFi") | ||
| // | ||
| // Public API (for pairing with a rotary encoder or other usermods): | ||
| // wakeDisplay() — wake from timeout, returns true if was sleeping | ||
| // showOverlay(...) — display temporary text over the watch face | ||
| // forceRedraw() — request immediate full refresh | ||
| // | ||
|
|
||
| #ifdef USERMOD_GC9A01_DISPLAY | ||
|
|
||
| #include <TFT_eSPI.h> | ||
| #include <SPI.h> | ||
|
|
||
| // Verify required build flags are present | ||
| #ifndef USER_SETUP_LOADED | ||
| #ifndef GC9A01_DRIVER | ||
| #error "GC9A01_DISPLAY: define GC9A01_DRIVER=1 in build flags" | ||
| #endif | ||
| #ifndef TFT_WIDTH | ||
| #error "GC9A01_DISPLAY: define TFT_WIDTH=240 in build flags" | ||
| #endif | ||
| #ifndef TFT_HEIGHT | ||
| #error "GC9A01_DISPLAY: define TFT_HEIGHT=240 in build flags" | ||
| #endif | ||
| #ifndef TFT_CS | ||
| #error "GC9A01_DISPLAY: define TFT_CS in build flags" | ||
| #endif | ||
| #ifndef TFT_DC | ||
| #error "GC9A01_DISPLAY: define TFT_DC in build flags" | ||
| #endif | ||
| #ifndef TFT_RST | ||
| #error "GC9A01_DISPLAY: define TFT_RST in build flags" | ||
| #endif | ||
| #endif | ||
|
|
||
| #ifndef TFT_BL | ||
| #define TFT_BL -1 | ||
| #endif | ||
|
|
||
| // How often the loop checks for state changes (ms) | ||
| #define GC9A01_REFRESH_MS 500 | ||
| // Screen off after this many ms without a state change | ||
| #define GC9A01_TIMEOUT_MS (5UL * 60UL * 1000UL) // 5 minutes | ||
| // Overlay display duration default (ms) | ||
| #define GC9A01_OVERLAY_MS 2000 | ||
| // Max chars for effect/palette name display | ||
| #define GC9A01_NAME_BUF 24 | ||
|
|
||
| // Display geometry | ||
| #define GC9A01_CX 120 // centre X | ||
| #define GC9A01_CY 120 // centre Y | ||
| #define GC9A01_R 120 // display radius | ||
| // Brightness arc: outer radius, inner radius | ||
| #define GC9A01_ARC_R_OUT 115 | ||
| #define GC9A01_ARC_R_IN 104 | ||
|
|
||
| // ------------------------------------------------------------------------- | ||
| // File-scope TFT object (must be file-scope so build flags apply) | ||
| // ------------------------------------------------------------------------- | ||
| TFT_eSPI tft = TFT_eSPI(TFT_WIDTH, TFT_HEIGHT); | ||
|
|
||
| // ------------------------------------------------------------------------- | ||
|
|
||
| class GC9A01DisplayUsermod : public Usermod { | ||
|
|
||
| private: | ||
|
|
||
| bool enabled = true; | ||
| bool initDone = false; | ||
| bool displayOff = false; | ||
| bool needRedraw = true; | ||
|
|
||
| // For state-change detection | ||
| uint8_t knownBri = 0; | ||
| uint8_t knownMode = 255; | ||
| uint32_t knownColor = 0; | ||
| bool knownWifi = false; | ||
| IPAddress knownIp; | ||
|
|
||
| unsigned long lastCheck = 0; | ||
| unsigned long lastChange = 0; // millis of last detected state change | ||
|
|
||
| // Overlay support | ||
| bool overlayActive = false; | ||
| unsigned long overlayUntil = 0; | ||
| char overlayLine1[GC9A01_NAME_BUF] = {0}; | ||
| char overlayLine2[GC9A01_NAME_BUF] = {0}; | ||
|
|
||
| // Config | ||
| uint16_t screenTimeoutSec = 300; // 5 minutes | ||
| bool flipDisplay = false; | ||
|
|
||
| static const char _name[]; | ||
| static const char _enabled[]; | ||
| static const char _timeout[]; | ||
| static const char _flip[]; | ||
|
|
||
| // ---------------------------------------------------------------- helpers | ||
|
|
||
| // Copy TFT_MOSI / TFT_SCLK compile-time pins into WLED's global SPI vars | ||
| // so PinManager and the web UI see them (same pattern as pixels_dice_tray). | ||
| static void setSPIPinsFromMacros() { | ||
| #ifdef TFT_MOSI | ||
| spi_mosi = TFT_MOSI; | ||
| #endif | ||
| #ifdef TFT_MISO | ||
| #if defined(TFT_MOSI) && (TFT_MISO == TFT_MOSI) | ||
| spi_miso = -1; // shared data line — not a real MISO | ||
| #else | ||
| spi_miso = TFT_MISO; | ||
| #endif | ||
| #endif | ||
| #ifdef TFT_SCLK | ||
| spi_sclk = TFT_SCLK; | ||
| #endif | ||
| } | ||
|
|
||
| // Convert WLED 0xRRGGBB(WW) to TFT RGB565 | ||
| uint16_t wledToRGB565(uint32_t color) { | ||
| uint8_t r = (color >> 16) & 0xFF; | ||
| uint8_t g = (color >> 8) & 0xFF; | ||
| uint8_t b = color & 0xFF; | ||
| return tft.color565(r, g, b); | ||
| } | ||
|
|
||
| void setBacklight(bool on) { | ||
| if (TFT_BL < 0) return; | ||
| #ifndef TFT_BACKLIGHT_ON | ||
| #define TFT_BACKLIGHT_ON HIGH | ||
| #endif | ||
| digitalWrite(TFT_BL, on ? TFT_BACKLIGHT_ON : (TFT_BACKLIGHT_ON == HIGH ? LOW : HIGH)); | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- drawing | ||
|
|
||
| // Fill the circular display background with a solid colour. | ||
| void fillDisplay(uint32_t color) { | ||
| tft.fillCircle(GC9A01_CX, GC9A01_CY, GC9A01_R, wledToRGB565(color)); | ||
| } | ||
|
|
||
| // Draw the brightness arc (0° = 12 o'clock, clockwise). | ||
| // TFT_eSPI drawArc: angle 0 = top, clockwise. | ||
| void drawBrightnessArc(uint8_t brightness) { | ||
| // Background ring | ||
| tft.drawArc(GC9A01_CX, GC9A01_CY, | ||
| GC9A01_ARC_R_OUT, GC9A01_ARC_R_IN, | ||
| 0, 360, | ||
| 0x2104 /*dark grey*/, TFT_BLACK); | ||
| if (brightness > 0) { | ||
| uint16_t endAngle = (uint16_t)((uint32_t)brightness * 360 / 255); | ||
| if (endAngle == 0) endAngle = 1; | ||
| tft.drawArc(GC9A01_CX, GC9A01_CY, | ||
| GC9A01_ARC_R_OUT, GC9A01_ARC_R_IN, | ||
| 0, endAngle, | ||
| TFT_YELLOW, TFT_BLACK); | ||
| } | ||
| } | ||
|
|
||
| // Draw the primary-colour filled centre circle. | ||
| void drawColorDisk(uint32_t color) { | ||
| uint16_t c565 = wledToRGB565(color); | ||
| // 80 px radius gives a nice centre disc with room for text | ||
| tft.fillCircle(GC9A01_CX, GC9A01_CY, 80, c565); | ||
| // Thin border between disc and text area | ||
| tft.drawCircle(GC9A01_CX, GC9A01_CY, 80, TFT_BLACK); | ||
| } | ||
|
|
||
| // Centered text helper — writes text centered at (cx, y). | ||
| void drawCenteredText(const char *text, int16_t y, | ||
| uint8_t textSize, uint32_t color) { | ||
| tft.setTextSize(textSize); | ||
| tft.setTextDatum(TC_DATUM); | ||
| tft.setTextColor(color, TFT_BLACK); | ||
| tft.drawString(text, GC9A01_CX, y); | ||
| } | ||
|
|
||
| // Full watch-face redraw. | ||
| void drawWatchFace() { | ||
| uint32_t primaryColor = RGBW32(colPri[0], colPri[1], colPri[2], colPri[3]); | ||
|
|
||
| // Black background | ||
| tft.fillScreen(TFT_BLACK); | ||
|
|
||
| // Outer brightness ring | ||
| drawBrightnessArc(bri); | ||
|
|
||
| // Coloured centre disc | ||
| drawColorDisk(primaryColor); | ||
|
|
||
| // Effect name — white text above centre | ||
| char modeName[GC9A01_NAME_BUF]; | ||
| extractModeName(effectCurrent, JSON_mode_names, modeName, GC9A01_NAME_BUF - 1); | ||
| tft.setTextColor(TFT_WHITE, TFT_BLACK); | ||
| tft.setTextDatum(TC_DATUM); | ||
| tft.setTextSize(2); | ||
| tft.drawString(modeName, GC9A01_CX, 54); | ||
|
|
||
| // Brightness % — large, centred on the disc | ||
| char briBuf[8]; | ||
| snprintf_P(briBuf, sizeof(briBuf), PSTR("%d%%"), (bri * 100 + 127) / 255); | ||
|
|
||
| // Pick a contrasting text colour based on primary colour brightness | ||
| uint8_t luma = (77 * colPri[0] + 150 * colPri[1] + 29 * colPri[2]) >> 8; | ||
| uint16_t textCol = (luma > 128) ? TFT_BLACK : TFT_WHITE; | ||
| tft.setTextDatum(MC_DATUM); | ||
| tft.setTextColor(textCol); | ||
| tft.setTextSize(3); | ||
| tft.drawString(briBuf, GC9A01_CX, GC9A01_CY); | ||
|
|
||
| // IP address — small, bottom of display | ||
| char ipBuf[20]; | ||
| if (WLED_CONNECTED) { | ||
| IPAddress ip = Network.localIP(); | ||
| snprintf_P(ipBuf, sizeof(ipBuf), PSTR("%d.%d.%d.%d"), | ||
| ip[0], ip[1], ip[2], ip[3]); | ||
| } else if (apActive) { | ||
| strlcpy_P(ipBuf, PSTR("AP 4.3.2.1"), sizeof(ipBuf)); | ||
| } else { | ||
| strlcpy_P(ipBuf, PSTR("No WiFi"), sizeof(ipBuf)); | ||
| } | ||
| tft.setTextDatum(BC_DATUM); | ||
| tft.setTextColor(TFT_DARKGREY); | ||
| tft.setTextSize(1); | ||
| tft.drawString(ipBuf, GC9A01_CX, 195); | ||
| } | ||
|
|
||
| // Overlay: two lines of text in a rounded rect, auto-cleared after duration. | ||
| void drawOverlay() { | ||
| tft.fillRoundRect(30, 85, 180, 70, 12, TFT_NAVY); | ||
| tft.drawRoundRect(30, 85, 180, 70, 12, TFT_WHITE); | ||
| tft.setTextDatum(MC_DATUM); | ||
| tft.setTextColor(TFT_WHITE); | ||
| tft.setTextSize(2); | ||
| tft.drawString(overlayLine1, GC9A01_CX, 108); | ||
| if (overlayLine2[0]) { | ||
| tft.setTextSize(1); | ||
| tft.drawString(overlayLine2, GC9A01_CX, 135); | ||
| } | ||
| } | ||
|
|
||
| // WLED splash screen shown once at boot. | ||
| void drawSplash() { | ||
| tft.fillScreen(TFT_BLACK); | ||
| tft.setTextDatum(MC_DATUM); | ||
| tft.setTextColor(TFT_ORANGE); | ||
| tft.setTextSize(4); | ||
| tft.drawString("WLED", GC9A01_CX, GC9A01_CY - 20); | ||
| tft.setTextColor(TFT_DARKGREY); | ||
| tft.setTextSize(1); | ||
| tft.drawString(versionString, GC9A01_CX, GC9A01_CY + 25); | ||
| } | ||
|
|
||
| public: | ||
|
|
||
| uint16_t getId() override { return USERMOD_ID_GC9A01_DISPLAY; } | ||
|
|
||
| // ------------------------------------------------------ public API | ||
|
|
||
| // Wake the display from timeout sleep. | ||
| // Returns true if the display was sleeping (so callers can discard input). | ||
| bool wakeDisplay() { | ||
| if (!enabled || !initDone) return false; | ||
| if (displayOff) { | ||
| setBacklight(true); | ||
| displayOff = false; | ||
| needRedraw = true; | ||
| lastChange = millis(); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // Show a temporary two-line overlay over the watch face. | ||
| void showOverlay(const char *line1, const char *line2 = nullptr, | ||
| uint32_t durationMs = GC9A01_OVERLAY_MS) { | ||
| if (!enabled || !initDone) return; | ||
| wakeDisplay(); | ||
| strlcpy(overlayLine1, line1 ? line1 : "", sizeof(overlayLine1)); | ||
| strlcpy(overlayLine2, line2 ? line2 : "", sizeof(overlayLine2)); | ||
| overlayUntil = millis() + durationMs; | ||
| overlayActive = true; | ||
| drawOverlay(); | ||
| } | ||
|
|
||
| // Request an immediate full redraw on the next loop tick. | ||
| void forceRedraw() { | ||
| needRedraw = true; | ||
| lastChange = millis(); | ||
| } | ||
|
|
||
| // ------------------------------------------------------ lifecycle | ||
|
|
||
| void setup() override { | ||
| DEBUG_PRINTLN(F("GC9A01 Display: init.")); | ||
|
|
||
| setSPIPinsFromMacros(); | ||
|
|
||
| // Register shared SPI bus pins | ||
| PinManagerPinType spiPins[] = { | ||
| { spi_mosi, true }, { spi_miso, false }, { spi_sclk, true } | ||
| }; | ||
| if (spi_sclk >= 0 && !PinManager::allocateMultiplePins(spiPins, 3, PinOwner::HW_SPI)) { | ||
| DEBUG_PRINTLN(F("GC9A01 Display: SPI pin allocation failed, disabling.")); | ||
| enabled = false; | ||
| return; | ||
| } | ||
|
|
||
| // Register display-specific pins (CS, DC, RST, BL) | ||
| PinManagerPinType dispPins[] = { | ||
| { TFT_CS, true }, { TFT_DC, true }, { TFT_RST, true }, { TFT_BL, true } | ||
| }; | ||
| if (!PinManager::allocateMultiplePins(dispPins, 4, PinOwner::UM_GC9A01Display)) { | ||
| PinManager::deallocateMultiplePins(spiPins, 3, PinOwner::HW_SPI); | ||
| DEBUG_PRINTLN(F("GC9A01 Display: display pin allocation failed, disabling.")); | ||
| enabled = false; | ||
| return; | ||
| } | ||
|
|
||
| tft.init(); | ||
| tft.setRotation(flipDisplay ? 2 : 0); | ||
| tft.fillScreen(TFT_BLACK); | ||
|
|
||
| if (TFT_BL >= 0) { | ||
| pinMode(TFT_BL, OUTPUT); | ||
| setBacklight(true); | ||
| } | ||
|
|
||
| drawSplash(); | ||
| delay(1200); | ||
|
|
||
| lastChange = millis(); | ||
| lastCheck = millis(); | ||
| initDone = true; | ||
| } | ||
|
|
||
| void loop() override { | ||
| if (!enabled || !initDone) return; | ||
|
|
||
| unsigned long now = millis(); | ||
| if (now - lastCheck < GC9A01_REFRESH_MS) return; | ||
| lastCheck = now; | ||
|
|
||
| // Clear overlay once it expires | ||
| if (overlayActive && now >= overlayUntil) { | ||
| overlayActive = false; | ||
| needRedraw = true; | ||
| } | ||
|
|
||
| // Detect state changes | ||
| uint32_t curColor = RGBW32(colPri[0], colPri[1], colPri[2], colPri[3]); | ||
| bool curWifi = WLED_CONNECTED || apActive; | ||
| if (bri != knownBri || | ||
| effectCurrent != knownMode || | ||
| curColor != knownColor || | ||
| curWifi != knownWifi || | ||
| (curWifi && Network.localIP() != knownIp)) { | ||
| needRedraw = true; | ||
| lastChange = now; | ||
| knownBri = bri; | ||
| knownMode = effectCurrent; | ||
| knownColor = curColor; | ||
| knownWifi = curWifi; | ||
| knownIp = curWifi ? Network.localIP() : IPAddress(0,0,0,0); | ||
| } | ||
|
|
||
| // Screen timeout | ||
| unsigned long timeoutMs = (unsigned long)screenTimeoutSec * 1000UL; | ||
| if (!displayOff && (now - lastChange > timeoutMs)) { | ||
| setBacklight(false); | ||
| tft.fillScreen(TFT_BLACK); | ||
| displayOff = true; | ||
| needRedraw = false; | ||
| return; | ||
| } | ||
|
|
||
| if (displayOff || !needRedraw || overlayActive) return; | ||
|
|
||
| needRedraw = false; | ||
| drawWatchFace(); | ||
| } | ||
|
|
||
| // ------------------------------------------------------ config | ||
|
|
||
| void addToConfig(JsonObject &root) override { | ||
| JsonObject top = root.createNestedObject(FPSTR(_name)); | ||
| top[FPSTR(_enabled)] = enabled; | ||
| top[FPSTR(_timeout)] = screenTimeoutSec; | ||
| top[FPSTR(_flip)] = flipDisplay; | ||
| // Expose compile-time pin values so the UI can show them (read-only) | ||
| JsonArray pins = top.createNestedArray("pin"); | ||
| pins.add(TFT_CS); | ||
| pins.add(TFT_DC); | ||
| pins.add(TFT_RST); | ||
| pins.add(TFT_BL); | ||
| } | ||
|
|
||
| void appendConfigData() override { | ||
| // Show pin labels (read-only — pins are compile-time constants) | ||
| oappend(F("addInfo('GC9A01-Display:pin[]',0,'','CS (compile-time)');")); | ||
| oappend(F("addInfo('GC9A01-Display:pin[]',1,'','DC (compile-time)');")); | ||
| oappend(F("addInfo('GC9A01-Display:pin[]',2,'','RST (compile-time)');")); | ||
| oappend(F("addInfo('GC9A01-Display:pin[]',3,'','BL (compile-time)');")); | ||
| } | ||
|
|
||
| bool readFromConfig(JsonObject &root) override { | ||
| JsonObject top = root[FPSTR(_name)]; | ||
| if (top.isNull()) return false; | ||
| bool en = top[FPSTR(_enabled)] | enabled; | ||
| if (en != enabled) { | ||
| enabled = en; | ||
| if (!enabled && TFT_BL >= 0) setBacklight(false); | ||
| } | ||
| screenTimeoutSec = top[FPSTR(_timeout)] | screenTimeoutSec; | ||
| bool newFlip = top[FPSTR(_flip)] | flipDisplay; | ||
| if (newFlip != flipDisplay && initDone) { | ||
| flipDisplay = newFlip; | ||
| tft.setRotation(flipDisplay ? 2 : 0); | ||
| needRedraw = true; | ||
| } else { | ||
| flipDisplay = newFlip; | ||
| } | ||
| return !top[FPSTR(_timeout)].isNull(); | ||
| } | ||
|
|
||
| void addToJsonInfo(JsonObject &root) override { | ||
| JsonObject user = root["u"]; | ||
| if (user.isNull()) user = root.createNestedObject("u"); | ||
| JsonArray arr = user.createNestedArray(F("GC9A01")); | ||
| arr.add(enabled ? F("active") : F("disabled")); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| const char GC9A01DisplayUsermod::_name[] PROGMEM = "GC9A01-Display"; | ||
| const char GC9A01DisplayUsermod::_enabled[] PROGMEM = "enabled"; | ||
| const char GC9A01DisplayUsermod::_timeout[] PROGMEM = "screenTimeoutSec"; | ||
| const char GC9A01DisplayUsermod::_flip[] PROGMEM = "flip"; | ||
|
|
||
|
|
||
| static GC9A01DisplayUsermod gc9a01_display; | ||
| REGISTER_USERMOD(gc9a01_display); | ||
|
|
||
| #endif // USERMOD_GC9A01_DISPLAY |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add required AI block markers around this generated usermod implementation.
This is a large contiguous AI-generated C++ block but it is not wrapped with the required // AI: below section was generated by an AI / // AI: end markers.
As per coding guidelines, “Embed larger blocks of AI-generated code within // AI: below section was generated by an AI ... // AI: end comments.”
🤖 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/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around
lines 1 - 459, The file contains a large AI-generated block (the
GC9A01DisplayUsermod class and related globals like TFT_eSPI tft and static
GC9A01DisplayUsermod gc9a01_display) that must be wrapped with the required AI
markers; add a comment line "// AI: below section was generated by an AI"
immediately before the start of the generated block (before the TFT_eSPI tft =
... line or before the class definition) and add "// AI: end" immediately after
the end of that block (after REGISTER_USERMOD(gc9a01_display);), ensuring both
markers are plain single-line comments and do not alter existing code or
spacing.
Comment on lines
+383
to
+385
| unsigned long timeoutMs = (unsigned long)screenTimeoutSec * 1000UL; | ||
| if (!displayOff && (now - lastChange > timeoutMs)) { | ||
| setBacklight(false); |
Contributor
There was a problem hiding this comment.
screenTimeoutSec=0 currently forces immediate blanking.
Line 383 makes timeout 0ms, so Line 384 will shut the display off almost instantly. That contradicts the documented “0 = never”.
Suggested change
// Screen timeout
unsigned long timeoutMs = (unsigned long)screenTimeoutSec * 1000UL;
- if (!displayOff && (now - lastChange > timeoutMs)) {
+ if (!displayOff && screenTimeoutSec > 0 && (now - lastChange > timeoutMs)) {
setBacklight(false);
tft.fillScreen(TFT_BLACK);
displayOff = true;🤖 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/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around
lines 383 - 385, The code treats screenTimeoutSec==0 as a 0ms timeout and blanks
immediately; modify the logic around screenTimeoutSec/timeoutMs so a value of 0
means "never" — e.g. only compute timeoutMs and perform the blanking check when
screenTimeoutSec > 0 (or explicitly skip the (now - lastChange > timeoutMs)
check when timeoutMs == 0); update the block referencing screenTimeoutSec,
timeoutMs, displayOff, lastChange, now and setBacklight to ensure no blanking
occurs for screenTimeoutSec==0.
Comment on lines
+157
to
+173
| void sortModesAndPalettes() { | ||
| modes_qstrings = strip.getModeDataSrc(); | ||
| modes_alpha_indexes = re_initIndexArray(strip.getModeCount()); | ||
| re_sortModes(modes_qstrings, modes_alpha_indexes, strip.getModeCount(), I2CENC_MODE_SORT_SKIP_COUNT); | ||
|
|
||
| palettes_qstrings = re_findModeStrings(JSON_palette_names, getPaletteCount()); | ||
| palettes_alpha_indexes = re_initIndexArray(getPaletteCount()); | ||
| if (customPalettes.size()) { | ||
| for (int i = 0; i < (int)customPalettes.size(); i++) { | ||
| palettes_alpha_indexes[FIXED_PALETTE_COUNT + i] = 255 - i; | ||
| palettes_qstrings[FIXED_PALETTE_COUNT + i] = PSTR("~Custom~"); | ||
| } | ||
| } | ||
| int skipPaletteCount = 1; | ||
| while (pgm_read_byte_near(palettes_qstrings[skipPaletteCount]) == '*') skipPaletteCount++; | ||
| re_sortModes(palettes_qstrings, palettes_alpha_indexes, FIXED_PALETTE_COUNT, skipPaletteCount); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard allocation failures before writing/sorting arrays.
malloc() results are dereferenced immediately in the helper functions. A failed allocation can crash the device before setup completes.
Proposed fix
void sortModesAndPalettes() {
modes_qstrings = strip.getModeDataSrc();
modes_alpha_indexes = re_initIndexArray(strip.getModeCount());
+ if (!modes_alpha_indexes) return;
re_sortModes(modes_qstrings, modes_alpha_indexes, strip.getModeCount(), I2CENC_MODE_SORT_SKIP_COUNT);
palettes_qstrings = re_findModeStrings(JSON_palette_names, getPaletteCount());
palettes_alpha_indexes = re_initIndexArray(getPaletteCount());
+ if (!palettes_qstrings || !palettes_alpha_indexes) return;
if (customPalettes.size()) {
@@
byte *re_initIndexArray(int numModes) {
byte *indexes = (byte *)malloc(sizeof(byte) * numModes);
+ if (!indexes) return nullptr;
for (unsigned i = 0; i < (unsigned)numModes; i++) indexes[i] = i;
return indexes;
}
const char **re_findModeStrings(const char json[], int numModes) {
const char **modeStrings = (const char **)malloc(sizeof(const char *) * numModes);
+ if (!modeStrings) return nullptr;
uint8_t modeIndex = 0;Also applies to: 175-183
🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` around
lines 157 - 173, The code in sortModesAndPalettes (and the nearby block at
175-183) uses allocations returned by re_initIndexArray and re_findModeStrings
and then dereferences them immediately; modify sortModesAndPalettes to check
each pointer returned (modes_qstrings, modes_alpha_indexes, palettes_qstrings,
palettes_alpha_indexes) for null before using them, and if any allocation fails,
perform a safe failure path (log an error via existing logger, free any
previously allocated arrays if needed, and abort/return from setup to avoid
dereferencing null). Ensure you also guard the customPalettes loop and the
pgm_read_byte_near access by confirming palettes_qstrings is valid and that
indexes are within bounds before writing. Use the helper names
re_initIndexArray, re_findModeStrings, and re_sortModes to locate changes.
Comment on lines
+344
to
+345
| case 8: if (presetHigh == 0 || presetLow == 0) newState++; else { strcpy_P(lineBuffer, PSTR("Preset")); changedState = true; } break; | ||
| case 9: |
Contributor
There was a problem hiding this comment.
Make preset-state eligibility consistent with preset execution.
The menu enters “Preset” whenever both bounds are non-zero, but changePreset() ignores it unless presetHigh > presetLow. That creates a selectable state with no effect.
Proposed fix
- case 8: if (presetHigh == 0 || presetLow == 0) newState++; else { strcpy_P(lineBuffer, PSTR("Preset")); changedState = true; } break;
+ case 8: if (presetHigh == 0 || presetLow == 0 || presetHigh < presetLow) newState++; else { strcpy_P(lineBuffer, PSTR("Preset")); changedState = true; } break;
@@
- if (presetHigh && presetLow && presetHigh > presetLow) {
+ if (presetHigh && presetLow && presetHigh >= presetLow) {Also applies to: 635-648
🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` around
lines 344 - 345, The menu allows entering "Preset" when both presetHigh and
presetLow are non-zero but changePreset() only acts when presetHigh > presetLow,
causing a no-op selectable item; update the eligibility check in the menu (the
case that writes "Preset" into lineBuffer and sets changedState/newState) to
require presetHigh > presetLow instead of merely non-zero, and make the same
analogous change for the duplicate block around the changePreset() call (lines
handling the same preset menu logic) so the menu selection and changePreset()
use the same condition.
Comment on lines
+651
to
+662
| void changeCCT(bool increase) { | ||
| #ifdef USERMOD_FOUR_LINE_DISPLAY | ||
| if (display && display->wakeDisplay()) { display->redraw(true); return; } | ||
| if (display) display->updateRedrawTime(); | ||
| #endif | ||
| currentCCT = max(min((increase ? currentCCT + fadeAmount : currentCCT - fadeAmount), 255), 0); | ||
| for (unsigned i = 0; i < strip.getSegmentsNum(); i++) { | ||
| Segment &seg = strip.getSegment(i); | ||
| if (!seg.isActive()) continue; | ||
| seg.setCCT(currentCCT); | ||
| } | ||
| lampUdated(); |
Contributor
There was a problem hiding this comment.
Respect applyToAll when changing CCT.
changeCCT() currently updates all active segments even when applyToAll is false, unlike the other parameter handlers.
Proposed fix
void changeCCT(bool increase) {
@@
currentCCT = max(min((increase ? currentCCT + fadeAmount : currentCCT - fadeAmount), 255), 0);
- for (unsigned i = 0; i < strip.getSegmentsNum(); i++) {
- Segment &seg = strip.getSegment(i);
- if (!seg.isActive()) continue;
- seg.setCCT(currentCCT);
- }
+ if (applyToAll) {
+ for (unsigned i = 0; i < strip.getSegmentsNum(); i++) {
+ Segment &seg = strip.getSegment(i);
+ if (!seg.isActive()) continue;
+ seg.setCCT(currentCCT);
+ }
+ } else {
+ strip.getMainSegment().setCCT(currentCCT);
+ }
lampUdated();🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` around
lines 651 - 662, changeCCT() always updates every active segment; change it to
respect applyToAll like the other parameter handlers: after computing
currentCCT, if applyToAll is true keep the existing loop over
strip.getSegmentsNum() calling seg.setCCT(currentCCT), otherwise only update the
currently selected segment(s) (the same selected-segment index/pattern used by
the other handlers) — check and use the same symbol those handlers use to find
the selected segment (e.g., currentSegmentIndex / getSelectedSegmentIndex or
equivalent) and call seg.setCCT(currentCCT) only for that segment (keeping the
seg.isActive() guard), then call lampUdated().
Comment on lines
+695
to
+708
| uint8_t newAddr = top[FPSTR(_i2cAddress)] | i2cAddress; | ||
|
|
||
| presetHigh = top[FPSTR(_presetHigh)] | presetHigh; | ||
| presetLow = top[FPSTR(_presetLow)] | presetLow; | ||
| presetHigh = MIN(250, MAX(0, presetHigh)); | ||
| presetLow = MIN(250, MAX(0, presetLow)); | ||
| enabled = top[FPSTR(_enabled)] | enabled; | ||
| applyToAll = top[FPSTR(_applyToAll)] | applyToAll; | ||
|
|
||
| if (!initDone) { | ||
| i2cAddress = newAddr; | ||
| } else if (i2cAddress != newAddr) { | ||
| i2cAddress = newAddr; | ||
| deviceFound = false; |
Contributor
There was a problem hiding this comment.
Validate configured I2C address before reinitialization.
readFromConfig() accepts any byte for i2c-address. Invalid or reserved addresses can put the usermod into a non-working state.
Proposed fix
- uint8_t newAddr = top[FPSTR(_i2cAddress)] | i2cAddress;
+ uint8_t newAddr = top[FPSTR(_i2cAddress)] | i2cAddress;
+ newAddr = MIN((uint8_t)0x77, MAX((uint8_t)0x08, newAddr));Based on learnings: readFromConfig/getJsonValue are untrusted ingress boundaries and field values must be treated as client-supplied/unvalidated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp (1)
1-727:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing required AI-generated code block markers.
This file appears to be a large AI-generated C++ block, but it is not wrapped with the required markers.
Proposed minimal fix
+// AI: below section was generated by an AI `#include` "wled.h" ... REGISTER_USERMOD(usermod_v2_i2c_encoder_ui); +// AI: endAs per coding guidelines: “Embed larger blocks of AI-generated code within
// AI: below section was generated by an AI...// AI: endcomments”.🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` around lines 1 - 727, The file lacks the required AI-generated code markers; wrap the large AI-generated C++ block that defines the I2CEncoderUIUsermod (the class I2CEncoderUIUsermod and its helper functions like readEncoderDelta, readButtonState, sortModesAndPalettes, changeEffect, etc.) with the mandated comments: add a line containing "// AI: below section was generated by an AI" immediately before the AI-generated block (e.g., just after the initial includes/headers) and add "// AI: end" immediately after the end of that block (after the class definition/related globals), ensuring the markers enclose the entire generated section.
♻️ Duplicate comments (3)
usermods/usermod_v2_gc9a01_display/readme.md (1)
8-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to the ASCII-art code fence.
Line 8 should use a typed fence to satisfy MD040.
Suggested fix
-``` +```text ╭──────────────────╮ ╱ │ ░░░░░░░░░░░░░░ │ ╲ ← brightness arc (yellow, 0–360°) ... ╰──────────────────╯</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/usermod_v2_gc9a01_display/readme.mdaround lines 8 - 19, The fenced
ASCII-art block uses an untyped code fence which triggers MD040; update the
opening fence (the triple backticks before the ASCII art in the README) to a
typed fence such as ```text so the block is explicitly marked as text and
satisfies MD040, leaving the art and closing fence unchanged.</details> </blockquote></details> <details> <summary>usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (2)</summary><blockquote> `382-385`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Fix `screenTimeoutSec=0` handling to mean “never timeout.”** Line 383 computes `timeoutMs=0`, so Line 384 blanks the display almost immediately. Guard the timeout check with `screenTimeoutSec > 0`. <details> <summary>Suggested fix</summary> ```diff // Screen timeout unsigned long timeoutMs = (unsigned long)screenTimeoutSec * 1000UL; - if (!displayOff && (now - lastChange > timeoutMs)) { + if (!displayOff && screenTimeoutSec > 0 && (now - lastChange > timeoutMs)) { setBacklight(false); tft.fillScreen(TFT_BLACK); displayOff = true; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around lines 382 - 385, The timeout calculation treats screenTimeoutSec==0 as zero milliseconds which immediately triggers setBacklight(false); update the timeout check to only apply when screenTimeoutSec > 0 by changing the if to require screenTimeoutSec > 0 in addition to the existing condition (use the existing variables timeoutMs, displayOff, now, lastChange and call setBacklight(false) only when screenTimeoutSec > 0 and (now - lastChange > timeoutMs)). ``` </details> --- `1-459`: _🛠️ Refactor suggestion_ | _🟠 Major_ | _⚡ Quick win_ **Wrap this contiguous AI-generated C++ block with required AI markers.** This file is a large generated block and should be enclosed with the mandated `// AI: below section was generated by an AI` and `// AI: end` comments. As per coding guidelines, “AI review requirement: if a contiguous block was AI-generated, wrap it with `// AI: below section was generated by an AI` ... `// AI: end` (single-line AI edits don’t need the marker).” <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around lines 1 - 459, The contiguous AI-generated C++ block (from the file-scope TFT object "TFT_eSPI tft = TFT_eSPI(TFT_WIDTH, TFT_HEIGHT);" through the end of the REGISTER_USERMOD(gc9a01_display);) must be wrapped with the mandated single-line markers exactly: add a line "// AI: below section was generated by an AI" immediately before the start of that block and a line "// AI: end" immediately after the final line (after REGISTER_USERMOD(gc9a01_display);). Ensure the markers are plain comments on their own lines and do not alter any other code or spacing inside the GC9A01DisplayUsermod class, its methods (e.g., drawWatchFace, loop, setup) or the tft declaration. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp:
- Line 559: The current single-line update to effectPaletteIndex casts the
decrement to unsigned and causes underflow when effectPaletteIndex is 0; change
the computation to use a signed integer before clamping: compute a signed
newIndex = effectPaletteIndex + (increase ? 1 : -1), compute maxIndex =
getPaletteCount() + customPalettes.size() - 1, then clamp newIndex to the range
[0, maxIndex] and assign back to effectPaletteIndex; reference
effectPaletteIndex, increase, getPaletteCount(), and customPalettes.size() when
making the fix.- Line 582: The expression assigning currentHue1 does unsigned arithmetic
causing underflow when decrementing (currentHue1 is unsigned); change the update
to perform the subtraction in a signed context or guard the decrement: compute a
signed/temporary value or check if currentHue1 < fadeAmount and set 0, otherwise
subtract fadeAmount, then clamp to 0..255; update the assignment using the
currentHue1 variable and the increase/fadeAmount logic so the decrement path
cannot wrap to 255.
Outside diff comments:
In@usermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp:
- Around line 1-727: The file lacks the required AI-generated code markers; wrap
the large AI-generated C++ block that defines the I2CEncoderUIUsermod (the class
I2CEncoderUIUsermod and its helper functions like readEncoderDelta,
readButtonState, sortModesAndPalettes, changeEffect, etc.) with the mandated
comments: add a line containing "// AI: below section was generated by an AI"
immediately before the AI-generated block (e.g., just after the initial
includes/headers) and add "// AI: end" immediately after the end of that block
(after the class definition/related globals), ensuring the markers enclose the
entire generated section.
Duplicate comments:
In@usermods/usermod_v2_gc9a01_display/readme.md:
- Around line 8-19: The fenced ASCII-art block uses an untyped code fence which
triggers MD040; update the opening fence (the triple backticks before the ASCII
art in the README) to a typed fence such as ```text so the block is explicitly
marked as text and satisfies MD040, leaving the art and closing fence unchanged.In
@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp:
- Around line 382-385: The timeout calculation treats screenTimeoutSec==0 as
zero milliseconds which immediately triggers setBacklight(false); update the
timeout check to only apply when screenTimeoutSec > 0 by changing the if to
require screenTimeoutSec > 0 in addition to the existing condition (use the
existing variables timeoutMs, displayOff, now, lastChange and call
setBacklight(false) only when screenTimeoutSec > 0 and (now - lastChange >
timeoutMs)).- Around line 1-459: The contiguous AI-generated C++ block (from the file-scope
TFT object "TFT_eSPI tft = TFT_eSPI(TFT_WIDTH, TFT_HEIGHT);" through the end of
the REGISTER_USERMOD(gc9a01_display);) must be wrapped with the mandated
single-line markers exactly: add a line "// AI: below section was generated by
an AI" immediately before the start of that block and a line "// AI: end"
immediately after the final line (after REGISTER_USERMOD(gc9a01_display);).
Ensure the markers are plain comments on their own lines and do not alter any
other code or spacing inside the GC9A01DisplayUsermod class, its methods (e.g.,
drawWatchFace, loop, setup) or the tft declaration.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `9497ec5c-bcbd-4d91-ad9f-81566b2933ea` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 68b61f0831fded6b81198826b1d9a28374370c4f and cc456670971b1f4fc164ef51f781dd5c5852a477. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `usermods/usermod_v2_gc9a01_display/library.json` * `usermods/usermod_v2_gc9a01_display/platformio_override.sample.ini` * `usermods/usermod_v2_gc9a01_display/readme.md` * `usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` * `usermods/usermod_v2_i2c_encoder_ui/readme.md` * `usermods/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` * `wled00/const.h` * `wled00/pin_manager.h` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if (display && display->wakeDisplay()) { display->redraw(true); return; } | ||
| if (display) display->updateRedrawTime(); | ||
| #endif | ||
| effectPaletteIndex = max(min((unsigned)(increase ? effectPaletteIndex + 1 : effectPaletteIndex - 1), getPaletteCount() + customPalettes.size() - 1), 0U); |
Contributor
There was a problem hiding this comment.
Palette decrement wraps to max index at lower bound.
Line 559 casts the decrement path to unsigned, so decreasing at index 0 underflows and jumps to the last palette instead of clamping.
Proposed fix
- effectPaletteIndex = max(min((unsigned)(increase ? effectPaletteIndex + 1 : effectPaletteIndex - 1), getPaletteCount() + customPalettes.size() - 1), 0U);
+ int next = increase ? (int)effectPaletteIndex + 1 : (int)effectPaletteIndex - 1;
+ effectPaletteIndex = (uint8_t)max(min(next, (int)(getPaletteCount() + customPalettes.size() - 1)), 0);🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` at line
559, The current single-line update to effectPaletteIndex casts the decrement to
unsigned and causes underflow when effectPaletteIndex is 0; change the
computation to use a signed integer before clamping: compute a signed newIndex =
effectPaletteIndex + (increase ? 1 : -1), compute maxIndex = getPaletteCount() +
customPalettes.size() - 1, then clamp newIndex to the range [0, maxIndex] and
assign back to effectPaletteIndex; reference effectPaletteIndex, increase,
getPaletteCount(), and customPalettes.size() when making the fix.
| if (display && display->wakeDisplay()) { display->redraw(true); return; } | ||
| if (display) display->updateRedrawTime(); | ||
| #endif | ||
| currentHue1 = max(min((increase ? currentHue1 + fadeAmount : currentHue1 - fadeAmount), 255), 0); |
Contributor
There was a problem hiding this comment.
Hue decrement has unsigned underflow behavior.
currentHue1 is unsigned; when it is 0, decreasing causes wrap before clamp, effectively jumping to 255.
Proposed fix
- currentHue1 = max(min((increase ? currentHue1 + fadeAmount : currentHue1 - fadeAmount), 255), 0);
+ int nextHue = increase ? (int)currentHue1 + fadeAmount : (int)currentHue1 - fadeAmount;
+ currentHue1 = (uint16_t)max(min(nextHue, 255), 0);🤖 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/usermod_v2_i2c_encoder_ui/usermod_v2_i2c_encoder_ui.cpp` at line
582, The expression assigning currentHue1 does unsigned arithmetic causing
underflow when decrementing (currentHue1 is unsigned); change the update to
perform the subtraction in a signed context or guard the decrement: compute a
signed/temporary value or check if currentHue1 < fadeAmount and set 0, otherwise
subtract fadeAmount, then clamp to 0..255; update the assignment using the
currentHue1 variable and the increase/fadeAmount logic so the decrement path
cannot wrap to 255.
softhack007
reviewed
May 26, 2026
Member
There was a problem hiding this comment.
@coderabbitai i have the feeling that this file is largely copy & paste of https://github.com/wled/WLED/blob/main/usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp 🤔
Please compare the new code to the (assumed) baseline usermod. Check if both could be merged into one, by just making the I2C code an additional driver option for reading out the rotary. If possible, suggest how to merge the usermod_v2_i2c_encoder_ui code with usermod_v2_rotary_encoder_ui_ALT.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the baseline rotary encoder usermod
cat usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cppLength of output: 40254
@softhack007 Your suspicion is correct — the new file is ~95% copy-paste from usermod_v2_rotary_encoder_ui_ALT.cpp. Here is the full breakdown.
Overlap analysis
| Section | Identical? | Notes |
|---|---|---|
#ifdef USERMOD_MODE_SORT error |
✅ identical | |
LAST_UI_STATE / I2CENC_LAST_UI_STATE |
✅ identical | same values 11/4 |
| qsort comparator | ✅ identical | only symbol names changed |
| All private state fields | ✅ identical | fadeAmount, loopTime, button*, select_state, currentHue1/Sat1/CCT, display, mode/palette arrays, presetHigh/Low, applyToAll, initDone, enabled |
sortModesAndPalettes() |
✅ identical | |
re_initIndexArray() |
✅ identical | |
re_findModeStrings() |
✅ identical | minor style only |
re_sortModes() |
✅ identical | |
| Constructor | ✅ identical defaults | |
All 10 change*() methods |
✅ identical | changeBrightness, changeEffect, changeEffectSpeed, changeEffectIntensity, changeCustom, changePalette, changeHue, changeSat, changePreset, changeCCT |
displayNetworkInfo() |
✅ identical | |
findCurrentEffectAndPalette() |
✅ identical | |
changeState() |
✅ identical | |
lampUdated() |
✅ identical | |
State-machine do{switch(newState)…} in loop() |
✅ identical | including all changeState() arguments |
addToConfig() / readFromConfig() |
🔶 same structure | I2C version replaces pin fields with i2cAddress |
Novel I2C-only code (~55 lines):
writeReg()/readEncoderDelta()/readButtonState()helperssetup()I2C init branch (checki2c_sda/scl, write reg0x00, read baseline count)loop()delta-based polling instead of GPIO edge detectioni2cAddressconfig field +deviceFoundflag
Why a merge makes sense
The ALT usermod already has a PCF8574 I2C-expander option (with ISR). The M5Stack UNIT-ENCODER is a second I2C backend — a natural addition. The state machine, all change*() methods, sorting, and FourLineDisplay integration are 100% shared and should not be duplicated.
Suggested merge approach
Add an #ifdef USERMOD_I2C_ENCODER driver block inside the existing usermod_v2_rotary_encoder_ui_ALT:
// ---------------------------------------------------------------------------
// In usermod_v2_rotary_encoder_ui_ALT.cpp
// ---------------------------------------------------------------------------
// ── new compile-time guard ──────────────────────────────────────────────────
`#ifdef` USERMOD_I2C_ENCODER
`#ifndef` I2C_ENCODER_ADDR
`#define` I2C_ENCODER_ADDR 0x40
`#endif`
`#ifndef` I2C_ENCODER_POLL_MS
`#define` I2C_ENCODER_POLL_MS 10
`#endif`
`#endif`
// ── new private fields (inside the class, alongside existing ones) ──────────
`#ifdef` USERMOD_I2C_ENCODER
uint8_t i2cEncoderAddr = I2C_ENCODER_ADDR;
uint16_t i2cLastRaw = 0;
bool i2cDeviceFound = false;
`#endif`
// ── new private helpers ─────────────────────────────────────────────────────
`#ifdef` USERMOD_I2C_ENCODER
bool i2cWriteReg(uint8_t reg, uint8_t value) {
Wire.beginTransmission(i2cEncoderAddr);
Wire.write(reg); Wire.write(value);
return Wire.endTransmission() == 0;
}
// Returns signed encoder delta since last call; 0 on I2C error.
int16_t i2cReadDelta() {
Wire.beginTransmission(i2cEncoderAddr);
Wire.write(0x10);
if (Wire.endTransmission(false) != 0) return 0;
if (Wire.requestFrom((uint8_t)i2cEncoderAddr, (uint8_t)2) < 2) return 0;
uint16_t raw = (uint16_t)Wire.read() | ((uint16_t)Wire.read() << 8);
int16_t delta = (int16_t)(raw - i2cLastRaw);
i2cLastRaw = raw;
return delta;
}
// Returns true when the M5Stack encoder button is pressed.
bool i2cReadButton() {
Wire.beginTransmission(i2cEncoderAddr);
Wire.write(0x20);
if (Wire.endTransmission(false) != 0) return false;
if (Wire.requestFrom((uint8_t)i2cEncoderAddr, (uint8_t)1) < 1) return false;
return Wire.read() == 1;
}
`#endif`setup() — add an I2C encoder branch alongside the existing PCF8574/GPIO branches:
void RotaryEncoderUIUsermod::setup() {
// ... existing GPIO / PCF8574 init ...
`#ifdef` USERMOD_I2C_ENCODER
if (i2c_sda < 0 || i2c_scl < 0) {
DEBUG_PRINTLN(F("RE-UI: I2C pins not configured, disabling."));
enabled = false; return;
}
if (!i2cWriteReg(0x00, 0x00)) { // set Pulse mode
DEBUG_PRINTLN(F("RE-UI: I2C encoder not found."));
i2cDeviceFound = false;
// keep enabled so address can be corrected without reboot
} else {
i2cDeviceFound = true;
// read baseline so first delta == 0
Wire.beginTransmission(i2cEncoderAddr); Wire.write(0x10);
Wire.endTransmission(false);
if (Wire.requestFrom((uint8_t)i2cEncoderAddr, (uint8_t)2) >= 2)
i2cLastRaw = (uint16_t)Wire.read() | ((uint16_t)Wire.read() << 8);
}
`#endif`
// ... rest of setup (CCT init, sortModesAndPalettes, FourLineDisplay lookup) ...
}loop() — replace the GPIO edge-detection block with a unified poll block:
void RotaryEncoderUIUsermod::loop() {
if (!enabled) return;
unsigned long currentTime = millis();
`#ifdef` USERMOD_I2C_ENCODER
// I2C path: poll at I2C_ENCODER_POLL_MS; bail if device not found
if (!i2cDeviceFound) return;
if (strip.isUpdating() && (currentTime - loopTime) < I2C_ENCODER_POLL_MS) return;
if (currentTime - loopTime < I2C_ENCODER_POLL_MS) return;
`#else`
// GPIO path: existing 2 ms guard
if (strip.isUpdating() && (currentTime - loopTime) < ENCODER_MAX_DELAY_MS) return;
if (currentTime < loopTime + 2) return;
`#endif`
// ── shared: effect/palette tracking, button, state machine ────────────────
if (!currentEffectAndPaletteInitialized) findCurrentEffectAndPalette();
// ... existing mode/palette change detection ...
`#ifdef` USERMOD_I2C_ENCODER
bool buttonPressed = i2cReadButton();
`#else`
bool buttonPressed = !readPin(pinC);
`#endif`
// ... identical button debounce / state machine code (unchanged) ...
// ── encoder reading ────────────────────────────────────────────────────────
`#ifdef` USERMOD_I2C_ENCODER
int16_t delta = i2cReadDelta();
if (delta != 0) {
bool increase = delta > 0;
switch (select_state) {
case 0: changeBrightness(increase); break;
case 1: changeEffectSpeed(increase); break;
// ... identical switch body ...
}
}
`#else`
Enc_A = readPin(pinA); Enc_B = readPin(pinB);
if (Enc_A && !Enc_A_prev) {
// ... existing clockwise / counter-clockwise switch ...
}
Enc_A_prev = Enc_A;
`#endif`
loopTime = currentTime;
}addToConfig() / readFromConfig() — guard the I2C address field:
void RotaryEncoderUIUsermod::addToConfig(JsonObject &root) {
JsonObject top = root.createNestedObject(FPSTR(_name));
top[FPSTR(_enabled)] = enabled;
`#ifdef` USERMOD_I2C_ENCODER
top[FPSTR(_i2cAddr)] = i2cEncoderAddr;
`#else`
top[FPSTR(_DT_pin)] = pinA;
top[FPSTR(_CLK_pin)] = pinB;
top[FPSTR(_SW_pin)] = pinC;
top[FPSTR(_pcf8574)] = usePcf8574;
top[FPSTR(_pcfAddress)] = addrPcf8574;
top[FPSTR(_pcfINTpin)] = pinIRQ;
`#endif`
top[FPSTR(_presetLow)] = presetLow;
top[FPSTR(_presetHigh)] = presetHigh;
top[FPSTR(_applyToAll)] = applyToAll;
}Summary
| New I2C file | Merged ALT file | |
|---|---|---|
| Files to maintain | 2 | 1 |
| Novel I2C code | ~55 lines | ~55 lines (#ifdef guarded) |
| Duplicated logic eliminated | — | ~670 lines |
| Existing PCF8574 support | ❌ removed | ✅ retained |
| Build flag | USERMOD_I2C_ENCODER |
(new) |
The new usermod_v2_i2c_encoder_ui directory can then be removed, and the README merged into the ALT usermod's readme under an "I2C encoder (M5Stack UNIT-ENCODER)" section.
🐰 (\(\) ) ─ sniff sniff... smells like a copy-paste~
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 4843
File: usermods/elastic_collisions/Elastic_Collisions.cpp:1-1
Timestamp: 2026-05-06T22:10:27.517Z
Learning: In C/C++, do not flag precedence issues for expressions that mix additive operators (`+`, `-`) with shift operators (`<<`, `>>`) based on the assumption that shifts bind tighter. Per C/C++ operator precedence, `+`/`-` have higher precedence than `<<`/`>>` (e.g., `x - edge0 << 8` parses as `(x - edge0) << 8`). When reviewing WLED (and other) fixed-point/bit-manipulation code, confirm the intended parse using cppreference before reporting a precedence bug for mixed `-`/`+` and `<<`/`>>` expressions.
Learnt from: softhack007
Repo: wled/WLED PR: 5599
File: usermods/audioreactive/audio_reactive.cpp:1303-1308
Timestamp: 2026-05-11T16:00:13.574Z
Learning: When targeting arduino-esp32 v3.x (ESP-IDF 5.x), treat `WiFiUDP::flush()` as deprecated and ineffective for RX draining; it should not be used to clear/drain the UDP receive buffer. Instead, use `WiFiUDP::clear()`, which was introduced in arduino-esp32 v3.0 and is marked as the deprecated replacement via `NetworkUdp.h`.
If the code must compile and behave correctly across both arduino-esp32 v2.x (ESP-IDF < 5) and v3.x (ESP-IDF >= 5), use conditional compilation:
- for `ESP_IDF_VERSION_MAJOR < 5`: use `udp.flush()`
- for `ESP_IDF_VERSION_MAJOR >= 5`: use `udp.clear()`
Do not flag `WiFiUDP::clear()` usages for ESP-IDF >= 5 as non-existent—this is the documented correct replacement.
Learnt from: softhack007
Repo: wled/WLED PR: 5572
File: .coderabbit.yaml:116-116
Timestamp: 2026-05-24T11:50:21.052Z
Learning: In WLED usermods, treat these callback entry points as untrusted ingress boundaries because the WLED core does not sanitize payload content before invoking them:
- onMqttMessage(char* topic, char* payload): core only reassembles chunked MQTT payload into a null-terminated char*; no content/length validation.
- onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len): raw received bytes and len are passed directly from the ESP-NOW handler.
- onUdpPacket(uint8_t* payload, size_t len): raw udpIn buffer and packetSize are passed through unchanged after WLED’s own sync handling.
- readFromJsonState(JsonObject& obj): ArduinoJson parses structure, but field values are still client-supplied and must be treated as unvalidated.
Also list readFromConfig/getJsonValue as untrusted ingress boundaries for usermods. Ensure these are represented in the .coderabbit.yaml trust-boundary model.
Member
There was a problem hiding this comment.
🐰 (() ) ─ sniff sniff... smells like a copy-paste~
😆 agreed.
softhack007
reviewed
May 26, 2026
Member
There was a problem hiding this comment.
@coderabbitai is there an existing usermod that could be the baseline for this code? If yes, would it be possible to merge both into one?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all display-related usermods
echo "=== Display-related usermod directories ==="
fd -t d . usermods/ | grep -iE 'display|tft|oled|lcd|screen|ssd|ili|st77|gc9' | sort
echo ""
echo "=== All usermod directories ==="
ls usermods/
echo ""
echo "=== TFT_eSPI usage in usermods ==="
rg -rl 'TFT_eSPI' usermods/
echo ""
echo "=== Usermods with wakeDisplay/showOverlay/similar API ==="
rg -rn 'wakeDisplay\|showOverlay\|forceRedraw\|FourLineDisplay\|four_line_display' usermods/ --type=cpp -l
echo ""
echo "=== FourLineDisplay usermod directory contents ==="
fd . usermods/ -e cpp -e h | grep -i 'four\|line_display\|display_v2\|display_v3'
echo ""
echo "=== Check for any existing round/circular display usermods ==="
rg -rn 'fillCircle\|drawArc\|round.*display\|circular' usermods/ --type=cpp -lLength of output: 7448
🏁 Script executed:
#!/bin/bash
echo "=== ST7789_display.cpp ==="
cat usermods/ST7789_display/ST7789_display.cpp
echo ""
echo "=== pixels_dice_tray - SPI/pin setup pattern ==="
rg -n 'setSPIPins\|spi_mosi\|spi_sclk\|PinManager\|allocate\|deallocate\|HW_SPI\|UM_PIXELS' usermods/pixels_dice_tray/pixels_dice_tray.cpp | head -60
echo ""
echo "=== pixels_dice_tray library.json ==="
cat usermods/pixels_dice_tray/library.json
echo ""
echo "=== FourLineDisplay ALT - public API methods ==="
rg -n 'void wake\|void show\|void force\|wakeDisplay\|showOverlay\|forceRedraw\|bool wake\|overlay\|public:' usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h | head -40Length of output: 15859
@softhack007 — yes, the closest existing baseline is usermods/ST7789_display. The two usermods share a lot of structural DNA:
| Aspect | ST7789 | GC9A01 (this PR) |
|---|---|---|
| Driver library | TFT_eSPI (Bodmer) | TFT_eSPI (Bodmer) |
Compile-time pins + USER_SETUP_LOADED guard |
✅ | ✅ |
PinManager for SPI + display pins |
✅ (with a bug, see below) | ✅ |
| State-change detection loop | ✅ | ✅ |
| Screen timeout (~5 min) | ✅ hardcoded | ✅ configurable |
addToConfig pin array + appendConfigData labels |
✅ | ✅ |
wakeDisplay() / showOverlay() / forceRedraw() |
❌ | ✅ |
flipDisplay / configurable timeout |
❌ | ✅ |
| Round geometry (arcs, filled circles) | ❌ | ✅ |
Where a merge gets complicated:
The display geometries are fundamentally different — ST7789 is rectangular (grid layout, cursor-based), GC9A01 is round (arc + filled-circle watch-face). The drawing code cannot be shared meaningfully. A realistic merge would require a common base class carrying shared logic (pin allocation, state tracking, timeout, addToConfig structure) with driver-specific rendering subclasses. That is a reasonable refactoring goal but significantly more work than this PR currently represents.
Side note — existing ST7789 bugs this PR has already solved correctly:
ST7789_displayregisters display pins underPinOwner::UM_FourLineDisplay— clearly a copy-paste error. This PR correctly introduces and usesPinOwner::UM_GC9A01Display.ST7789_display::readFromConfig()returnstruewithout actually reading any persistent values — it is effectively a stub. The GC9A01 version readsenabled,screenTimeoutSec, andflipcorrectly.USERMOD_ID_ST7789_DISPLAY = 97is defined locally in the.cppfile rather than inconst.h, inconsistent with WLED conventions. This PR correctly adds its IDs toconst.h.
Recommendation: A merge/refactor into a shared TFT-display base is a sensible long-term goal, but it's out of scope for this PR. If you'd like, I can open a follow-up issue to track "unify ST7789 and GC9A01 display usermods under a common TFT base class + fix ST7789 PinOwner bug". Would that be useful?
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 4843
File: usermods/elastic_collisions/Elastic_Collisions.cpp:1-1
Timestamp: 2026-05-06T22:10:27.517Z
Learning: In C/C++, do not flag precedence issues for expressions that mix additive operators (`+`, `-`) with shift operators (`<<`, `>>`) based on the assumption that shifts bind tighter. Per C/C++ operator precedence, `+`/`-` have higher precedence than `<<`/`>>` (e.g., `x - edge0 << 8` parses as `(x - edge0) << 8`). When reviewing WLED (and other) fixed-point/bit-manipulation code, confirm the intended parse using cppreference before reporting a precedence bug for mixed `-`/`+` and `<<`/`>>` expressions.
Learnt from: softhack007
Repo: wled/WLED PR: 5599
File: usermods/audioreactive/audio_reactive.cpp:1303-1308
Timestamp: 2026-05-11T16:00:13.574Z
Learning: When targeting arduino-esp32 v3.x (ESP-IDF 5.x), treat `WiFiUDP::flush()` as deprecated and ineffective for RX draining; it should not be used to clear/drain the UDP receive buffer. Instead, use `WiFiUDP::clear()`, which was introduced in arduino-esp32 v3.0 and is marked as the deprecated replacement via `NetworkUdp.h`.
If the code must compile and behave correctly across both arduino-esp32 v2.x (ESP-IDF < 5) and v3.x (ESP-IDF >= 5), use conditional compilation:
- for `ESP_IDF_VERSION_MAJOR < 5`: use `udp.flush()`
- for `ESP_IDF_VERSION_MAJOR >= 5`: use `udp.clear()`
Do not flag `WiFiUDP::clear()` usages for ESP-IDF >= 5 as non-existent—this is the documented correct replacement.
Learnt from: softhack007
Repo: wled/WLED PR: 5572
File: .coderabbit.yaml:116-116
Timestamp: 2026-05-24T11:50:21.052Z
Learning: In WLED usermods, treat these callback entry points as untrusted ingress boundaries because the WLED core does not sanitize payload content before invoking them:
- onMqttMessage(char* topic, char* payload): core only reassembles chunked MQTT payload into a null-terminated char*; no content/length validation.
- onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len): raw received bytes and len are passed directly from the ESP-NOW handler.
- onUdpPacket(uint8_t* payload, size_t len): raw udpIn buffer and packetSize are passed through unchanged after WLED’s own sync handling.
- readFromJsonState(JsonObject& obj): ArduinoJson parses structure, but field values are still client-supplied and must be treated as unvalidated.
Also list readFromConfig/getJsonValue as untrusted ingress boundaries for usermods. Ensure these are represented in the .coderabbit.yaml trust-boundary model.
Author
|
Meant to open PR against my fork not to this codebase, my apologies :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new usermod at usermods/usermod_v2_gc9a01_display for the Waveshare 1.28" Round LCD Module (240x240 IPS, GC9A01 driver, SPI).
Shows a circular watch-face layout: outer arc for brightness (0-360° yellow progress), colour disc filled with the primary segment colour, effect name, brightness %, and IP address. Screen blanks after a configurable timeout.
Uses TFT_eSPI (Bodmer) which requires compile-time pin configuration via build flags — a ready-to-use platformio_override.sample.ini is included. Pins are registered with PinManager for conflict detection.
Exposes wakeDisplay(), showOverlay(), and forceRedraw() for pairing with the I2C encoder usermod in a later pass.
USERMOD_ID_GC9A01_DISPLAY (60) added to const.h. UM_GC9A01Display and UM_I2CEncoderUI added to pin_manager.h.
Summary by CodeRabbit
New Features
Documentation