Usermod for ESP32-C3 Super Mini OLED 72x40, rebased to v0.16.x#5475
Open
Usermod for ESP32-C3 Super Mini OLED 72x40, rebased to v0.16.x#5475
Conversation
Port of the OLED 72x40 usermod from PR wled#5335 (originally targeting 0_15_x) to the v0.16.x main branch. Addresses all review feedback from softhack007: - Use WLED global I2C pins instead of hardcoded SDA/SCL - Remove WiFi.setTxPower() workaround (use -DLOLIN_WIFI_FIX instead) - Replace blocking delay(5000) splash with non-blocking state machine - Fix strip.isUpdating() guard to prevent display stalling - Remove direct btnPin[]/buttonType[] modification; use configurable buttonIndex with handleButton() instead - Fix bootButtonPin config persistence - Fix brightness graph off-by-one rendering bug - Use getJsonValue() 3-arg form to prevent config defaults drift - Migrate to v0.16.x usermod format (REGISTER_USERMOD, custom_usermods, appendConfigData with oappend) - Assign USERMOD_ID 59 (next available on main)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new WLED usermod "OLED_72x40" that drives a U8g2 SSD1306 I2C display (72x40 UI in 128x64 buffer), implements a non-blocking splash→dashboard state machine, inactivity power-save, optional heartbeat GPIO, persistent config, registration, and a library manifest plus README. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 135-150: The LED heartbeat in loop() currently manipulates ledPin
before validating the usermod state; update loop() so it first returns if
!enabled || !initDone || !u8g2, or alternatively include those same guards in
the LED heartbeat condition, so digitalWrite()/analogWrite() are only called
when the usermod is enabled and initialized; locate the heartbeat code
referencing ledPin and ledTimer inside loop() and add the enabled/initDone/u8g2
checks (or move the heartbeat block below the existing if (!enabled || !initDone
|| !u8g2) return;) to prevent driving a GPIO when initialization failed.
- Around line 90-112: The code is incorrectly using the persisted usermod
setting enabled as a runtime error flag and leaking allocated resources on init
failure; change the init paths in the OLED_72x40 usermod so you do NOT modify
the persisted enabled flag on I2C or display begin() failures (leave enabled
untouched), introduce or use a separate runtime flag such as initialized or
available to reflect whether the display is usable, and on any early failure
after allocating ledPin call PinManager::releasePin(ledPin) (and reset ledPin =
-1) so the heartbeat pin is not permanently reserved; also ensure u8g2 is
deleted and nulled on failure while keeping enabled for config persistence and
use addToConfig() to serialize only the persisted enabled flag.
In `@usermods/OLED_72x40/README.md`:
- Around line 82-91: The example custom_usermods list is not minimal and
incorrectly enables the unrelated audioreactive usermod; update the example so
custom_usermods only includes OLED_72x40 (remove audioreactive) so readers won't
pull in unintended dependencies — edit the README example line containing
custom_usermods and ensure it reads only "custom_usermods = OLED_72x40"
(referencing the custom_usermods, audioreactive, and OLED_72x40 symbols).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1538640b-a040-4371-8f2e-036dd7c41d53
⛔ Files ignored due to path filters (1)
usermods/OLED_72x40/esp32-c3-super-mini.pngis excluded by!**/*.png
📒 Files selected for processing (4)
usermods/OLED_72x40/OLED_72x40.cppusermods/OLED_72x40/README.mdusermods/OLED_72x40/library.jsonwled00/const.h
- Add setupFailed flag to avoid persisting init failures in the enabled config setting (which would silently disable the usermod on next config save) - Deallocate heartbeat LED pin on u8g2->begin() failure to prevent pin leak - Move loop() early-return guard above heartbeat code so GPIO is not driven when usermod failed to initialize - Remove unrelated audioreactive from README custom_usermods example
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 97-103: The code currently mutates the persisted configuration
variable ledPin when allocation/init fails; instead introduce a separate runtime
variable (e.g., allocatedLedPin or runtimeLedPin) and use that for calls to
PinManager::allocatePin(ledPin,...), pinMode(allocatedLedPin,...), and any pin
release logic; do not change the persisted ledPin value so addToConfig()
continues to save the original user setting. Update all places that currently
set ledPin = -1 on allocation/failure or set pinMode/release to operate on the
new runtime variable (keep ledPin read-only for config), and ensure
addToConfig() and any config serialization reference the original ledPin, not
the runtime variable.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 61e8c3cf-c87a-4590-b3b9-ee783dbdfbb7
📒 Files selected for processing (2)
usermods/OLED_72x40/OLED_72x40.cppusermods/OLED_72x40/README.md
Separate ledPin (persisted config value) from activeLedPin (runtime allocation state). Previously, ledPin was set to -1 on allocation failure, which caused addToConfig() to persist -1 and erase the user's configured heartbeat pin.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
usermods/OLED_72x40/OLED_72x40.cpp (1)
71-78: Hoist the repeated defaults into named constants.
60000/60,28,24,0, and8are duplicated between the member initializers and thegetJsonValue()fallbacks. A future tweak to only one side will reintroduce config drift.Based on learnings: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR.
Also applies to: 247-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/OLED_72x40/OLED_72x40.cpp` around lines 71 - 78, Several numeric literals (60000, 28, 24, 0, 8 and the 72 array size) are duplicated between the member initializers (screenTimeoutMS, xOff, yOff, buttonIndex, ledPin, vValues) and the getJsonValue() fallback values; hoist these into named constants (e.g., DEFAULT_SCREEN_TIMEOUT_MS, DEFAULT_SCREEN_TIMEOUT_S if needed, DEFAULT_X_OFFSET, DEFAULT_Y_OFFSET, DEFAULT_BUTTON_INDEX, DEFAULT_LED_PIN, DEFAULT_VVALUES_SIZE) and use those constants both when initializing the members and as the fallback arguments inside getJsonValue(); update any other repeated literals in the same file (including the section referenced around getJsonValue usages) to use the new constants so defaults remain single-source-of-truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 106-114: When releasing the heartbeat LED GPIO (activeLedPin)
ensure the pin is no longer electrically driven before calling
PinManager::deallocatePin; specifically, before the deallocation path in the
u8g2->begin() failure block and the other release path around lines ~258-264,
first stop driving the pin (e.g., drive it to a safe state and set it to input)
and then call PinManager::deallocatePin(activeLedPin, PinOwner::UM_Unspecified),
set activeLedPin = -1, delete u8g2 and set setupFailed = true as currently done;
update both places that currently only call PinManager::deallocatePin to perform
the “stop driving” step first to avoid leaving the GPIO actively driven.
---
Nitpick comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 71-78: Several numeric literals (60000, 28, 24, 0, 8 and the 72
array size) are duplicated between the member initializers (screenTimeoutMS,
xOff, yOff, buttonIndex, ledPin, vValues) and the getJsonValue() fallback
values; hoist these into named constants (e.g., DEFAULT_SCREEN_TIMEOUT_MS,
DEFAULT_SCREEN_TIMEOUT_S if needed, DEFAULT_X_OFFSET, DEFAULT_Y_OFFSET,
DEFAULT_BUTTON_INDEX, DEFAULT_LED_PIN, DEFAULT_VVALUES_SIZE) and use those
constants both when initializing the members and as the fallback arguments
inside getJsonValue(); update any other repeated literals in the same file
(including the section referenced around getJsonValue usages) to use the new
constants so defaults remain single-source-of-truth.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65f2ecfd-544e-419f-a828-2266044ca3b2
📒 Files selected for processing (1)
usermods/OLED_72x40/OLED_72x40.cpp
Add pinMode(activeLedPin, INPUT) before PinManager::deallocatePin() in both failure/reconfig paths so the pin is not left electrically driven after release.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 133-160: When transitioning from the splash to the dashboard in
loop(), reset the inactivity timer so the dashboard isn't immediately turned
off: when you set displayState = DASHBOARD after checking millis() -
splashStartTime >= SPLASH_DURATION, also assign lastInteraction = millis() (or
equivalent) to update the inactivity timestamp; touch the block that uses
splashStartTime, displayState, SPLASH, SPLASH_DURATION, DASHBOARD and
lastInteraction to implement this change.
- Around line 244-269: readFromConfig updates the enabled flag but doesn’t bring
the live hardware into sync; update readFromConfig to handle enabled
transitions: detect oldEnabled vs enabled, and if enabled changed from
false->true call the same initialization path used by setup() (or extract the
setup body into a reusable method e.g., initHardware/initDisplay that sets
initDone, allocates activeLedPin via PinManager and calls pinMode, and
initializes the OLED), and if true->false call a teardown method (e.g.,
deinitHardware/deinitDisplay) that clears initDone, resets/inputs activeLedPin
and deallocates it via PinManager, and ensures display is powered down; reuse
symbols enabled, initDone, setup(), activeLedPin, ledPin, and PinManager to
implement this so enabling after boot or setupFailed initializes correctly and
disabling immediately cleans up hardware before any further reconfiguration.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 40c4230f-702a-4ecb-8d8b-7e8957bc727c
📒 Files selected for processing (1)
usermods/OLED_72x40/OLED_72x40.cpp
- Reset lastInteraction when splash ends so the dashboard isn't immediately blanked if sleepTimeout < 5 seconds - Handle enabled->disabled transition in readFromConfig: power-save the display, release and tristate the heartbeat LED pin - Guard LED pin reallocation with enabled check so pins aren't allocated when usermod is disabled
Saving config triggers a state change event which was waking the display immediately after the teardown disabled it. Add the same enabled/setupFailed/initDone guards to onStateChange() and handleButton() so they are no-ops when the usermod is disabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 192-194: The buffer lineBuffer used with
extractModeName(effectCurrent, JSON_mode_names, lineBuffer, 16) must be
initialized before the call to avoid drawStr reading uninitialized data when
extractModeName returns 0; initialize lineBuffer (e.g., set lineBuffer[0] = '\0'
or zero-fill the 17-byte array) immediately after its declaration (char
lineBuffer[17];) and before calling extractModeName, then call
u8g2->drawStr(currentX, currentY + 7, lineBuffer) as before.
- Around line 215-221: The onStateChange(uint8_t mode) callback (and the other
wake callbacks around lines 224-230) can re-power the display even after
readFromConfig() disabled the usermod; add an early guard that returns
immediately when the usermod is disabled (e.g., if (!enabled) return; or if
(!isEnabled()) return;) before touching u8g2, displayState, lastInteraction, or
lastUpdate so callbacks remain inert while the usermod is disabled.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: efb3be77-4499-48c7-88a7-f877f6eafbbb
📒 Files selected for processing (1)
usermods/OLED_72x40/OLED_72x40.cpp
Zero-initialize lineBuffer so drawStr() doesn't read garbage if extractModeName() returns 0 for an out-of-range mode index.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
usermods/OLED_72x40/OLED_72x40.cpp (1)
123-127: Consider extracting the panel/layout constants.
128,64,72,40, and the fixed y-baselines are repeated in several coordinate calculations. Giving themstatic constexprnames would make the flip/layout math easier to audit.Based on learnings, replacing meaningful magic numbers with named constants improves maintainability and helps avoid layout regressions.
Also applies to: 187-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/OLED_72x40/OLED_72x40.cpp` around lines 123 - 127, Extract the repeated layout numbers into named constants and use them in the flip math: define static constexpr ints like SCREEN_WIDTH (128), SCREEN_HEIGHT (64), PANEL_WIDTH (72), PANEL_HEIGHT (40), LOGO_WIDTH (16) and optional X_BASELINE/Y_BASELINE to replace the raw literals used when computing splashY and textX; update calculations that reference flipDisplay, xOff, yOff, splashY and textX (and the bitmap draw call u8g2->drawBitmap and subsequent layout code around lines 187-209) to use those constants so the flip/layout math is clearer and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 264-286: The code only attempts to allocate activeLedPin when
oldLedPin != ledPin, so if the usermod was disabled (which clears activeLedPin)
but the configured ledPin didn't change you never retry allocation; change the
logic to also attempt allocation whenever enabled && initDone && activeLedPin <
0. Concretely, keep the existing branch that handles pin changes (if oldLedPin
!= ledPin) but add (or replace with) a separate branch that runs when enabled &&
initDone && activeLedPin < 0 && ledPin >= 0 and calls
PinManager::allocatePin(ledPin, true, PinOwner::UM_Unspecified), sets
activeLedPin and pinMode(activeLedPin, OUTPUT) on success; ensure you still
deallocate any previous activeLedPin before allocating (using pinMode(...,
INPUT) and PinManager::deallocatePin) as in the current code paths.
---
Nitpick comments:
In `@usermods/OLED_72x40/OLED_72x40.cpp`:
- Around line 123-127: Extract the repeated layout numbers into named constants
and use them in the flip math: define static constexpr ints like SCREEN_WIDTH
(128), SCREEN_HEIGHT (64), PANEL_WIDTH (72), PANEL_HEIGHT (40), LOGO_WIDTH (16)
and optional X_BASELINE/Y_BASELINE to replace the raw literals used when
computing splashY and textX; update calculations that reference flipDisplay,
xOff, yOff, splashY and textX (and the bitmap draw call u8g2->drawBitmap and
subsequent layout code around lines 187-209) to use those constants so the
flip/layout math is clearer and consistent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63276b47-3a0d-4afb-9a23-3726d8b22d4d
📒 Files selected for processing (1)
usermods/OLED_72x40/OLED_72x40.cpp
Comment on lines
+264
to
+286
| // Handle enabled -> disabled: tear down hardware | ||
| if (initDone && oldEnabled && !enabled) { | ||
| if (activeLedPin >= 0) { | ||
| pinMode(activeLedPin, INPUT); | ||
| PinManager::deallocatePin(activeLedPin, PinOwner::UM_Unspecified); | ||
| activeLedPin = -1; | ||
| } | ||
| if (u8g2) u8g2->setPowerSave(1); | ||
| displayState = OFF; | ||
| } | ||
|
|
||
| // Reallocate LED pin on config change (only when enabled) | ||
| if (enabled && initDone && oldLedPin != ledPin) { | ||
| if (activeLedPin >= 0) { | ||
| pinMode(activeLedPin, INPUT); | ||
| PinManager::deallocatePin(activeLedPin, PinOwner::UM_Unspecified); | ||
| } | ||
| activeLedPin = -1; | ||
| if (ledPin >= 0 && PinManager::allocatePin(ledPin, true, PinOwner::UM_Unspecified)) { | ||
| activeLedPin = ledPin; | ||
| pinMode(activeLedPin, OUTPUT); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Retry activeLedPin allocation when the configured GPIO didn't change.
The disable path clears activeLedPin, but the reallocation path only runs when oldLedPin != ledPin. If the original allocation failed, or the user toggles the usermod off and back on with the same configured pin, the heartbeat stays off until reboot or another pin edit.
🔧 Minimal fix
- if (enabled && initDone && oldLedPin != ledPin) {
+ if (enabled && initDone && (oldLedPin != ledPin || (ledPin >= 0 && activeLedPin < 0))) {
if (activeLedPin >= 0) {
pinMode(activeLedPin, INPUT);
PinManager::deallocatePin(activeLedPin, PinOwner::UM_Unspecified);
}
activeLedPin = -1;
if (ledPin >= 0 && PinManager::allocatePin(ledPin, true, PinOwner::UM_Unspecified)) {
activeLedPin = ledPin;
pinMode(activeLedPin, OUTPUT);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/OLED_72x40/OLED_72x40.cpp` around lines 264 - 286, The code only
attempts to allocate activeLedPin when oldLedPin != ledPin, so if the usermod
was disabled (which clears activeLedPin) but the configured ledPin didn't change
you never retry allocation; change the logic to also attempt allocation whenever
enabled && initDone && activeLedPin < 0. Concretely, keep the existing branch
that handles pin changes (if oldLedPin != ledPin) but add (or replace with) a
separate branch that runs when enabled && initDone && activeLedPin < 0 && ledPin
>= 0 and calls PinManager::allocatePin(ledPin, true, PinOwner::UM_Unspecified),
sets activeLedPin and pinMode(activeLedPin, OUTPUT) on success; ensure you still
deallocate any previous activeLedPin before allocating (using pinMode(...,
INPUT) and PinManager::deallocatePin) as in the current code paths.
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
2 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.
Port of the OLED 72x40 usermod from PR #5335 (originally targeting 0_15_x) to the v0.16.x main branch. Addresses all review feedback from softhack007:
Summary by CodeRabbit
New Features
Documentation
Chores