Hub75 fixes #4950
Hub75 fixes #4950
Conversation
WalkthroughPlatformIO sample updated to add LED_TYPES=65 and WLED_DEBUG_BUS and remove NO_CIE1931 from HUB75 envs. bus_manager.cpp adjusted HUB75 sizing/types to use uint8_t, disabled panel chaining (commented), and removed unGamma24 color-correction; bus_manager.h returns 3 pins for HUB75 in getNumberOfPins. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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 |
| setBitInArray(_ledsDirty, pix, c != IS_BLACK); // dirty = true means "color is not BLACK" | ||
|
|
||
| #ifndef NO_CIE1931 | ||
| c = unGamma24(c); // to use the driver linear brightness feature, we first need to undo WLED gamma correction |
There was a problem hiding this comment.
@DedeHai - I'm not sure if I'm making the right change here, I've not been able to follow all the ins and outs about where Gamma correction should be applied, in WLED-MM we do use NO_CIE1931, but with the current WLED I was seeing issues with this flag, but then we don't have an unGamma24 in the current WLED codebase
There was a problem hiding this comment.
I implemented "unGamma" but its either 8bit or 32bit. do HUB75 have built in gamma correction or why is that needed? if undoing gamma, colors will be all weird on linear panels.
There was a problem hiding this comment.
but let me worry about that after this is in main, the whole brightness/gamma/ungamma took me a long time to fix properly. All I need to know is what the HUB75 driver does to raw colors.
There was a problem hiding this comment.
i.e. does it scale the brightness or apply any gamma and if so can we disable it as that's what we need to do in WLED code.
There was a problem hiding this comment.
By default the driver is already non linear, check the docs for the NO_CIE1931 flag
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/bus_manager.cpp (2)
814-840: VirtualMatrixPanel constructed before ‘display’ exists (null deref risk).In QS branch, virtualDisp is created with (*display) before display is allocated at Line 937.
Apply this diff: create VirtualMatrixPanel after display->begin() succeeds, and remove the early construction.
@@ - } else if (bc.type == TYPE_HUB75MATRIX_QS) { - mxconfig.mx_width = min((uint8_t) 64, bc.pins[0]) * 2; - mxconfig.mx_height = min((uint8_t) 64, bc.pins[1]) / 2; - virtualDisp = new VirtualMatrixPanel((*display), 1, 1, bc.pins[0], bc.pins[1]); - virtualDisp->setRotation(0); - switch(bc.pins[1]) { - case 16: - virtualDisp->setPhysicalPanelScanRate(FOUR_SCAN_16PX_HIGH); - break; - case 32: - virtualDisp->setPhysicalPanelScanRate(FOUR_SCAN_32PX_HIGH); - break; - case 64: - virtualDisp->setPhysicalPanelScanRate(FOUR_SCAN_64PX_HIGH); - break; - default: - DEBUGBUS_PRINTLN("Unsupported height"); - return; - } - } else { + } else if (bc.type == TYPE_HUB75MATRIX_QS) { + mxconfig.mx_width = min((uint8_t)64, bc.pins[0]) * 2; + mxconfig.mx_height = min((uint8_t)64, bc.pins[1]) / 2; + } else { DEBUGBUS_PRINTLN("Unknown type"); return; } @@ - else { + else { DEBUGBUS_PRINTLN("MatrixPanel_I2S_DMA begin ok"); @@ _valid = true; display->clearScreen(); // initially clear the screen buffer DEBUGBUS_PRINTLN("MatrixPanel_I2S_DMA clear ok"); + + // Create virtual panel mapping only after display exists + if (bc.type == TYPE_HUB75MATRIX_QS) { + virtualDisp = new VirtualMatrixPanel((*display), 1, 1, bc.pins[0], bc.pins[1]); + virtualDisp->setRotation(0); + switch (bc.pins[1]) { + case 16: virtualDisp->setPhysicalPanelScanRate(FOUR_SCAN_16PX_HIGH); break; + case 32: virtualDisp->setPhysicalPanelScanRate(FOUR_SCAN_32PX_HIGH); break; + case 64: virtualDisp->setPhysicalPanelScanRate(FOUR_SCAN_64PX_HIGH); break; + default: + DEBUGBUS_PRINTLN("Unsupported height"); + _valid = false; + return; + } + }
845-858: Bit‑depth heuristic runs before chain_length is set.At Lines 845–851 you use mxconfig.chain_length, but it’s only assigned at Line 853. For chains >1, bit depth selection will be wrong.
@@ -#if defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S2)// classic esp32, or esp32-s2: reduce bitdepth for large panels + // Clamp chain length before using it in heuristics + mxconfig.chain_length = max((uint8_t)1, min(bc.pins[2], (uint8_t)4)); // prevent bad data preventing boot due to low memory + +#if defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S2)// classic esp32, or esp32-s2: reduce bitdepth for large panels @@ -#endif - - mxconfig.chain_length = max((uint8_t) 1, min(bc.pins[2], (uint8_t) 4)); // prevent bad data preventing boot due to low memory +#endif
🧹 Nitpick comments (2)
platformio_override.sample.ini (1)
552-555: Make WLED_DEBUG_BUS opt-in in sample config.Leaving WLED_DEBUG_BUS enabled in a sample env can flood serial and degrade FPS. Recommend commenting it out by default and documenting when to enable.
-D WLED_ENABLE_HUB75MATRIX -D NO_GFX - -D LED_TYPES=65 - -D WLED_DEBUG_BUS + -D LED_TYPES=65 + ; -D WLED_DEBUG_BUS ;; enable for verbose HUB75 diagnosticsEnsure LED_TYPES=65 maps to the intended HUB75 default type in const.h (HS or QS) for these envs.
wled00/bus_manager.cpp (1)
818-821: Stale comment references pins[3]/pins[4].HUB75 now uses 3 “pins” (width,height,chain). Update the comment to match the new interface.
- // Disable chains of panels for now, incomplete UI changes - // if(bc.pins[2] > 1 && bc.pins[3] != 0 && bc.pins[4] != 0 && bc.pins[3] != 255 && bc.pins[4] != 255) { - // virtualDisp = new VirtualMatrixPanel((*display), bc.pins[3], bc.pins[4], mxconfig.mx_width, mxconfig.mx_height, CHAIN_BOTTOM_LEFT_UP); - // } + // Panel chaining UI is not finalized. Only chain_length (pins[2]) is honored for now. + // Future: support multi-panel virtual mapping once UI is complete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platformio_override.sample.ini(4 hunks)wled00/bus_manager.cpp(2 hunks)wled00/bus_manager.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/bus_manager.hwled00/bus_manager.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: netmindz
PR: wled/WLED#3777
File: wled00/bus_manager.cpp:982-989
Timestamp: 2025-09-13T13:13:36.092Z
Learning: In the WLED HUB75 implementation, when HUB75 matrix initialization fails after pin allocation, pins are intentionally not deallocated because they're physically connected to the HUB75 panel and cannot be reused for other purposes. This design choice prioritizes simplicity over strict software resource management consistency.
Learnt from: DedeHai
PR: wled/WLED#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.
Learnt from: netmindz
PR: wled/WLED#3777
File: wled00/bus_manager.cpp:941-947
Timestamp: 2025-09-13T13:13:35.817Z
Learning: For HUB75 matrix panels in WLED, pins are physically dedicated to the matrix hardware and don't need to be deallocated on driver initialization failure since they cannot be reused for other purposes without hardware changes.
Learnt from: DedeHai
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
📚 Learning: 2025-09-13T13:13:36.092Z
Learnt from: netmindz
PR: wled/WLED#3777
File: wled00/bus_manager.cpp:982-989
Timestamp: 2025-09-13T13:13:36.092Z
Learning: In the WLED HUB75 implementation, when HUB75 matrix initialization fails after pin allocation, pins are intentionally not deallocated because they're physically connected to the HUB75 panel and cannot be reused for other purposes. This design choice prioritizes simplicity over strict software resource management consistency.
Applied to files:
wled00/bus_manager.h
📚 Learning: 2025-09-18T03:17:30.081Z
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED BusManager (wled00/FX_fcn.cpp), direct access to BusManager::busses is part of the intended public API design, not an encapsulation violation. The wrapper accessor methods are considered unnecessary obfuscation that should be removed in future refactoring.
Applied to files:
wled00/bus_manager.h
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/bus_manager.h
📚 Learning: 2025-09-18T03:17:30.081Z
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Applied to files:
wled00/bus_manager.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#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:
wled00/bus_manager.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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:
wled00/bus_manager.cpp
📚 Learning: 2025-09-02T02:27:39.220Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S:8-16
Timestamp: 2025-09-02T02:27:39.220Z
Learning: The ESP32 macro is defined when compiling code on all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3, etc.) in the Arduino framework, providing consistent library compatibility across the entire ESP32 family. For target-specific detection, CONFIG_IDF_TARGET_* macros should be used instead.
Applied to files:
platformio_override.sample.ini
🔇 Additional comments (5)
platformio_override.sample.ini (3)
566-571: Same debug flag concern for forum pinout env.Apply the same opt-in treatment to WLED_DEBUG_BUS here.
-D ESP32_FORUM_PINOUT ;; enable for SmartMatrix default pins - -D LED_TYPES=65 - -D WLED_DEBUG_BUS + -D LED_TYPES=65 + ; -D WLED_DEBUG_BUS
588-593: Keep HUB75 debug off by default on MatrixPortal S3.Avoid enabling WLED_DEBUG_BUS by default on S3; keep instructions instead.
-D WLED_ENABLE_HUB75MATRIX -D NO_GFX -D S3_LCD_DIV_NUM=20 ;; Attempt to fix wifi performance issue when panel active with S3 chips -D ARDUINO_ADAFRUIT_MATRIXPORTAL_ESP32S3 - -D LED_TYPES=65 - -D WLED_DEBUG_BUS + -D LED_TYPES=65 + ; -D WLED_DEBUG_BUS
617-622: Same for MOONHUB S3 env: make debug opt-in.Consistent with other HUB75 envs.
-D MOONHUB_S3_PINOUT ;; HUB75 pinout - -D LED_TYPES=65 - -D WLED_DEBUG_BUS + -D LED_TYPES=65 + ; -D WLED_DEBUG_BUSwled00/bus_manager.h (1)
167-170: Add HUB75 pin-count handling (3 fields) — good.This aligns BusConfig pins with HUB75 (width, height, chain). Verify UI/JSON flows expect three “pins” for HUB75 types.
wled00/bus_manager.cpp (1)
815-817: Verify HS/QS dimension math.HS: clamp width/height to 64. QS: width*=2, height/=2. Confirm this matches the library’s expected mapping for half/quarter-scan panels you target.
Also applies to: 822-823
…refactor bus config to be more flexible, this is what we have to work around
|
there are quite a few things that need an update, I will add a few remarks, I can fix brightness after this is merged, as that is easier for me than explaining it. |
|
|
||
| virtualDisp = nullptr; | ||
|
|
||
| if (bc.type == TYPE_HUB75MATRIX_HS) { |
There was a problem hiding this comment.
this code needs to go belw, as it is in MM. cant create a virtual panel from a null pointer.
| //mxconfig.min_refresh_rate = 120; | ||
| mxconfig.clkphase = bc.reversed; | ||
|
|
||
| virtualDisp = nullptr; |
There was a problem hiding this comment.
please add aliases for the bc.pins[x] at the top, the code is very hard to follow and highly error prone with just indices.
| -D WLED_ENABLE_HUB75MATRIX -D NO_GFX -D NO_CIE1931 | ||
| -D WLED_ENABLE_HUB75MATRIX -D NO_GFX | ||
| -D ESP32_FORUM_PINOUT ;; enable for SmartMatrix default pins | ||
| -D LED_TYPES=65 -D DATA_PINS=64,64,1 |
There was a problem hiding this comment.
a comment that DATA_PINS means x,y,chainlength would be nice
|
|
||
|
|
||
| lib_deps = ${esp32s3.lib_deps} | ||
| ${esp32.AR_lib_deps} |
There was a problem hiding this comment.
AR_lib_deps is deprecated.
| -DBOARD_HAS_PSRAM | ||
| -DLOLIN_WIFI_FIX ; seems to work much better with this | ||
| -D WLED_WATCHDOG_TIMEOUT=0 | ||
| ${esp32.AR_build_flags} |
There was a problem hiding this comment.
AR_build_flags is deprecated as well
|
as you know, something is still wrong with the config. I could not get it to save at all, it would just crash. |
That is not the behaviour I was seeing. I could update panel dimensions and chain length correctly |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores