Define custom pin mappings in wled.h#370
Conversation
|
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:
Walkthrough
ChangesHardware Configuration for ESP32-S3 Variant
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/wled.h`:
- Around line 1-17: The pin definitions (R1_PIN, G1_PIN, B1_PIN, R2_PIN, G2_PIN,
B2_PIN, A_PIN, B_PIN, C_PIN, D_PIN, E_PIN, LAT_PIN, OE_PIN, CLK_PIN) in the
WLED_MY_PINS block do not match the HUB75 pin order documented in
bus_manager.cpp. Verify the correct pin mapping against the board schematic and
update each pin definition to match the proper HUB75 wiring order. Additionally,
if this header file is used by multiple build targets, scope the WLED_MY_PINS
block with a conditional preprocessor directive to apply these pin definitions
only to the Matrixboard build.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b08eb68-bebb-4ce5-8929-9f6fb4de6d39
📒 Files selected for processing (1)
wled00/wled.h
| #ifndef WLED_MY_PINS | ||
| #define WLED_MY_PINS | ||
| #define R1_PIN 42 | ||
| #define G1_PIN 41 | ||
| #define B1_PIN 40 | ||
| #define R2_PIN 39 | ||
| #define G2_PIN 38 | ||
| #define B2_PIN 45 | ||
| #define A_PIN 1 | ||
| #define B_PIN 2 | ||
| #define C_PIN 3 | ||
| #define D_PIN 4 | ||
| #define E_PIN 5 | ||
| #define LAT_PIN 6 | ||
| #define OE_PIN 7 | ||
| #define CLK_PIN 8 | ||
| #endif |
There was a problem hiding this comment.
Align this pinout with the Matrixboard wiring contract.
These values don’t match the HUB75 pin order documented in bus_manager.cpp, so the matrix driver will be wired incorrectly on the target board. Please verify the mapping against the actual board schematic, and scope this block to the Matrixboard build only if this header is shared by other targets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/wled.h` around lines 1 - 17, The pin definitions (R1_PIN, G1_PIN,
B1_PIN, R2_PIN, G2_PIN, B2_PIN, A_PIN, B_PIN, C_PIN, D_PIN, E_PIN, LAT_PIN,
OE_PIN, CLK_PIN) in the WLED_MY_PINS block do not match the HUB75 pin order
documented in bus_manager.cpp. Verify the correct pin mapping against the board
schematic and update each pin definition to match the proper HUB75 wiring order.
Additionally, if this header file is used by multiple build targets, scope the
WLED_MY_PINS block with a conditional preprocessor directive to apply these pin
definitions only to the Matrixboard build.
There was a problem hiding this comment.
@coderabbitai it's even worse - the pins defined here seem to be HUB75 pins, but the HUB75 driver will not use these values.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the HUB75 driver or related code references R1_PIN, G1_PIN etc.
rg -n 'R1_PIN|G1_PIN|B1_PIN|R2_PIN|G2_PIN|B2_PIN|A_PIN|LAT_PIN|OE_PIN|CLK_PIN' --type=cpp --type=h -C2
echo "=== HUB75 usermod/driver pin config ==="
rg -n 'HUB75|hub75|ESP32-HUB75' -l
echo "=== Look for pin config structs in HUB75 driver ==="
rg -n 'mxconfig|MatrixConfig|HUB75_I2S_CFG|pinConfig|PIN_' --type=cpp --type=h -C2 | head -80Length of output: 22584
@softhack007 — confirmed. The smoking gun is in bus_manager.cpp lines 803–804:
// HUB75_I2S_CFG::i2s_pins _pins={R1_PIN, G1_PIN, B1_PIN, R2_PIN, G2_PIN, B2_PIN, A_PIN, B_PIN, C_PIN, D_PIN, E_PIN, LAT_PIN, OE_PIN, CLK_PIN};
mxconfig.gpio = { 1, 5, 6, 7, 13, 9, 16, 48, 47, 21, 38, 8, 4, 18 };The R1_PIN/G1_PIN/… form was once the intended interface (hence the commented-out line), but the driver now hard-codes its GPIO assignment directly and never reads these macros. A grep across the entire repo shows zero live uses of those defines outside of wled.h itself — they are completely dead code. The whole WLED_MY_PINS block as written has no effect on any hardware configuration.
The PR should either:
- Remove the block entirely if it's left-over scaffolding, or
- Wire the defines into
bus_manager.cppby replacing the hardcodedmxconfig.gpioinitialiser with the_pinsstruct that was already sketched out in the comment, and guard both sides with the board-specific build target.
(́◉◞౪◟◉‵)
✏️ Learnings added
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 370
File: wled00/wled.h:1-17
Timestamp: 2026-06-20T22:10:37.043Z
Learning: In WLED-MM (MoonModules/WLED-MM), the HUB75 matrix driver pin configuration in `wled00/bus_manager.cpp` is hardcoded directly in `mxconfig.gpio = { ... }` and does NOT read from preprocessor defines such as R1_PIN, G1_PIN, B1_PIN, R2_PIN, G2_PIN, B2_PIN, A_PIN, B_PIN, C_PIN, D_PIN, E_PIN, LAT_PIN, OE_PIN, CLK_PIN. A commented-out line at bus_manager.cpp line 803 shows the former intended interface using those defines, but the live code ignores them. Any WLED_MY_PINS block defining these macros in wled.h is therefore dead code with no effect on the matrix hardware.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platformio.ini`:
- Line 108: The environment name esp32s3_16MB_OPI is referenced in the
default_envs list but lacks a corresponding environment section definition in
platformio.ini. To fix this, you must either add a new [env:esp32s3_16MB_OPI]
section with the appropriate board configuration and build settings to match the
other environment definitions in the file, or remove esp32s3_16MB_OPI from the
default_envs list if this environment is not needed. Choose the appropriate
action based on whether you intend to support building for this ESP32-S3
variant.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cf2e36b-62e3-4f7d-bf81-6f17765c2ff4
📒 Files selected for processing (1)
platformio.ini
|
@kelteks78 this looks like your personal configuration, are you sure you want this merged into the main WLEDMM repo? |
There was a problem hiding this comment.
Please revert all changes in platformio.ini.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platformio.ini (2)
1-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMaintainer has explicitly requested reverting all platformio.ini changes.
A comment from maintainer softhack007 in the existing review thread states: "Please revert all changes in platformio.ini." This is a clear signal that these modifications should not proceed in their current form. This PR is in draft status and should be discussed with the maintainer before advancing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platformio.ini` around lines 1 - 108, The maintainer has explicitly requested that all changes to platformio.ini be reverted and the file be restored to its original state prior to any modifications. Remove all edits made to platformio.ini and restore the file to match the version before these changes were introduced. This change should be discussed with the maintainer before proceeding further.
108-108:⚠️ Potential issue | 🔴 CriticalAdd missing
[env:esp32s3_16MB_OPI]environment section or remove fromdefault_envs.Line 108 adds
esp32s3_16MB_OPIto the activedefault_envslist, but no corresponding[env:esp32s3_16MB_OPI]section exists in platformio.ini. This will cause PlatformIO build failures. Define the environment section with appropriate settings or remove the reference from default_envs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platformio.ini` at line 108, The platformio.ini file references esp32s3_16MB_OPI in the default_envs list on line 108, but there is no corresponding [env:esp32s3_16MB_OPI] environment section defined in the file. Either create a new [env:esp32s3_16MB_OPI] section with the appropriate PlatformIO configuration settings (board, framework, build flags, etc.) matching your project requirements, or remove esp32s3_16MB_OPI from the default_envs list if this environment is not needed for your builds.
🧹 Nitpick comments (1)
platformio.ini (1)
14-45: ⚡ Quick winMissing attribution for hardware pin mappings and build environment changes.
Per coding guidelines, custom hardware pin mappings and build configurations should document their inspiration and sources (with links to relevant GitHub repositories or documentation). The changes here—adding pin definitions for "Matrixboard" hardware and a new ESP32-S3 build variant—lack any attribution, comments explaining their purpose, or links to hardware specifications.
Consider adding comments explaining the hardware target, relevant design constraints, and any upstream inspiration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platformio.ini` around lines 14 - 45, The platformio.ini file lacks documentation for custom hardware configurations and build environment changes. Add comments above or adjacent to environment definitions that explain the hardware target, design purpose, and any upstream inspiration or sources. Specifically, for environment entries related to "Matrixboard" hardware configurations and the ESP32-S3 build variants (such as esp32s3dev_16MB_PSRAM_opi and esp32s3_16MB_OPI), include detailed comments that reference the relevant GitHub repositories, hardware specification links, and design constraints. Ensure each custom pin mapping or build configuration includes clear attribution and purpose documentation as per coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@platformio.ini`:
- Around line 1-108: The maintainer has explicitly requested that all changes to
platformio.ini be reverted and the file be restored to its original state prior
to any modifications. Remove all edits made to platformio.ini and restore the
file to match the version before these changes were introduced. This change
should be discussed with the maintainer before proceeding further.
- Line 108: The platformio.ini file references esp32s3_16MB_OPI in the
default_envs list on line 108, but there is no corresponding
[env:esp32s3_16MB_OPI] environment section defined in the file. Either create a
new [env:esp32s3_16MB_OPI] section with the appropriate PlatformIO configuration
settings (board, framework, build flags, etc.) matching your project
requirements, or remove esp32s3_16MB_OPI from the default_envs list if this
environment is not needed for your builds.
---
Nitpick comments:
In `@platformio.ini`:
- Around line 14-45: The platformio.ini file lacks documentation for custom
hardware configurations and build environment changes. Add comments above or
adjacent to environment definitions that explain the hardware target, design
purpose, and any upstream inspiration or sources. Specifically, for environment
entries related to "Matrixboard" hardware configurations and the ESP32-S3 build
variants (such as esp32s3dev_16MB_PSRAM_opi and esp32s3_16MB_OPI), include
detailed comments that reference the relevant GitHub repositories, hardware
specification links, and design constraints. Ensure each custom pin mapping or
build configuration includes clear attribution and purpose documentation as per
coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 521efba3-2ae8-473b-be1a-c7bc24a5845b
📒 Files selected for processing (1)
platformio.ini
2 participants
Matrixboard
Summary by CodeRabbit
Chores
Build