📝 Add docstrings to feature/gc9a01-display-usermod
#5301
+1,932
−37
Conversation
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
Docstrings generation was requested by @srg74. * #4989 (comment) The following files were modified: * `usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` * `usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h` * `usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`
Contributor
Author
WalkthroughThis PR introduces a new GC9A01 240x240 TFT display usermod with hardware initialization, dynamic UI rendering, overlay management, sleep/wake logic, and clock support. It also integrates GC9A01 support into the rotary encoder UI usermod for coordinated display updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
@coderabbitai review |
Contributor
Author
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`:
- Around line 663-691: The code calls gc9a01Display methods (wakeDisplay,
redraw) when USERMOD_GC9A01_DISPLAY may be undefined causing a compile error
because gc9a01Display is a void*; wrap the GC9A01-specific branch and any direct
calls to gc9a01Display (the special-case "newState == 0" block and its
wakeDisplay()/redraw() calls) in an `#ifdef` USERMOD_GC9A01_DISPLAY ... `#endif`
guard so those methods are only compiled when the GC9A01 display type is
enabled; keep the surrounding logic for display and the switch/newState handling
unchanged and ensure no other unguarded calls to gc9a01Display remain (search
for gc9a01Display, wakeDisplay, redraw).
🧹 Nitpick comments (4)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h (1)
34-183: Floating docstrings are disconnected from their declarations.These docstrings (lines 34–183) are placed before the class definition and are not associated with any actual declarations. They appear to document methods that are declared later inside the class (lines 184–274), but there's no syntactic connection between them.
Consider one of the following approaches:
- Move these docstrings directly above their corresponding method declarations inside the class
- Remove them entirely since the
.cppfile already contains comprehensive docstrings for each implementation- Use Doxygen-style grouping if you want to keep them as a reference block
The current placement may confuse documentation generators and doesn't follow C++ conventions.
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (3)
325-357: Consider extracting duplicated arc drawing code.The arc drawing logic appears twice: once for the brightness ring (lines 325–357) and again for value arcs in overlay modes (lines 435–466). This could be refactored into a helper function to reduce code duplication.
♻️ Proposed helper function
// Add as private method in header: void drawArc(float startAngle, float arcLength, float progressAngle, uint16_t bgColor, uint16_t fgColor); // Implementation: void UsermodGC9A01Display::drawArc(float startAngle, float arcLength, float progressAngle, uint16_t bgColor, uint16_t fgColor) { // 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 if (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 = 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, fgColor); } } } } }Also applies to: 435-466
684-697: Consider using named constants for logo dimensions.The magic number
14400(120 × 120 pixels) is used directly in the loop. WhileLOGO_SIZEis defined, it could be used more consistently.♻️ Suggested improvement
const int LOGO_SIZE = 120; + const int LOGO_PIXELS = LOGO_SIZE * LOGO_SIZE; // 14400 const int OFFSET_X = (240 - LOGO_SIZE) / 2; const int OFFSET_Y = (240 - LOGO_SIZE) / 2; // ... - for (int i = 0; i < 14400; i++) { // 120 * 120 = 14,400 pixels + for (int i = 0; i < LOGO_PIXELS; i++) {
378-388: Remove or clarify the defensive UTF-8 note symbol stripping logic.The code checks for and removes a 5-byte UTF-8 "note symbol" prefix from effect names, but this format does not exist in the current codebase. The
extractModeName()function returns plain ASCII text without any leading space or special character prefix. The same defensive pattern exists inusermod_v2_four_line_display_ALT.cpp(lines 507-510) with identical logic, suggesting this may be legacy code or anticipatory for future formats that never materialized.Consider either removing this check (since it's unreachable with current data) or documenting why this defensive handling is necessary if it's intended to support future extensibility.
Comment on lines
+663
to
691
| // Support both Four Line Display and GC9A01 display | ||
| if (display != nullptr || gc9a01Display != nullptr) { | ||
| // Special handling for state 0 (Brightness) with GC9A01 - no overlay needed | ||
| if (newState == 0 && gc9a01Display != nullptr && display == nullptr) { | ||
| // For GC9A01 only (no four line display), just update main screen for brightness | ||
| if (gc9a01Display->wakeDisplay()) { | ||
| gc9a01Display->redraw(true); | ||
| changedState = false; // Throw away wake up input | ||
| } else { | ||
| gc9a01Display->redraw(false); // Update main screen directly | ||
| changedState = true; | ||
| } | ||
| } else { | ||
| // Normal overlay handling for all other states and for Four Line Display | ||
| switch (newState) { | ||
| case 0: changedState = changeState(lineBuffer, 1, 0, 1); break; //1 = sun | ||
| case 1: changedState = changeState(lineBuffer, 1, 4, 2); break; //2 = skip forward | ||
| case 2: changedState = changeState(lineBuffer, 1, 8, 3); break; //3 = fire | ||
| case 3: changedState = changeState(lineBuffer, 2, 0, 4); break; //4 = custom palette | ||
| case 4: changedState = changeState(lineBuffer, 3, 0, 5); break; //5 = puzzle piece | ||
| case 5: changedState = changeState(lineBuffer, 255, 255, 7); break; //7 = brush | ||
| case 6: changedState = changeState(lineBuffer, 255, 255, 8); break; //8 = contrast | ||
| case 7: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | ||
| case 8: changedState = changeState(lineBuffer, 255, 255, 11); break; //11 = heart | ||
| case 9: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | ||
| case 10: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | ||
| case 11: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Critical: Compilation error due to missing preprocessor guard.
The pipeline failure indicates that when USERMOD_GC9A01_DISPLAY is not defined, gc9a01Display is declared as void* (line 188), but this code attempts to call methods on it without the proper #ifdef guard. This causes the compilation error: 'void*' is not a pointer-to-object type.
🐛 Proposed fix: wrap GC9A01-specific code in preprocessor guard
// Support both Four Line Display and GC9A01 display
- if (display != nullptr || gc9a01Display != nullptr) {
+ `#ifdef` USERMOD_GC9A01_DISPLAY
+ bool hasGc9a01 = (gc9a01Display != nullptr);
+ `#else`
+ bool hasGc9a01 = false;
+ `#endif`
+ if (display != nullptr || hasGc9a01) {
// Special handling for state 0 (Brightness) with GC9A01 - no overlay needed
- if (newState == 0 && gc9a01Display != nullptr && display == nullptr) {
+ `#ifdef` USERMOD_GC9A01_DISPLAY
+ if (newState == 0 && hasGc9a01 && display == nullptr) {
// For GC9A01 only (no four line display), just update main screen for brightness
if (gc9a01Display->wakeDisplay()) {
gc9a01Display->redraw(true);
changedState = false; // Throw away wake up input
} else {
gc9a01Display->redraw(false); // Update main screen directly
changedState = true;
}
- } else {
+ } else
+ `#endif`
+ {
// Normal overlay handling for all other states and for Four Line Display
switch (newState) {
// ... existing switch cases ...
}
}
}📝 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
| // Support both Four Line Display and GC9A01 display | |
| if (display != nullptr || gc9a01Display != nullptr) { | |
| // Special handling for state 0 (Brightness) with GC9A01 - no overlay needed | |
| if (newState == 0 && gc9a01Display != nullptr && display == nullptr) { | |
| // For GC9A01 only (no four line display), just update main screen for brightness | |
| if (gc9a01Display->wakeDisplay()) { | |
| gc9a01Display->redraw(true); | |
| changedState = false; // Throw away wake up input | |
| } else { | |
| gc9a01Display->redraw(false); // Update main screen directly | |
| changedState = true; | |
| } | |
| } else { | |
| // Normal overlay handling for all other states and for Four Line Display | |
| switch (newState) { | |
| case 0: changedState = changeState(lineBuffer, 1, 0, 1); break; //1 = sun | |
| case 1: changedState = changeState(lineBuffer, 1, 4, 2); break; //2 = skip forward | |
| case 2: changedState = changeState(lineBuffer, 1, 8, 3); break; //3 = fire | |
| case 3: changedState = changeState(lineBuffer, 2, 0, 4); break; //4 = custom palette | |
| case 4: changedState = changeState(lineBuffer, 3, 0, 5); break; //5 = puzzle piece | |
| case 5: changedState = changeState(lineBuffer, 255, 255, 7); break; //7 = brush | |
| case 6: changedState = changeState(lineBuffer, 255, 255, 8); break; //8 = contrast | |
| case 7: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| case 8: changedState = changeState(lineBuffer, 255, 255, 11); break; //11 = heart | |
| case 9: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| case 10: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| case 11: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| } | |
| } | |
| // Support both Four Line Display and GC9A01 display | |
| `#ifdef` USERMOD_GC9A01_DISPLAY | |
| bool hasGc9a01 = (gc9a01Display != nullptr); | |
| `#else` | |
| bool hasGc9a01 = false; | |
| `#endif` | |
| if (display != nullptr || hasGc9a01) { | |
| // Special handling for state 0 (Brightness) with GC9A01 - no overlay needed | |
| `#ifdef` USERMOD_GC9A01_DISPLAY | |
| if (newState == 0 && hasGc9a01 && display == nullptr) { | |
| // For GC9A01 only (no four line display), just update main screen for brightness | |
| if (gc9a01Display->wakeDisplay()) { | |
| gc9a01Display->redraw(true); | |
| changedState = false; // Throw away wake up input | |
| } else { | |
| gc9a01Display->redraw(false); // Update main screen directly | |
| changedState = true; | |
| } | |
| } else | |
| `#endif` | |
| { | |
| // Normal overlay handling for all other states and for Four Line Display | |
| switch (newState) { | |
| case 0: changedState = changeState(lineBuffer, 1, 0, 1); break; //1 = sun | |
| case 1: changedState = changeState(lineBuffer, 1, 4, 2); break; //2 = skip forward | |
| case 2: changedState = changeState(lineBuffer, 1, 8, 3); break; //3 = fire | |
| case 3: changedState = changeState(lineBuffer, 2, 0, 4); break; //4 = custom palette | |
| case 4: changedState = changeState(lineBuffer, 3, 0, 5); break; //5 = puzzle piece | |
| case 5: changedState = changeState(lineBuffer, 255, 255, 7); break; //7 = brush | |
| case 6: changedState = changeState(lineBuffer, 255, 255, 8); break; //8 = contrast | |
| case 7: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| case 8: changedState = changeState(lineBuffer, 255, 255, 11); break; //11 = heart | |
| case 9: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| case 10: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| case 11: changedState = changeState(lineBuffer, 255, 255, 10); break; //10 = star | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: WLED CI
[error] 668-672: Compilation error: 'void*' is not a pointer-to-object type. Accessing gc9a01Display->wakeDisplay()/redraw() with a void* type in RotaryEncoder UI ALT module. This suggests a type mismatch where gc9a01Display is not a class instance.
🤖 Prompt for AI Agents
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`
around lines 663 - 691, The code calls gc9a01Display methods (wakeDisplay,
redraw) when USERMOD_GC9A01_DISPLAY may be undefined causing a compile error
because gc9a01Display is a void*; wrap the GC9A01-specific branch and any direct
calls to gc9a01Display (the special-case "newState == 0" block and its
wakeDisplay()/redraw() calls) in an `#ifdef` USERMOD_GC9A01_DISPLAY ... `#endif`
guard so those methods are only compiled when the GC9A01 display type is
enabled; keep the surrounding logic for display and the switch/newState handling
unchanged and ensure no other unguarded calls to gc9a01Display remain (search
for gc9a01Display, wakeDisplay, redraw).
softhack007
added
the
AI
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Docstrings generation was requested by @srg74.
The following files were modified:
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cppusermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.husermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cppThese file types are not supported
.gitignoreusermods/usermod_v2_gc9a01_display/library.jsonusermods/usermod_v2_gc9a01_display/readme.mdℹ️ Note
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.