Marcel muxio patch 1 #4786
Marcel muxio patch 1 #4786
Conversation
WalkthroughA new usermod for a rotary rack dimmer was added, including its implementation and header files. The build configuration was updated to compile only for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.cpp (1)
9-9: Consider using English for comments and messages.The comments and debug messages are in Dutch/German. For consistency with the WLED codebase, consider using English:
- Serial.println(F("[RotaryRackDimmer] usermod geladen!")); + Serial.println(F("[RotaryRackDimmer] usermod loaded!"));- // Optioneel + // Optional- // Optioneel, laat usermod zelf zijn werk doen + // Optional, let usermod handle its own work- user["RotaryRackDimmer"] = "Actief"; + user["RotaryRackDimmer"] = "Active";Also applies to: 13-13, 17-17, 22-22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
platformio.ini(2 hunks)usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.cpp(1 hunks)usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h(1 hunks)wled00/usermod.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
wled00/usermod.cpp (9)
Learnt from: willmmiles
PR: #4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Learnt from: KrX3D
PR: #4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: KrX3D
PR: #4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use strstr(topic, "/specific/path") instead of strcmp_P(topic, PSTR("/specific/path")) to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern <mqttDeviceTopic>/specific/path.
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, bri is a global variable used for brightness control.
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: blazoncek
PR: #4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
platformio.ini (3)
Learnt from: Jason2866
PR: #4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:15:24.051Z
Learning: In espressif Arduino framework, -fno-lto is set as a standard default in the platformio-build.py script. Therefore, using -fno-lto in build_unflags is not only valid but necessary to remove this default setting before enabling LTO with -flto=auto or -flto=4 in build_flags.
Learnt from: Jason2866
PR: #4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:11:05.896Z
Learning: In PlatformIO, using -fno-lto in build_unflags is a valid pattern to remove potentially existing -fno-lto flags from the build system before adding -flto=auto in build_flags. This ensures that LTO can be enabled without conflicts from default build system flags that might disable LTO.
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.cpp (1)
Learnt from: willmmiles
PR: #4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h (1)
Learnt from: willmmiles
PR: #4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
🔇 Additional comments (5)
wled00/usermod.cpp (1)
2-2: Minor formatting improvement.The addition of a blank line after the include statement improves readability and follows common formatting conventions.
platformio.ini (2)
448-449: USERMOD flag correctly added.The preprocessor definition
-D USERMOD_ROTARYRACKDIMMERis properly added to enable compilation of the new usermod. The placement within the esp32dev environment is appropriate.
13-13: Confirm default_envs restrictionI only see
default_envs = esp32devinplatformio.ini(line 13); all otherdefault_envsentries are in sample or usermods-specific override files, which aren’t applied by default.Please verify that:
- Your CI/release pipeline will import
usermods/platformio_override.usermods.ini(or another override) to build the additional environments, or- You intentionally intend to limit the default build to ESP32 only.
Files to review:
- platformio.ini: line 13
- usermods/platformio_override.usermods.ini (override for other targets)
usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.cpp (1)
1-10: Usermod registration implementation looks correct.The global pointer approach and usermod registration in
userSetup()follows WLED usermod patterns correctly. The instance creation and addition to the usermods list is properly implemented.usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h (1)
12-16: Setup method implementation is correct.The pin initialization with pull-up resistors and initial state reading follows good practices for rotary encoder interfacing.
| void Usermod_RotaryRackDimmer::addToJsonInfo(JsonObject &root) { | ||
| JsonObject user = root["usermod"]; | ||
| user["RotaryRackDimmer"] = "Actief"; | ||
| } |
There was a problem hiding this comment.
Move method implementation to header file.
The addToJsonInfo method implementation should be in the header file since it's part of the class definition. Implementing class methods in separate files outside the class definition violates C++ best practices and can cause linking issues.
Move this implementation to the header file within the class definition:
-void Usermod_RotaryRackDimmer::addToJsonInfo(JsonObject &root) {
- JsonObject user = root["usermod"];
- user["RotaryRackDimmer"] = "Actief";
-}Add to the header file within the class:
void addToJsonInfo(JsonObject &root) override {
JsonObject user = root["usermod"];
user["RotaryRackDimmer"] = "Active";
}🤖 Prompt for AI Agents
In usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.cpp lines 20 to
23, move the implementation of the addToJsonInfo method from the .cpp file to
the header file. Place the method definition inside the class declaration in the
header file, marking it as override, and update the string value to "Active" as
suggested. This ensures the method is inline and avoids potential linking
issues.
| int pinA = 33; // A (CLK) pin van de encoder | ||
| int pinB = 13; // B (DT) pin van de encoder | ||
| int lastState = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make pin assignments and step size configurable.
The hardcoded pin assignments and brightness step should be configurable through the WLED web interface for better usability and hardware flexibility.
class Usermod_RotaryRackDimmer : public Usermod {
private:
- int pinA = 33; // A (CLK) pin van de encoder
- int pinB = 13; // B (DT) pin van de encoder
+ int pinA = 33; // A (CLK) pin of encoder - configurable
+ int pinB = 13; // B (DT) pin of encoder - configurable
+ int stepSize = 5; // Brightness step size - configurable
int lastState = 0;Then implement proper addToConfig and readFromConfig methods to handle these parameters.
🤖 Prompt for AI Agents
In usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h around lines 7
to 9, the pin assignments and step size are hardcoded, limiting flexibility.
Modify the code to make pinA, pinB, and the brightness step size configurable
via the WLED web interface by adding corresponding variables and UI controls.
Then implement the addToConfig and readFromConfig methods to save and load these
parameters from the configuration, ensuring they persist across restarts.
| void loop() override { | ||
| int currentState = digitalRead(pinA); | ||
| if (currentState != lastState) { | ||
| if (digitalRead(pinB) != currentState) { | ||
| // Rechtsom → dim omhoog | ||
| bri = min(255, bri + 5); | ||
| } else { | ||
| // Linksom → dim omlaag | ||
| bri = max(0, bri - 5); | ||
| } | ||
| lastState = currentState; | ||
| colorUpdated(CALL_MODE_DIRECT_CHANGE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add debouncing to prevent erratic behavior.
Rotary encoders are prone to bouncing, which will cause erratic brightness changes. The current implementation lacks debouncing and may trigger multiple times per detent.
Add debouncing with a simple time-based approach:
class Usermod_RotaryRackDimmer : public Usermod {
private:
int pinA = 33;
int pinB = 13;
int lastState = 0;
+ unsigned long lastChangeTime = 0;
+ const unsigned long debounceDelay = 2; // 2ms debounce
void loop() override {
int currentState = digitalRead(pinA);
- if (currentState != lastState) {
+ if (currentState != lastState && (millis() - lastChangeTime) > debounceDelay) {
if (digitalRead(pinB) != currentState) {
- bri = min(255, bri + 5);
+ bri = min(255, bri + stepSize);
} else {
- bri = max(0, bri - 5);
+ bri = max(0, bri - stepSize);
}
lastState = currentState;
+ lastChangeTime = millis();
colorUpdated(CALL_MODE_DIRECT_CHANGE);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h around lines 18
to 31, the loop function handling the rotary encoder input lacks debouncing,
causing erratic brightness changes due to signal bouncing. To fix this,
implement a simple time-based debounce by recording the last time the state
changed and only processing changes if a minimum debounce interval (e.g., 5-10
milliseconds) has passed since the last update. Add a timestamp variable to
track the last update time and check this before updating brightness and
lastState.
| bri = min(255, bri + 5); | ||
| } else { | ||
| // Linksom → dim omlaag | ||
| bri = max(0, bri - 5); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using stepSize variable instead of hardcoded value.
The brightness adjustment uses a hardcoded step of 5, but this should use the configurable stepSize variable for consistency.
if (digitalRead(pinB) != currentState) {
- bri = min(255, bri + 5);
+ bri = min(255, bri + stepSize);
} else {
- bri = max(0, bri - 5);
+ bri = max(0, bri - stepSize);
}🤖 Prompt for AI Agents
In usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h around lines 23
to 26, the brightness adjustment uses a hardcoded step value of 5. Replace this
hardcoded value with the configurable stepSize variable to ensure consistent and
adjustable brightness increments and decrements.
| void addToConfig(JsonObject &root) override {} | ||
| bool readFromConfig(JsonObject &root) override { return true; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper configuration methods.
The configuration methods are currently empty but should handle the configurable parameters (pins and step size) to make the usermod user-friendly.
- void addToConfig(JsonObject &root) override {}
- bool readFromConfig(JsonObject &root) override { return true; }
+ void addToConfig(JsonObject &root) override {
+ JsonObject top = root.createNestedObject(FPSTR("RotaryRackDimmer"));
+ top["pinA"] = pinA;
+ top["pinB"] = pinB;
+ top["stepSize"] = stepSize;
+ }
+
+ bool readFromConfig(JsonObject &root) override {
+ JsonObject top = root[FPSTR("RotaryRackDimmer")];
+ if (top.isNull()) return false;
+
+ pinA = top["pinA"] | pinA;
+ pinB = top["pinB"] | pinB;
+ stepSize = top["stepSize"] | stepSize;
+
+ return true;
+ }📝 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.
| void addToConfig(JsonObject &root) override {} | |
| bool readFromConfig(JsonObject &root) override { return true; } | |
| void addToConfig(JsonObject &root) override { | |
| JsonObject top = root.createNestedObject(FPSTR("RotaryRackDimmer")); | |
| top["pinA"] = pinA; | |
| top["pinB"] = pinB; | |
| top["stepSize"] = stepSize; | |
| } | |
| bool readFromConfig(JsonObject &root) override { | |
| JsonObject top = root[FPSTR("RotaryRackDimmer")]; | |
| if (top.isNull()) return false; | |
| pinA = top["pinA"] | pinA; | |
| pinB = top["pinB"] | pinB; | |
| stepSize = top["stepSize"] | stepSize; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In usermods/usermod_rotaryrackdimmer/usermod_rotaryrackdimmer.h at lines 33-34,
the addToConfig and readFromConfig methods are empty and need to be implemented
to handle the usermod's configurable parameters such as pins and step size.
Modify addToConfig to serialize these parameters into the provided JsonObject,
and update readFromConfig to deserialize and apply these parameters from the
JsonObject, ensuring the usermod can save and restore its configuration
properly.
1 participant
Summary by CodeRabbit
New Features
Chores