-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Usermod for ESP32C3 Super Mini OLED 72x40 #5335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 0_15_x
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
|
@doccaz is there a special reason why you made your PR against the normally we expect PRs to be made against the |
| void setup() { | ||
| pinMode(LED_PIN, OUTPUT); | ||
| Wire.begin(OLED_SDA, OLED_SCL); | ||
| WiFi.setTxPower(WIFI_POWER_8_5dBm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are known issues with the WiFi in this board that can make it quite unuseable. But it doesn't happen on all environments, maybe this should be a configuration option, I guess.
Reference: https://www.youtube.com/watch?v=UHTdhCrSA3g
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, actually its already an option, so you don't need to repeat that in your UM
Lines 379 to 380 in 6b953d9
| #if defined(LOLIN_WIFI_FIX) && (defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32S3)) | |
| WLED_GLOBAL uint8_t txPower _INIT(WIFI_POWER_8_5dBm); |
just add -DLOLIN_WIFI_FIX to your build flags.
| public: | ||
| void setup() { | ||
| pinMode(LED_PIN, OUTPUT); | ||
| Wire.begin(OLED_SDA, OLED_SCL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be necessary, if you use the usermod global I2C pins.
| if (displayOff && u8g2) { u8g2->setPowerSave(0); displayOff = false; } | ||
| } | ||
|
|
||
| bool handleButton(uint8_t b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will react on all buttons defined in the LEDs setting page, is this what you want?
| else u8g2->print(cmDNS); | ||
|
|
||
| u8g2->sendBuffer(); | ||
| delay(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, such a long delay in the usermod setup function is a no-go.
This will delay the main loop for 5 seconds. Please find a different way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It was just for showing a silly "bootsplash", I'll think of some other way.
| } | ||
|
|
||
| // 3. DRAWING LOGIC (Bypassed if screen is blanked) | ||
| if (strip.isUpdating()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might stall your usermod completely if someone has lots of LEDs. better:
if (strip.isUpdating() && (millis() - lastUpdate < 100)) return;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted! Thanks for taking the time to review this so thoroughly. First time contributing to WLED.
|
|
||
| public: | ||
| void setup() { | ||
| pinMode(LED_PIN, OUTPUT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use pin manager to check if the pin is free before writing to it.
| // Apply the button pin 9 default logic | ||
| int pin = top[F("bootButtonPin")] | 9; | ||
| if (btnPin[0] == -1 || btnPin[0] != pin) { | ||
| btnPin[0] = pin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usermods should not interfere with the button handling code in button.cpp.
Its OK to read from the global btnPin[] and buttonType[] arrays. However modifying these arrays "behind the back" of the WLED core invites trouble.
Edit: It might be better to just make the "button index" (from LEDs preferences) configurable in your UM settings, and leave the button handling (and de-bouncing) to the wled core. You'll still receive the handleButton(b) event, and you could simply compare if b is the configured button index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@usermods/OLED_72x40/UsermodOLED72x40.h`:
- Around line 199-229: The addToConfig() currently writes a literal 9 so UI
changes are never persisted; introduce a class member int bootButtonPin = 9 and
use that member in addToConfig() instead of the hardcoded 9, update
readFromConfig() to read into bootButtonPin (e.g. bootButtonPin =
top["bootButtonPin"] | 9) and then use bootButtonPin when assigning btnPin[0]
(instead of reading pin from config and not saving it back to the member),
ensuring the new member is declared in the UsermodOLED72x40 class so the value
is preserved across config read/write and used when initializing buttonType and
btnPin[0].
- Around line 159-163: The brightness graph code fills vValues[71] but the
drawing loop only iterates i < 71 so the newest sample at index 71 is never
rendered; update the render loop in the UsermodOLED72x40 (the block that calls
u8g2->drawLine with currentX/currentY and vValues[i]) to include index 71 (e.g.,
iterate i < 72 or i <= 71) so vValues[71] is drawn, keeping the existing shift
logic (for (int i = 0; i < 71; i++) vValues[i] = vValues[i+1]) and the
assignment vValues[71] = map(bri, 0, 255, 0, 15).
🧹 Nitpick comments (1)
usermods/OLED_72x40/UsermodOLED72x40.h (1)
213-216: Avoid re-hardcoding default offsets in config load.This duplicates defaults and risks drift if defaults change later.
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.♻️ Suggested cleanup
- xOff = top[F("x-offset")] | 28; - yOff = top[F("y-offset")] | 24; + xOff = top[F("x-offset")] | xOff; + yOff = top[F("y-offset")] | yOff;
| for (int i = 0; i < 71; i++) vValues[i] = vValues[i+1]; | ||
| vValues[71] = map(bri, 0, 255, 0, 15); | ||
| for (int i = 0; i < 71; i++) { | ||
| u8g2->drawLine(currentX + i, currentY + 26, currentX + i, currentY + 26 - vValues[i]); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brightness graph drops the newest sample.
vValues[71] is populated but never rendered.
🐛 Proposed fix
- for (int i = 0; i < 71; i++) {
+ for (int i = 0; i < 72; i++) {
u8g2->drawLine(currentX + i, currentY + 26, currentX + i, currentY + 26 - vValues[i]);
}🤖 Prompt for AI Agents
In `@usermods/OLED_72x40/UsermodOLED72x40.h` around lines 159 - 163, The
brightness graph code fills vValues[71] but the drawing loop only iterates i <
71 so the newest sample at index 71 is never rendered; update the render loop in
the UsermodOLED72x40 (the block that calls u8g2->drawLine with currentX/currentY
and vValues[i]) to include index 71 (e.g., iterate i < 72 or i <= 71) so
vValues[71] is drawn, keeping the existing shift logic (for (int i = 0; i < 71;
i++) vValues[i] = vValues[i+1]) and the assignment vValues[71] = map(bri, 0,
255, 0, 15).
| void addToConfig(JsonObject& root) { | ||
| JsonObject top = root.createNestedObject(F("OLED_72x40")); | ||
| top[F("enabled")] = enabled; | ||
| top[F("flipDisplay")] = flipDisplay; // Registration for UI | ||
| top["bootButtonPin"] = 9; | ||
| top["x-offset"] = xOff; | ||
| top["y-offset"] = yOff; | ||
| top["sleepTimeout"] = screenTimeoutMS / 1000; | ||
| } | ||
|
|
||
| bool readFromConfig(JsonObject& root) { | ||
| JsonObject top = root[F("OLED_72x40")]; | ||
| if (top.isNull()) return false; | ||
|
|
||
| enabled = top[F("enabled")] | enabled; | ||
| flipDisplay = top[F("flipDisplay")] | flipDisplay; | ||
| xOff = top[F("x-offset")] | 28; | ||
| yOff = top[F("y-offset")] | 24; | ||
|
|
||
| // Guard against 0 or negative timeout values | ||
| unsigned long timeoutSec = top[F("sleepTimeout")] | 60; | ||
| screenTimeoutMS = timeoutSec * 1000; | ||
|
|
||
| if (initDone && u8g2) u8g2->setFlipMode(flipDisplay ? 1 : 0); | ||
|
|
||
| // Apply the button pin 9 default logic | ||
| int pin = top[F("bootButtonPin")] | 9; | ||
| if (btnPin[0] == -1 || btnPin[0] != pin) { | ||
| btnPin[0] = pin; | ||
| buttonType[0] = BTN_TYPE_PUSH; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bootButtonPin setting is never persisted.
addToConfig() always writes 9, so UI changes are lost on save.
🛠️ Proposed fix
- top["bootButtonPin"] = 9;
+ top["bootButtonPin"] = bootButtonPin;
@@
- int pin = top[F("bootButtonPin")] | 9;
- if (btnPin[0] == -1 || btnPin[0] != pin) {
- btnPin[0] = pin;
+ bootButtonPin = top[F("bootButtonPin")] | bootButtonPin;
+ if (btnPin[0] == -1 || btnPin[0] != bootButtonPin) {
+ btnPin[0] = bootButtonPin;
buttonType[0] = BTN_TYPE_PUSH;
}// Add alongside other member fields
int bootButtonPin = 9;🤖 Prompt for AI Agents
In `@usermods/OLED_72x40/UsermodOLED72x40.h` around lines 199 - 229, The
addToConfig() currently writes a literal 9 so UI changes are never persisted;
introduce a class member int bootButtonPin = 9 and use that member in
addToConfig() instead of the hardcoded 9, update readFromConfig() to read into
bootButtonPin (e.g. bootButtonPin = top["bootButtonPin"] | 9) and then use
bootButtonPin when assigning btnPin[0] (instead of reading pin from config and
not saving it back to the member), ensuring the new member is declared in the
UsermodOLED72x40 class so the value is preserved across config read/write and
used when initializing buttonType and btnPin[0].
Sorry, no particular reason at all. It was just the branch I had locally while testing this code. I'll work the changes for the main branch. |
This is a usermod built against the 0.15.3 branch that adds support for the ESP32 C3 Super Mini version with a 0.42" 72x40 OLED screen.
I added a few configuration options and quality-of-life improvements if you're using one of these in your project:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.