Add GC9A01 Round TFT Display UsermodFeature/gc9a01 display usermod #4989
Conversation
- Complete usermod for GC9A01 240x240 round displays - Circular UI optimized for round displays with blue bezel theming - Real-time status display: WiFi signal, power state, brightness, effects - Clock mode with 12/24-hour format support - Power management with configurable sleep and clock modes (5-300s timeout) - Brightness arc visualization (semicircular display) - Color preview buttons (FX, BG, CS) with automatic color synchronization - Effect and palette name display - Startup logo display (2-second duration) - Web UI configuration for all settings (timeout, backlight, rotation, etc.) - Non-blocking updates with state-based change detection - Compatible with rotary encoder usermod for enhanced interaction - Follows Four Line Display ALT pattern for consistency
Changed cast from (unsigned) to (int) in changePalette() function to prevent wraparound when decrementing palette index from 0. Previously, when decrementing from the first palette (index 0), the expression (unsigned)(-1) would wrap around to 4294967295, causing the palette selection to jump to the end of the palette list instead of staying at the first palette. This fix ensures palette navigation stays at boundaries, matching the behavior of effect navigation.
WalkthroughAdds a new GC9A01 240×240 TFT display usermod (code, manifest, README), updates Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
![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
🧹 Nitpick comments (4)
usermods/usermod_v2_gc9a01_display/library.json (1)
4-7: Consider dropping explicit Wire dependencyWire is part of the core on Arduino frameworks; declaring it here is typically unnecessary and can introduce resolution quirks. Safe to keep if you rely on PlatformIO to fetch it, but consider removing for simplicity.
If you keep it, please verify CI resolves it from the core and does not add a duplicate library.
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h (1)
25-33: Reserve the backlight pin via PinManager to avoid conflictsBacklight control uses raw pinMode/analogWrite. In WLED, PWM/LEDC channels and pins are a shared resource. Please allocate/deallocate TFT_BL via PinManager (PinOwner::UM_*) and attach a dedicated LEDC channel to avoid collisions with analog/PWM usermods.
Confirm no conflicts with LEDC channels when using analogWrite on ESP32. If you see flicker or channel contention, I can draft a PinManager/LEDC-based helper.
Also applies to: 46-58, 90-96
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (2)
894-901: Respect showHowLong in overlay() to match 4LD behavioroverlay() ignores showHowLong. Use it when >0, otherwise fall back to overlayInactivityTimeout.
Apply this diff:
- overlayUntil = millis() + overlayInactivityTimeout; // Use longer timeout for user interaction + overlayUntil = millis() + (showHowLong > 0 ? (unsigned long)showHowLong : overlayInactivityTimeout);
20-25: Backlight PWM: consider PinManager/LEDC attach to avoid channel conflictsDirect analogWrite/pinMode is convenient but may collide with other PWM users. Recommend:
- Reserve TFT_BL via PinManager.
- Attach a dedicated LEDC channel and use ledcWrite for stable PWM on ESP32.
I can provide a minimal patch aligned with WLED’s pin/channel management if desired.
Also applies to: 737-742
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)usermods/usermod_v2_gc9a01_display/library.json(1 hunks)usermods/usermod_v2_gc9a01_display/readme.md(1 hunks)usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp(1 hunks)usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h (1)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (62)
initDisplay(15-41)initDisplay(15-15)drawMainInterface(181-549)drawMainInterface(181-181)drawMainScreen(553-622)drawMainScreen(553-553)drawStatusBar(624-640)drawStatusBar(624-624)drawWLEDLogo(689-729)drawWLEDLogo(689-689)drawWiFiIcon(642-687)drawWiFiIcon(642-642)setBrightness(731-735)setBrightness(731-731)setBacklight(737-742)setBacklight(737-737)sleepOrClock(793-815)sleepOrClock(793-793)sleepDisplay(817-822)sleepDisplay(817-817)wakeDisplayFromSleep(826-842)wakeDisplayFromSleep(826-826)drawClockScreen(744-791)drawClockScreen(744-744)drawModeOverlay(1030-1032)drawModeOverlay(1030-1030)drawCurrentModeIndicator(1026-1028)drawCurrentModeIndicator(1026-1026)getEncoderModeName(1019-1024)getEncoderModeName(1019-1019)wakeDisplay(845-847)wakeDisplay(845-845)updateRedrawTime(849-858)updateRedrawTime(849-849)overlay(860-904)overlay(860-860)redraw(43-179)redraw(43-43)isOverlayActive(1034-1036)isOverlayActive(1034-1034)getActiveOverlayMode(1038-1043)getActiveOverlayMode(1038-1038)isDisplayAsleep(1045-1047)isDisplayAsleep(1045-1045)setup(907-926)setup(907-907)loop(928-938)loop(928-928)addToJsonInfo(940-947)addToJsonInfo(940-940)readFromJsonState(949-963)readFromJsonState(949-949)addToJsonState(965-968)addToJsonState(965-965)readFromConfig(970-995)readFromConfig(970-970)addToConfig(997-1007)addToConfig(997-997)appendConfigData(1009-1017)appendConfigData(1009-1009)getId(1049-1051)getId(1049-1049)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
wled00/colors.h (1)
getPaletteCount(127-127)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (1)
wled00/colors.h (1)
getPaletteCount(127-127)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp
Outdated
Show resolved
Hide resolved
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp
Outdated
Show resolved
Hide resolved
- readme.md: Remove AI attribution line, fix GPIO pin assignments
- Standardize DC=GPIO15, CS=GPIO5, BL=GPIO26 in both wiring table and build flags
- Remove unnecessary FLD_* build flags (not used by TFT_eSPI)
- usermod_v2_gc9a01_display.cpp: Improve palette and effect name handling
- Fix palette mode to use FIXED_PALETTE_COUNT and check extractModeName return value
- Add custom palette labeling ("Custom 1", "Custom 2", etc.)
- Fix effect name display to use extractModeName() instead of direct JSON indexing
- usermod_v2_rotary_encoder_ui_ALT.cpp: Fix palette double-counting
- Remove +customPalettes.size() from getPaletteCount() calls
- getPaletteCount() already includes custom palettes
- Fixed in both changePalette() and findCurrentEffectAndPalette()
Wire is already included in the Arduino/ESP32 core and doesn't need to be explicitly declared as a dependency.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (2)
286-318: Consider extracting arc drawing into a helper function.The brightness ring (lines 286-318) and value arc (lines 393-425) use nearly identical drawing logic. Extracting this into a helper function would reduce duplication and improve maintainability.
Example helper function:
void UsermodGC9A01Display::drawProgressArc(float startAngle, float arcLength, int progressPercent, uint16_t bgColor, uint16_t fgColor, int baseRadius) { float progressAngle = (progressPercent > 0) ? map(progressPercent, 0, 100, 0, arcLength) : 0; // Draw background arc for (float angle = 0; angle < arcLength; angle += 6) { float currentAngle = startAngle + angle; float rad = radians(currentAngle - 90); for (int ringWidth = 0; ringWidth < 3; ringWidth++) { int radius = baseRadius - ringWidth; int x = 120 + radius * cos(rad); int y = 120 + radius * sin(rad); if (x >= 0 && x < 240 && y >= 0 && y < 240) { tft.drawPixel(x, y, bgColor); } } } // Draw progress arc if (progressPercent > 0 && progressAngle > 0) { for (float angle = 0; angle < progressAngle; angle += 3) { float currentAngle = startAngle + angle; float rad = radians(currentAngle - 90); for (int ringWidth = 0; ringWidth < 3; ringWidth++) { int radius = baseRadius - ringWidth; int x = 120 + radius * cos(rad); int y = 120 + radius * sin(rad); if (x >= 0 && x < 240 && y >= 0 && y < 240) { tft.drawPixel(x, y, fgColor); } } } } }Also applies to: 393-425
693-733: Consider using named constants for display dimensions.Magic numbers like 14400, 120, and 240 reduce readability. Using named constants (e.g.,
DISPLAY_WIDTH,LOGO_SIZE) would make the code clearer.Example:
void UsermodGC9A01Display::drawWLEDLogo() { const int DISPLAY_SIZE = 240; const int LOGO_SIZE = 120; const int LOGO_PIXELS = LOGO_SIZE * LOGO_SIZE; // 14400 const int OFFSET_X = (DISPLAY_SIZE - LOGO_SIZE) / 2; const int OFFSET_Y = (DISPLAY_SIZE - LOGO_SIZE) / 2; tft.fillScreen(TFT_BLACK); tft.setAddrWindow(OFFSET_X, OFFSET_Y, LOGO_SIZE, LOGO_SIZE); tft.startWrite(); for (int i = 0; i < LOGO_PIXELS; i++) { // ... rest of the code } tft.endWrite(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/usermod_v2_gc9a01_display/library.json(1 hunks)usermods/usermod_v2_gc9a01_display/readme.md(1 hunks)usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- usermods/usermod_v2_gc9a01_display/readme.md
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp
- usermods/usermod_v2_gc9a01_display/library.json
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (2)
wled00/colors.h (1)
getPaletteCount(127-127)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (7)
enabled(299-299)setup(484-540)setup(484-484)loop(552-682)loop(552-552)readFromConfig(1104-1165)readFromConfig(1104-1104)
🔇 Additional comments (3)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (3)
496-529: LGTM! Robust effect name handling.This defensive implementation properly handles PROGMEM data, strips metadata, and sanitizes non-printable characters. This is the preferred pattern for effect name retrieval throughout the codebase.
356-375: LGTM! Palette handling properly addresses custom palettes.The fix from the previous review has been correctly applied. Custom palettes (indexes >= FIXED_PALETTE_COUNT) are now handled separately without calling
extractModeName, preventing out-of-bounds reads of JSON_palette_names.
864-908: LGTM! Well-integrated overlay system.The overlay mechanism properly:
- Maps rotary encoder glyphs to display modes
- Wakes the display when needed
- Manages timeout and redraw state
- Provides visual feedback for user interactions
Good integration with the rotary encoder usermod.
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp
Outdated
Show resolved
Hide resolved
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp
Outdated
Show resolved
Hide resolved
- Add extractModeName return value check in effect mode (line ~338) - Mirrors palette mode error handling pattern - Falls back to 'Effect N' if extraction fails - Remove unused dead code functions: - drawMainScreen() - replaced by drawMainInterface() - drawStatusBar() - only called from removed drawMainScreen() - Add TFT_BL compile-time guards for backlight control: - setBacklight() now compiles without TFT_BL defined - sleepDisplay() guards analogWrite(TFT_BL, 0) - sleepOrClock() uses setBacklight(0) instead of direct analogWrite - Functions maintain state management even without backlight pin All changes preserve existing behavior and ensure compilation safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (4)
503-527: Effect name retrieval correctly implemented.The code properly retrieves effect names from PROGMEM using
strncpy_Pandstrip.getModeData(), avoiding the direct indexing issue flagged in past reviews. The character filtering logic, while verbose, safely removes non-printable characters.Consider simplifying the character filtering (lines 514-520) using a helper function or standard library utilities for improved readability:
// Helper function (add to private methods) String cleanEffectName(const String& name) { String cleaned = ""; for (char c : name) { if (c >= 32 && c <= 126 && c != '@') cleaned += c; } return cleaned.length() > 0 ? cleaned : "Effect " + String(knownMode); }
73-88: Consider extracting repeated overlay state-checking logic.The repeated pattern (check state → update known value → redraw) for brightness, mode, palette, speed, and intensity could be consolidated into a helper method to improve maintainability and reduce duplication.
Example refactor:
// Add to private methods template<typename T> bool checkAndUpdateOverlayState(T& knownValue, T currentValue) { if (knownValue != currentValue) { if (displayTurnedOff) return true; // Signal need for full redraw knownValue = currentValue; drawMainInterface(activeOverlayMode); lastRedraw = millis(); } return false; } // Usage in redraw() if (checkAndUpdateOverlayState(knownBrightness, bri)) { needRedraw = true; return; } if (checkAndUpdateOverlayState(knownMode, effectCurrent)) { needRedraw = true; return; } // ... etc
278-318: Arc drawing logic is duplicated.The brightness ring (lines 278-318) and value arc (lines 386-428) share nearly identical drawing logic with minor variations in colors and angle ranges. Extracting this into a helper method would reduce duplication.
Consider adding a helper method:
void drawProgressArc(float startAngle, float arcLength, float progressPercent, uint16_t bgColor, uint16_t fgColor) { float progressAngle = map(progressPercent, 0, 100, 0, arcLength); // Draw background arc for (float angle = 0; angle < arcLength; angle += 6) { float currentAngle = startAngle + angle; float rad = radians(currentAngle - 90); for (int ringWidth = 0; ringWidth < 3; ringWidth++) { int radius = 108 - ringWidth; int x = 120 + radius * cos(rad); int y = 120 + radius * sin(rad); if (x >= 0 && x < 240 && y >= 0 && y < 240) { tft.drawPixel(x, y, bgColor); } } } // Draw progress arc (similar pattern with fgColor) // ... }Also applies to: 386-428
696-696: Prefer snprintf over sprintf for buffer safety.While the buffer is correctly sized for this specific format string, using
snprintfprovides explicit overflow protection and follows modern C safety practices.Apply this change:
- sprintf(timeStr, "%02d:%02d", hrs, mins); + snprintf(timeStr, sizeof(timeStr), "%02d:%02d", hrs, mins);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp(1 hunks)usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h (1)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (44)
initDisplay(15-41)initDisplay(15-15)drawMainInterface(181-555)drawMainInterface(181-181)drawWLEDLogo(606-646)drawWLEDLogo(606-606)drawWiFiIcon(559-604)drawWiFiIcon(559-559)setBrightness(648-652)setBrightness(648-648)setBacklight(654-663)setBacklight(654-654)sleepOrClock(714-736)sleepOrClock(714-714)sleepDisplay(738-745)sleepDisplay(738-738)wakeDisplayFromSleep(749-765)wakeDisplayFromSleep(749-749)drawClockScreen(665-712)drawClockScreen(665-665)drawModeOverlay(953-955)drawModeOverlay(953-953)drawCurrentModeIndicator(949-951)drawCurrentModeIndicator(949-949)getEncoderModeName(942-947)getEncoderModeName(942-942)wakeDisplay(768-770)wakeDisplay(768-768)updateRedrawTime(772-781)updateRedrawTime(772-772)overlay(783-827)overlay(783-783)redraw(43-179)redraw(43-43)isOverlayActive(957-959)isOverlayActive(957-957)getActiveOverlayMode(961-966)getActiveOverlayMode(961-961)isDisplayAsleep(968-970)isDisplayAsleep(968-968)setup(830-849)setup(830-830)loop(851-861)loop(851-851)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (2)
wled00/colors.h (1)
getPaletteCount(127-127)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (5)
enabled(299-299)setup(484-540)setup(484-484)loop(552-682)loop(552-552)
🔇 Additional comments (6)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h (1)
1-126: LGTM! Well-structured usermod header.The header file demonstrates a clean design with appropriate use of the singleton pattern for rotary encoder integration, sensible pin definition fallbacks, and a comprehensive public interface that follows WLED usermod conventions. The state tracking variables and method declarations are well-organized.
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (5)
333-349: Past review feedback properly addressed.The code now correctly checks the return value of
extractModeNamebefore using the extracted effect name, with an appropriate fallback for extraction failures. This prevents potential display corruption from garbage data.
359-378: Custom palette handling correctly implemented.The code now properly distinguishes between fixed palettes (using
extractModeNamesafely) and custom palettes (showing a generic label), preventing out-of-bounds reads inJSON_palette_names. This addresses the past review concern.
654-663: TFT_BL pin usage properly guarded.All backlight control code now has appropriate
#ifdef TFT_BLguards or uses the guardedsetBacklight()wrapper, ensuring safe compilation when the backlight pin is not defined. This addresses all past review concerns about unguardedanalogWritecalls.Also applies to: 714-736, 738-745
893-918: Configuration handling is well-implemented.The
readFromConfigmethod properly validates and clamps the timeout value (5-300 seconds), uses safe fallback defaults with the|operator, and correctly converts between seconds (UI) and milliseconds (internal storage). The backlight update only occurs when the value changes, preventing unnecessary writes.
1-980: Comprehensive and well-tested implementation.The implementation successfully addresses all past review concerns (effect/palette name handling, TFT_BL guards) and provides a feature-rich display usermod with proper state management, overlay support, and rotary encoder integration. The code follows established WLED patterns and includes appropriate error handling and fallbacks.
|
nice one, I have been tempted to try this. Do you mind sharing a foto of what it looks like? |
My pleasure |
Documentation improvements: - Remove AI attribution from readme - Fix GPIO pin numbers in wiring table (DC=GPIO15, BL=GPIO26) - Remove incorrect FLD_* build flags documentation Code improvements: - Add extractModeName() return value checks in 3 locations - Remove unused drawMainScreen() and drawStatusBar() functions - Add #ifdef TFT_BL guards around all backlight control code - Fix palette double-counting in rotary encoder navigation - Remove Wire dependency from library.json (part of core) - Adjust overlay text X coordinate to 124 for better centering All changes tested on hardware and verified working.
|
PR Feedback Addressed Changes Made: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
777-804: Prefer DEBUG_PRINTLN for consistency.Lines 792 and 800 use
Serial.printfandSerial.printlndirectly. For consistency with the rest of the codebase, consider usingDEBUG_PRINTFandDEBUG_PRINTLNmacros instead.Apply this diff:
#ifdef USERMOD_GC9A01_DISPLAY if (gc9a01Display != nullptr) { - Serial.printf("[RotaryEncoder] Calling GC9A01 overlay: '%s' glyph=%d\n", stateName, glyph); + DEBUG_PRINTF("[RotaryEncoder] Calling GC9A01 overlay: '%s' glyph=%d\n", stateName, glyph); if (gc9a01Display->wakeDisplay()) { // Throw away wake up input gc9a01Display->redraw(true); return false; } gc9a01Display->overlay(stateName, 750, glyph); } else { - Serial.println("[RotaryEncoder] gc9a01Display is NULL!"); + DEBUG_PRINTLN(F("[RotaryEncoder] gc9a01Display is NULL!")); } #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (2)
wled00/colors.h (1)
getPaletteCount(127-127)usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (2)
USERMOD_FOUR_LINE_DISPLAY(83-90)USERMOD_FOUR_LINE_DISPLAY(100-113)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (2)
wled00/colors.h (1)
getPaletteCount(127-127)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (5)
enabled(310-310)setup(495-563)setup(495-495)loop(575-739)loop(575-575)
🔇 Additional comments (16)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (7)
34-36: LGTM! GC9A01 display integration follows the established pattern.The conditional compilation, member declaration, and initialization mirror the existing Four Line Display integration, ensuring consistency and maintainability.
Also applies to: 185-189, 278-278
547-557: LGTM! GC9A01 display lookup is properly implemented.The UsermodManager::lookup pattern matches the existing Four Line Display integration, with appropriate null checks and status logging.
644-695: LGTM! State handling logic is well-designed.The code includes several defensive programming measures:
- Special handling for GC9A01-only brightness mode (direct main screen update instead of overlay)
- Automatic reset to brightness mode when overlay expires or display sleeps
- Validation that the current effect supports the selected state's slider
These safeguards prevent invalid UI states and improve user experience.
741-751: LGTM! Network info overlay integration is correct.The GC9A01 overlay call uses the appropriate glyph type (12) for network information display.
753-775: LGTM! Palette search uses the correct count.Line 768 correctly uses
getPaletteCount()which already includes custom palettes (as defined inwled00/colors.h), avoiding any double-counting issues.
814-855: LGTM! Brightness handling respects the GC9A01 design.The special logic at lines 845-853 correctly handles brightness changes:
- In state 0 (brightness mode): Updates the main screen directly without overlay
- In other states: Shows a brightness overlay
This design choice aligns with the circular UI's main screen showing brightness by default.
1047-1089: LGTM! Critical palette wraparound bug fixed.Line 1066 correctly clamps
effectPaletteIndexto[0, getPaletteCount()-1]. This fixes the critical issue from the previous review where addingcustomPalettes.size()would double-count custom palettes (sincegetPaletteCount()already includes them), potentially causing out-of-bounds access.Based on past review comments.
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (9)
15-41: LGTM! Display initialization properly guards backlight control.The
#ifdef TFT_BLguard (lines 21-25) prevents compilation errors when the backlight pin is not defined, addressing the concern from past reviews.
43-179: LGTM! Redraw logic follows proven Four Line Display pattern.The redraw method implements comprehensive state tracking and conditional updates:
- Welcome screen transition
- Overlay mode with timeout
- State change detection (brightness, mode, palette, speed, intensity, colors)
- Sleep/clock mode transitions
This design minimizes unnecessary redraws while ensuring the display stays synchronized with WLED state.
332-349: LGTM! Effect name extraction now checks return value.Line 338 properly checks the return value of
extractModeNamebefore using the result, preventing garbage data from being displayed if extraction fails. This addresses the concern from past reviews.
359-378: LGTM! Palette name extraction handles custom palettes safely.Lines 364 and 372-373 properly check the
extractModeNamereturn value and fall back to a generic label when extraction fails (e.g., for custom palettes not inJSON_palette_names). The else block at lines 375-377 is defensive programming for edge cases, though it's likely unreachable in normal operation sinceeffectPaletteis clamped to[0, getPaletteCount()-1]inchangePalette.
498-533: LGTM! Effect name retrieval correctly uses PROGMEM access.Lines 504-520 properly retrieve the effect name using
strip.getModeData()withstrncpy_Pfor PROGMEM access, addressing the critical issue from past reviews where direct indexing ofJSON_mode_nameswould yield a single char instead of the full name.
648-661: LGTM! Backlight control properly guards TFT_BL pin.Lines 657-659 guard the
analogWrite(TFT_BL, pwm)call with#ifdef TFT_BL, preventing compilation errors when the backlight pin is not defined. This addresses the concern from past reviews.
712-745: LGTM! Sleep and clock modes properly guard backlight control.Lines 724-726 and 739-741 guard
analogWrite(TFT_BL, 0)calls with#ifdef TFT_BL, ensuring the code compiles even when the backlight pin is not defined. This addresses the concern from past reviews.
747-827: LGTM! Rotary encoder integration follows Four Line Display pattern.The integration methods (
wakeDisplay,updateRedrawTime,overlay) mirror the existing Four Line Display API, ensuring compatibility with the rotary encoder usermod. The overlay method correctly maps glyph types to display modes (lines 803-815) and handles sleep-to-wake transitions properly.
829-978: LGTM! Public API implementation is complete and correct.The usermod properly implements all required methods:
setup()andloop()for lifecycle management- JSON/config methods for state persistence with validation (e.g., timeout clamping at lines 907-909)
- Utility methods for overlay state and display status
- Proper registration using
REGISTER_USERMODmacroThe implementation follows WLED usermod patterns and integrates seamlessly with the rotary encoder usermod.
Replace Serial.printf and Serial.println with WLED debug macros in rotary encoder usermod for consistency with codebase standards.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
768-768: LGTM: Palette double-counting issue correctly resolved.Line 1066 now correctly uses
getPaletteCount()-1as the upper bound without addingcustomPalettes.size(), addressing the critical issue flagged in the previous review. ThegetPaletteCount()function already includes custom palettes, so the previous double-counting has been eliminated. Line 768 also correctly usesgetPaletteCount()for iteration.Also applies to: 1066-1066
🧹 Nitpick comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
547-557: Consider removing Serial.println in favor of DEBUG_PRINTLN.Lines 552 and 555 use
Serial.printlnfor logging. Since these are already wrapped withDEBUG_PRINTLN, theSerial.printlnstatements are redundant and should be removed to avoid unnecessary output in production builds.Apply this diff:
gc9a01Display = (UsermodGC9A01Display*) UsermodManager::lookup(USERMOD_ID_GC9A01_DISPLAY); if (gc9a01Display != nullptr) { DEBUG_PRINTLN(F("[RotaryEncoder] GC9A01 display integration enabled")); - Serial.println(F("[RotaryEncoder] GC9A01 display integration enabled")); } else { DEBUG_PRINTLN(F("[RotaryEncoder] GC9A01 display NOT FOUND")); - Serial.println(F("[RotaryEncoder] GC9A01 display NOT FOUND")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(26 hunks)
🔇 Additional comments (6)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (6)
34-36: LGTM: Clean conditional compilation pattern.The GC9A01 display integration follows the existing Four Line Display pattern with proper conditional compilation guards and consistent initialization.
Also applies to: 185-189, 278-278
644-695: LGTM: Thoughtful state management for GC9A01 display.The implementation includes several intelligent safeguards:
- Lines 647-656: Special handling for brightness mode (state 0) to update the main screen directly without overlays when using GC9A01 alone
- Lines 677-685: Automatic return to brightness mode when overlays expire or display sleeps
- Lines 687-695: Validation that the current effect supports the active slider state
The complexity is justified by the different display behaviors and ensures a smooth user experience.
745-750: LGTM: Consistent display integration in helper methods.Both
displayNetworkInfo()andchangeState()properly integrate GC9A01 display support with appropriate null checks, wake handling, and overlay rendering using consistent glyph mappings.Also applies to: 790-802
824-831: LGTM: Brightness handling aligns with state management design.The brightness change implementation correctly differentiates between default state (state 0) and overlay modes:
- In state 0: Updates the main screen directly for seamless brightness control
- In overlay modes: Shows a temporary brightness overlay
This aligns with the state management logic in the
loop()method and provides a consistent user experience.Also applies to: 841-854
867-875: LGTM: Consistent pattern across effect, speed, intensity, custom, and palette methods.All change handlers follow a uniform pattern:
- Wake display and discard input if waking from sleep
- Update redraw timer to prevent premature sleep
- Render overlays or update main screen as appropriate
The implementation is consistent, properly guarded with null checks, and maintains parity with the Four Line Display integration.
Also applies to: 905-913, 949-957, 994-1002, 1056-1064, 932-936, 976-980, 1037-1043, 1084-1088
1101-1109: LGTM: Consistent pattern in hue, saturation, preset, and CCT handlers.The remaining change handlers (
changeHue,changeSat,changePreset,changeCCT) maintain the same high-quality pattern as the other methods, with proper wake handling, timer updates, and informative overlay messages.Also applies to: 1149-1157, 1196-1204, 1246-1254, 1131-1137, 1178-1184, 1227-1233, 1274-1280
DEBUG_PRINTLN already handles the logging, so the duplicate Serial.println calls are unnecessary and would create redundant output.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (2)
wled00/colors.h (1)
getPaletteCount(127-127)usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (2)
USERMOD_FOUR_LINE_DISPLAY(83-90)USERMOD_FOUR_LINE_DISPLAY(100-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (4)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (4)
1064-1064: LGTM! Palette navigation fix correctly implements the past review feedback.The change from
getPaletteCount()+customPalettes.size()-1togetPaletteCount()-1correctly fixes the double-counting issue flagged in the previous review. SincegetPaletteCount()already returnsFIXED_PALETTE_COUNT + customPalettes.size(), addingcustomPalettes.size()again would cause out-of-bounds access. Themax(..., 0)also prevents wraparound when decrementing from index 0, addressing the bug mentioned in the PR summary.Based on past review comments.
766-766: LGTM! Palette iteration correctly uses getPaletteCount().This addresses the optional follow-up suggestion from the past review comment to ensure loops iterate using
getPaletteCount()directly without addingcustomPalettes.size().Based on past review comments.
675-693: Excellent defensive programming with state validation.The addition of overlay expiration checking (lines 675-683) and effect slider validation (lines 685-693) properly handles edge cases:
Overlay expiration: Automatically returns to brightness mode (state 0) when the GC9A01 overlay expires or the display goes to sleep, preventing the user from being stuck in a non-visible overlay state.
Effect slider validation: Handles the scenario where the current effect changes (e.g., via web UI or preset) while the rotary encoder is in a slider state (speed, intensity, or custom parameters). If the new effect doesn't support the current slider, it gracefully resets to brightness mode rather than attempting to control a non-existent parameter.
These checks prevent potential confusion and ensure the UI state remains consistent with the available controls.
642-654: Good UX design: Brightness mode displays on main screen for GC9A01.The special handling for state 0 (brightness) with GC9A01-only configurations is well-designed. Rather than showing a temporary overlay, brightness changes update the main screen directly, providing continuous visual feedback. This is appropriate since brightness is the default state and users expect to see the current brightness value on the main display rather than as a temporary overlay.
The wake detection properly discards the input that triggered the wake, preventing unintended state changes.
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp
Show resolved
Hide resolved
Prevent potential null pointer dereference crashes when GC9A01 display usermod is not found or fails to initialize. The gc9a01Display pointer can be null (as shown in setup() where it logs 'GC9A01 display NOT FOUND'), so all calls to updateRedrawTime() must be guarded with null checks. Fixed 10 locations where updateRedrawTime() was called without checking if gc9a01Display is non-null.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
676-693: Consider documenting automatic state reset behaviors.The code introduces two automatic state reset mechanisms:
- Lines 676-683: Resets to brightness mode (state 0) when GC9A01 overlay expires or display sleeps
- Lines 686-693: Resets to brightness mode when the current effect doesn't support the active slider (e.g., effect changed via web UI)
These are executed in the main loop and can change state without direct user input. While this is likely intentional UX design, consider adding inline comments explaining these behaviors, as they differ from the Four Line Display implementation and could be non-obvious to future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (2)
wled00/colors.h (1)
getPaletteCount(127-127)usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (2)
USERMOD_FOUR_LINE_DISPLAY(83-90)USERMOD_FOUR_LINE_DISPLAY(100-113)
🔇 Additional comments (6)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (6)
34-36: LGTM: Clean conditional compilation pattern.The GC9A01 display integration follows the same conditional compilation pattern as the existing Four Line Display support, with proper header guards, fallback void* pointer for undefined builds, and null initialization.
Also applies to: 185-189, 278-278
547-555: LGTM: Proper usermod lookup with null handling.The GC9A01 display is correctly looked up using
UsermodManager::lookup()with appropriate null checking and debug logging, consistent with the existing Four Line Display integration pattern.
766-766: Previous palette double-counting issues resolved.The palette counting has been corrected:
- Line 766: Loop now uses
getPaletteCount()directly (which already includes custom palettes)- Line 1064: Upper bound clamp correctly uses
getPaletteCount()-1These changes address the critical issue flagged in previous reviews where custom palettes were being double-counted.
Also applies to: 1064-1064
828-828: Previous null pointer dereference issues resolved.All
gc9a01Display->updateRedrawTime()calls now include proper null checks (if (gc9a01Display) ...), addressing the critical issue flagged in previous reviews at all 10 locations.Also applies to: 872-872, 910-910, 954-954, 999-999, 1061-1061, 1106-1106, 1154-1154, 1201-1201, 1251-1251
744-748: LGTM: Consistent GC9A01 integration across all UI methods.All UI methods (
displayNetworkInfo,changeState,changeBrightness,changeEffect, etc.) follow a consistent and safe integration pattern:
- Wake display check with null guard
- Update redraw time with null check
- Conditional overlay/redraw calls with null checks
The implementation properly multiplexes between Four Line Display and GC9A01 display without breaking existing functionality.
Also applies to: 788-800, 822-852, 866-891, 904-934, 948-978, 993-1041, 1055-1086, 1100-1135, 1148-1182, 1195-1231, 1245-1278
643-654: LGTM: Thoughtful UX for GC9A01 brightness handling.The special handling for brightness mode (state 0) when using GC9A01 as the sole display is well-designed:
- Updates the main screen directly instead of showing an overlay, which is appropriate for the default state
- Properly handles wake/sleep scenarios
- Only activates when GC9A01 is present without Four Line Display, preserving existing behavior for other configurations
2 participants
Overview
Adds comprehensive support for GC9A01 240x240 round TFT displays with a circular UI layout optimized for round displays.
Features
Hardware Requirements
Additional Changes
This PR also includes a bug fix for the rotary encoder ALT usermod that prevents palette navigation wraparound when decrementing from index 0.
Testing
Thoroughly tested on ESP32-D0WDQ6 with GC9A01 display. All features verified including power management, color synchronization, and rotary encoder integration.
Files Changed
usermods/usermod_v2_gc9a01_display/(5 files)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(line 895)Summary by CodeRabbit
New Features
Documentation
Chores