Usermod I2C Rotary Encoder #5243
Conversation
WalkthroughAdds a new I2C encoder button usermod (DUPPA I2CEncoder V2.1) to WLED: docs, PlatformIO sample, library manifest, a new v2 usermod implementation with brightness/effect/button behaviors and config, and a new usermod ID macro. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 5
🧹 Nitpick comments (1)
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp (1)
104-127: Consider guarding setup() with enabled check and using WLED's I2C initialization.
setup()initializes the encoder hardware even whenenabledisfalse, which wastes resources and may cause issues if no hardware is connected.Calling
Wire.begin(sdaPin, sclPin)directly may conflict with other I2C usermods or WLED's global I2C bus. Consider using WLED'sPinManagerto allocate pins and checking if I2C is already initialized.🔎 Proposed fix
void setup() override { + if (!enabled) return; // (Re)initialize encoder with current config if (encoder_p) delete encoder_p; encoder_p = new i2cEncoderLibV2(i2cAddress); + if (!PinManager::allocatePin(intPin, false, PinOwner::UM_Unspecified) || + !PinManager::allocatePin(sdaPin, false, PinOwner::HW_I2C) || + !PinManager::allocatePin(sclPin, false, PinOwner::HW_I2C)) { + enabled = false; + return; + } pinMode(intPin, INPUT); Wire.begin(sdaPin, sclPin);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
usermods/i2c_encoder_button/README.mdusermods/i2c_encoder_button/library.jsonusermods/i2c_encoder_button/platformio_override.sample.iniusermods/i2c_encoder_button/usermod_i2c_encoder_button.cppwled00/const.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.h
🧠 Learnings (7)
📓 Common learnings
Learnt from: willmmiles
Repo: wled/WLED 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.
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED 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.
Applied to files:
wled00/const.husermods/i2c_encoder_button/README.mdusermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/const.h
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: 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. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/const.h
📚 Learning: 2025-11-30T15:29:00.726Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
wled00/const.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to platformio.ini : Use platformio.ini as the single source of truth for hardware build targets and settings
Applied to files:
usermods/i2c_encoder_button/platformio_override.sample.ini
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
usermods/i2c_encoder_button/library.jsonusermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
🪛 LanguageTool
usermods/i2c_encoder_button/README.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # usermod_i2c_encoder This usermod enables the use of a [DUPPA I2CEncoder ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~25-~25: Ensure spelling is correct
Context: ...RGB LED to illuminate the shaft and any know attached to it. - This is the encoder...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
usermods/i2c_encoder_button/README.md
26-26: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
wled00/const.h (1)
210-210: LGTM!The new usermod ID follows the sequential numbering convention and the comment format is consistent with existing entries. Based on learnings, hardcoded usermod IDs are intentional in WLED.
usermods/i2c_encoder_button/platformio_override.sample.ini (1)
1-19: LGTM!The sample configuration is well-documented with helpful comments about pin conflicts. The environment structure properly extends base configurations and demonstrates both ESP8266 and ESP32 setups.
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp (1)
141-171: LGTM!The JSON configuration methods follow WLED conventions, and the usermod registration is correct. The
getId()properly returns the constant defined inconst.h.
|
Working through coderabbit suggestions... Give me a few days to polish and test. |
|
@broccoliboy some automated builds are failing, looks like there is still a typo in your code |
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp:
- Line 107: Remove the redundant Wire.begin(sdaPin, sclPin) call from the
usermod setup() (the re-initialization of the global Wire bus); rely on WLED's
global Wire initialization instead and delete that single Wire.begin(...) line
so you don't conflict with cfg.cpp/set.cpp warnings about bus re-initialisation
and Wire.setPins() failing when already initialized.
- Around line 18-20: The default I2C address macro I2C_ENCODER_DEFAULT_ADDRESS
is currently set to 0x00; update the definition in
usermod_i2c_encoder_button.cpp so the macro I2C_ENCODER_DEFAULT_ADDRESS is
defined as 0x20 (7-bit default address for DUPPA I2CEncoder V2.1) to match the
stock hardware.
🧹 Nitpick comments (1)
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp (1)
44-46: Consider using PinManager for pin allocation.WLED usermods typically use
PinManagerto allocate GPIO pins, ensuring they aren't used by multiple components. This would prevent conflicts with LED outputs or other usermods.Example pattern
In
setup(), after verifyingenabled:// Allocate pins if (!pinManager.allocatePin(intPin, true, PinOwner::UM_I2CEncoder) || !pinManager.allocatePin(sdaPin, true, PinOwner::UM_I2CEncoder) || !pinManager.allocatePin(sclPin, true, PinOwner::UM_I2CEncoder)) { enabled = false; return; // Pin allocation failed }And deallocate in a cleanup method or when disabling.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/i2c_encoder_button/README.mdusermods/i2c_encoder_button/platformio_override.sample.iniusermods/i2c_encoder_button/usermod_i2c_encoder_button.cppwled00/const.h
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/i2c_encoder_button/platformio_override.sample.ini
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.h
🧠 Learnings (9)
📓 Common learnings
Learnt from: willmmiles
Repo: wled/WLED 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.
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED 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.
Applied to files:
wled00/const.husermods/i2c_encoder_button/README.mdusermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/const.husermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: 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. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/const.husermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2025-11-30T15:29:00.726Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
wled00/const.h
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
📚 Learning: 2026-01-01T13:18:14.738Z
Learnt from: softhack007
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:225-227
Timestamp: 2026-01-01T13:18:14.738Z
Learning: In WLED effects, when computing time differences between unsigned long timestamps (e.g., `strip.now`, `lastBumpUpdate`), always cast the subtraction result to `int` before converting to `float` to ensure correct behavior on ESP32-C3 (RISC-V). Use pattern: `float(int(timestamp1 - timestamp2))` instead of `float(timestamp1 - timestamp2)`.
Applied to files:
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp
🪛 LanguageTool
usermods/i2c_encoder_button/README.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # usermod_i2c_encoder This usermod enables the use of a [DUPPA I2CEncoder ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (4)
wled00/const.h (1)
210-210: LGTM! Usermod ID follows conventions.The new
USERMOD_ID_I2C_ENCODER_BUTTONconstant is correctly added with the next sequential ID (59) and follows the established naming pattern with the_ID_infix. The comment properly references the usermod name.usermods/i2c_encoder_button/README.md (1)
1-37: Documentation looks good.The README clearly documents the usermod's features, hardware requirements, and compilation instructions. Previous issues (typo and bare URL) have been resolved.
usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp (2)
126-138: LGTM! Loop implementation is sound.The
loop()method correctly guards against nullencoder_pand disabled state (resolving a past review concern). The interrupt-driven approach with mode timeout provides good UX.
165-165: LGTM! Usermod ID is now correct.The
getId()method correctly returnsUSERMOD_ID_I2C_ENCODER_BUTTON, matching the constant defined inwled00/const.h. This resolves the compiler error mentioned in the PR comments.
| if (encoder_p) delete encoder_p; | ||
| encoder_p = new i2cEncoderLibV2(i2cAddress); | ||
| pinMode(intPin, INPUT); | ||
| Wire.begin(sdaPin, sclPin); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Wire.begin usage patterns in WLED usermods
rg -n -C 3 'Wire\.begin' --type cpp --type hRepository: wled/WLED
Length of output: 9935
🏁 Script executed:
# Get the specific file and its context around line 107
cat -n usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp | sed -n '95,120p'Repository: wled/WLED
Length of output: 1282
🏁 Script executed:
# Check for global Wire initialization in main files
rg -n 'Wire\.begin' --type cpp --type h | grep -v 'usermods' | head -20Repository: wled/WLED
Length of output: 981
Avoid re-initializing Wire in usermod setup().
Calling Wire.begin(sdaPin, sclPin) is redundant; WLED initializes Wire globally once in wled00/cfg.cpp. Re-initialization conflicts with the documented WLED architecture (see wled00/set.cpp line 671: "no bus re-initialisation as usermods do not get any notification"). Additionally, cfg.cpp line 475 contains an explicit warning that Wire.setPins() fails if Wire is already initialized. Remove this call and rely on the global Wire initialization instead.
🤖 Prompt for AI Agents
In @usermods/i2c_encoder_button/usermod_i2c_encoder_button.cpp around line 107,
Remove the redundant Wire.begin(sdaPin, sclPin) call from the usermod setup()
(the re-initialization of the global Wire bus); rely on WLED's global Wire
initialization instead and delete that single Wire.begin(...) line so you don't
conflict with cfg.cpp/set.cpp warnings about bus re-initialisation and
Wire.setPins() failing when already initialized.
|
CI is passing now, but please don't merge until I have some time to test on hardware. Thanks! |
|
I am wondering why:
|
|
Good questions, and thank you for the comment. For one, I originally wrote this usermod long before global i2c existed, but now that I'm trying to get it merged I plan on updating to use recent wled features like global i2c and pinmanager. This is a work in progress... I'm okay waiting on merge until I get this figured out. For the second, this usermod uses i2c to interface with an external micro that's connected to the rotary encoder. I didn't reimplement any encoder logic. If there is an existing usermod that does this or has functions I can make use of, please let me know. I don't want to reinvent any wheels ;) |
3 participants
This usermod enables the use of a DUPPA I2CEncoder V2.1 rotary encoder + pushbutton to control WLED.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.