Improvements & fixes for HUB75 #5026
Conversation
- added proper config parameters to allow multiple panels - added config checks - fixed crashes on S3 - changed constant variables to constexpr - added "wled.h" to bus_manager.cpp and removed local function prototypes (needed for buffer allocations) - speed optimisations: yields about 10% higher FPS - proper brightness handling - updated platformio_override.sample.ini - some code cleanup
WalkthroughThis pull request refactors the HUB75 matrix display handling with memory management improvements, bitwise operation optimizations, and virtual display support in the bus manager. API signature changes update pin counts, configuration validation is added to the HUB75 UI settings, and documentation adjustments are made to platformio configuration and color processing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
🧹 Nitpick comments (2)
wled00/colors.cpp (1)
66-66: Consider using BLACK constant consistently.For consistency, the return statement should also use
BLACKinstead of the literal0:- if (c1 == BLACK || amount == 0) return 0; // black or full fade + if (c1 == BLACK || amount == 0) return BLACK; // black or full fadewled00/data/settings_leds.htm (1)
115-128: Address TODO: conditional reboot warning.The reboot warning currently shows for all S3 HUB75 form submissions. The TODO comment suggests checking if config actually changed before warning.
Consider tracking the initial HUB75 configuration and only showing the reboot alert when values actually change:
// At page load, store initial config let initialHub75Config = {}; // In trySubmit, compare and warn only if changed if(isS3() && configChanged(n, initialHub75Config)) { alert("HUB75 changes require a reboot"); }This improves UX by avoiding unnecessary reboot warnings when users haven't modified HUB75 settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
platformio_override.sample.ini(3 hunks)wled00/bus_manager.cpp(11 hunks)wled00/bus_manager.h(2 hunks)wled00/colors.cpp(1 hunks)wled00/data/settings_leds.htm(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/colors.cppwled00/bus_manager.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/bus_manager.h
🧠 Learnings (2)
📚 Learning: 2025-09-13T13:13:35.817Z
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.
Applied to files:
wled00/bus_manager.cppwled00/bus_manager.h
📚 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.cppwled00/data/settings_leds.htmwled00/bus_manager.hplatformio_override.sample.ini
🧬 Code graph analysis (2)
wled00/colors.cpp (1)
usermods/Analog_Clock/Analog_Clock.cpp (2)
c1(110-117)c1(110-110)
wled00/bus_manager.cpp (3)
wled00/fcn_declare.h (2)
d_free(446-446)d_free(448-448)wled00/util.cpp (6)
d_malloc(658-662)d_malloc(658-658)d_malloc(690-707)d_malloc(690-690)allocate_buffer(750-811)allocate_buffer(750-750)wled00/bus_manager.h (5)
show(248-254)show(307-308)show(334-335)show(355-359)show(381-389)
🔇 Additional comments (21)
platformio_override.sample.ini (5)
532-532: Added helpful ghosting guidance for HUB75 configurations.This new comment documents a common HUB75 issue and provides a concrete solution (max brightness setting), which improves user experience and reduces support questions.
565-566: Added board file placement guidance with reference URL.The comment about manual board file placement and the link to the official board definition is helpful for users encountering missing board file errors. This reduces friction during environment setup.
573-574: Improved USB configuration flag organization and clarity.Separating
ARDUINO_USB_CDC_ON_BOOTandARDUINO_USB_MODEinto distinct lines with explanatory comments makes the ESP32-S3 USB troubleshooting guidance clearer and easier to debug if users need to adjust these settings. The mention of possible boot issues is valuable context.
576-576: Clarified TX power reduction in LOLIN_WIFI_FIX comment.The updated comment now explicitly documents that this flag sets lower TX power, which explains why it helps with WiFi stability on ESP32-S3 boards with active HUB75 panels. This is more informative than the previous wording.
602-602: Consistent TX power clarification in second HUB75 environment.The same LOLIN_WIFI_FIX comment update applied here maintains consistency across both ESP32-S3 HUB75 configurations.
wled00/bus_manager.h (2)
398-406: LGTM - Well-structured additions for virtual display support.The new member variables and constants are properly typed and support the expanded HUB75 functionality:
_rowsand_colsenable multi-panel configurations_ledBuffertype change fromuint32_t*toCRGB*is more semantically appropriate for FastLED integration- Constants use proper
constexprwith explicit typing
169-169: API change correctly implements expanded HUB75 pin configuration.The change from 3 to 5 pins for HUB75 type is properly implemented and consistent throughout the codebase:
- BusHub75Matrix::getPins() returns exactly 5 values (width, height, chain_length, rows, cols)
- All callers treat the pin count as a dynamic value via loops—no hardcoded assumptions
- OUTPUT_MAX_PINS is already defined as 5, supporting the expanded configuration
- Call sites in cfg.cpp and xml.cpp scale correctly with the returned pin count
wled00/data/settings_leds.htm (4)
121-123: LGTM - Robust validation with sensible defaults.The validation logic correctly enforces:
- Panel count must equal rows × cols
- Panels with height ≥ 64 are limited to single panel (memory constraint)
The
||1defaults handle missing values gracefully.
263-268: LGTM - Clear and descriptive UI labels.The updated labels correctly reflect the HUB75 configuration model with width, height, panel count, rows, and columns.
275-275: LGTM - Pin calculation aligns with API change.The change from
2*isHub75(t)to4*isHub75(t)correctly reflects the expanded HUB75 configuration from 3 to 5 "pins" (width, height, chain_length, rows, cols).
385-391: LGTM - Sensible defaults and constraints.The field validation correctly:
- Limits chain length, rows, and cols to 1-4 (matching C++ limits)
- Defaults empty values to 1
- Prevents invalid configurations
wled00/bus_manager.cpp (10)
26-26: LGTM - Explicit dependency inclusion.Including "wled.h" makes buffer allocation dependencies (d_malloc, d_free, allocate_buffer) explicit and supports the memory management improvements in this PR.
34-54: LGTM - Micro-optimization using bit operations.Replacing division and modulo with bit shifts is a classic optimization:
position / 8→position >> 3position % 8→position & 0x07While modern compilers typically optimize these automatically, the explicit bit operations make the intent clearer and may provide minor performance benefits on embedded targets.
762-768: LGTM - Clear initialization and readable aliases.Explicitly initializing
virtualDisp = nullptrprevents potential use-after-free issues, and the pin aliases (panelWidth, panelHeight, chainLength, rows, cols) improve code readability.
781-787: LGTM - Defensive memory constraints.The chain length validation correctly:
- Clamps to maximum of 4 panels (prevents excessive memory allocation)
- Forces single panel for height ≥ 64 (prevents memory exhaustion)
- Provides clear debug output
This aligns with the UI validation in settings_leds.htm.
927-946: LGTM - Careful memory management with proper cleanup.The memory allocation logic demonstrates good practices:
- Defensive freeing of existing buffers (Lines 927-928)
- Proper error handling with cleanup on allocation failure
- Using
allocate_bufferwith appropriate flags (BFRALLOC_PREFER_DRAM | BFRALLOC_CLEAR)- Clear debug messages for troubleshooting
949-980: LGTM - Comprehensive virtual display initialization.The virtual display setup correctly:
- Determines when virtualization is needed (multi-panel chains or quarter-scan)
- Creates
VirtualMatrixPanelwith proper geometry- Configures scan rates for quarter-scan panels based on height
- Handles unsupported configurations with cleanup
998-998: LGTM - Appropriate IRAM placement for hot path.Adding
IRAM_ATTRtosetPixelColoris appropriate as this is a frequently-called hot-path function. Placing it in IRAM reduces instruction cache misses and improves performance, especially during LED updates.
1045-1061: LGTM - Efficient show() with proper brightness handling.The implementation correctly:
- Selects virtual or physical display based on configuration
- Applies brightness using FastLED's
nscale8_video()(video scaling)- Updates only dirty pixels for performance
- Clears dirty flags after display update
1063-1078: LGTM - Careful cleanup with S3 workaround.The cleanup implementation demonstrates proper resource management:
- Stops DMA output before cleanup
- Deallocates pins
- Works around ESP32-S3 DMA deletion crash (Lines 1070-1075)
- Properly frees buffers using d_free
The S3 workaround is well-documented and aligns with the "requests additional testing" note in the PR objectives.
Based on learnings: "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."
1093-1102: LGTM - getPins correctly returns 5 configuration values.The implementation returns the expanded HUB75 configuration:
- Width (mxconfig.mx_width)
- Height (mxconfig.mx_height)
- Chain length (mxconfig.chain_length)
- Rows (_rows)
- Columns (_cols)
This aligns with the API change in bus_manager.h (Line 169) and the UI implementation in settings_leds.htm.
| if (bc.type == TYPE_HUB75MATRIX_HS) { | ||
| mxconfig.mx_width = min((uint8_t) 64, bc.pins[0]); | ||
| mxconfig.mx_height = min((uint8_t) 64, bc.pins[1]); | ||
| // 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); | ||
| // } | ||
| mxconfig.mx_width = min((uint8_t) 64, panelWidth); // TODO: UI limit is 128, this limits to 64 | ||
| mxconfig.mx_height = min((uint8_t) 64, panelHeight); | ||
| } 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; | ||
| } | ||
| _isVirtual = true; | ||
| mxconfig.mx_width = min((uint8_t) 64, panelWidth) * 2; | ||
| mxconfig.mx_height = min((uint8_t) 64, panelHeight) / 2; | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
Address dimension limit inconsistency.
The code limits panel dimensions to 64 pixels, but Line 790's TODO notes "UI limit is 128, this limits to 64". This creates a mismatch between what the UI allows and what the backend accepts.
Verify the correct maximum dimension for HUB75 panels. If 64 is the correct limit, update the UI validation in settings_leds.htm. If 128 is correct, adjust the code here.
🏁 Script executed:
#!/bin/bash
# Check UI field limits for HUB75 width/height
rg -n "LC.*max.*128" wled00/data/settings_leds.htm -A 2 -B 2Length of output: 164
And check for any related constants or configuration:
🏁 Script executed:
#!/bin/bash
# Search for more context about HUB75 matrix dimensions and type handling
rg -n "TYPE_HUB75MATRIX" wled00/bus_manager.cpp -B 2 -A 5Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Look for HUB75-related constants or limits in the codebase
rg -n "HUB75|mx_width|mx_height" wled00/bus_manager.cpp -B 1 -A 1 | head -100Length of output: 4927
Update the UI to enforce type-specific dimension limits for HUB75 panels.
The code correctly handles different limits for panel types:
- Half-Scan (HS): limited to 64×64 via
min((uint8_t) 64, panelWidth) - Quarter-Scan (QS): can reach 128×128 via
min(64, panelWidth) * 2
However, the UI allows 128 for both types without differentiation. This means users can set invalid 128×128 dimensions for Half-Scan panels through the UI, which the code silently clamps to 64×64. Update settings_leds.htm to set LC.max = 64 when isHub75(t) AND the panel type is Half-Scan, or dynamically adjust based on selected panel type. Remove or resolve the TODO comment at line 790 once fixed.
🤖 Prompt for AI Agents
In wled00/bus_manager.cpp around lines 789-796 the code clamps HS panels to
64×64 but the UI still allows 128×128 causing silent clamping; update
settings_leds.htm to enforce type-specific limits: when isHub75(t) is true and
panel type is Half-Scan set LC.max = 64, when panel type is Quarter-Scan set
LC.max = 128 (or compute from selected type), add a change handler to
dynamically adjust LC.max when the panel type dropdown changes and
validate/submit to prevent values above the type limit, and remove or resolve
the TODO comment at the C++ line once the UI enforces the correct limit.
There was a problem hiding this comment.
correct, this is what the TODO sais.
There was a problem hiding this comment.
@DedeHai, understood! Since this is a known issue already documented in the TODO comment, would you like to:
- Address it in this PR - I can help update the UI validation logic in
settings_leds.htmto enforce type-specific limits (64 for Half-Scan, 128 for Quarter-Scan) - Defer to a follow-up - If this is out of scope for the current PR, I can create a tracking issue that references this PR and the TODO comment for future work
Which would you prefer?
|
@DedeHai and @netmindz one thing you might want to try - currently chained hub75 panels are not compatible with the wled 2D layout settings. The reason is that HUB75 is scan line oriented, and adding a second panel means that the physical width (scanline) is increased, for example from width=64 (1panel) to width=128 (2 panels). This is not foreseen in the 2D setup of wled (at least it was not foreseen in the code from 6 months ago 😉 ). The solution might be to tell the driver to use "vertical" panel layout, which means that chained panels are stacked in one column, instead of putting them into a row next to each other. This "vertical layout" should make multi-panels (chained panels) compatible with what WLED expects: first pixel = top left, no serpentine, each panel having a separate (not interleaved) pixels range. |
There was a problem hiding this comment.
some comments on the core HUB75 driver parts.
- not sure if removing
display->setBrightness(_bri);is a good idea - what was the reason?
-> OK, clarified. - consider to add user settings for
mxconfig.driveroptions. panels with FM6124 might mis-behave with driver=default.
-> possible enhancement, still open. - Think about removing the fallback code for "if (! ledBuffer)", to optimize speed in the critical path. Downside would be that the HUB75 driver will refuse to start if it cannot get its ledBuffer.
-> OK, clarified. Keep the fallback for low RAM boards like HD-WF2.
|
|
||
| mxconfig.double_buff = false; // Use our own memory-optimised buffer rather than the driver's own double-buffer | ||
|
|
||
| // mxconfig.driver = HUB75_I2S_CFG::ICN2038S; // experimental - use specific shift register driver |
There was a problem hiding this comment.
@DedeHai, @netmindz actually the shift register driver should be user-configurable.
Especially panels with FM6124 will mis-behave with driver=default. In the MM code we made an assumption for simplicity, that "outdoor" panels are always 4-scan FM6124, while "indoor" is always 2-scan with "default" shift register driver -> https://github.com/MoonModules/WLED-MM/blob/d9e542663634812e9136b66bd1d53f187979d33a/wled00/bus_manager.cpp#L684
This assumption works in 90% of all cases, but for a proper solution the user should also have an option to select the shift register chip type.
NB: panel brightness control via display->setbrightness() works better with the correct shift register driver.
There was a problem hiding this comment.
The issue is the settings UI is a total pain in the ass and needs complete rewrite really. It's a similar issue with the virtual display params, we need the ability for the bus config and UI to allow for an enum type, there is an old issue/pr I started a while back for that, but even getting the basic changes to delegate the led type list to the bus type was slow progress for adoption
| // 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); | ||
| // } | ||
| mxconfig.mx_width = min((uint8_t) 64, panelWidth); // TODO: UI limit is 128, this limits to 64 |
There was a problem hiding this comment.
This toDo is a bit of a pity ... I have a 128x64 2-scan panel, and it can be used with width=128 height=64... but yes this was never supported in upstream, so ok to address 128x64 panels later in another PR.
Hint: many 128x64 can also be used as 64x64 chain_lenght=2.
There was a problem hiding this comment.
this is code from @netmindz, I simply added the TODO as I noticed the discrepancy and did not know the reason for this limit.
There was a problem hiding this comment.
i'm also not sure any more why the limit was added ... maybe just to catch user errors, because 64x64 used to be the max supported panel size. I can confirm that 128x64 works here, too. So it might be safe to increase the "width" limit to 128.
There was a problem hiding this comment.
Was when i was battling issues with the settings management and so never wanted to use width 255, height 255 or chain length 255 as you were then in a memory based bootloop, so just adding some sanity
There was a problem hiding this comment.
This toDo is a bit of a pity ... I have a 128x64 2-scan panel, and it can be used with width=128 height=64... but yes this was never supported in upstream, so ok to address 128x64 panels later in another PR. Hint: many 128x64 can also be used as 64x64 chain_lenght=2.
I believe all 128s are really a pair of 64s. Not sure if was the width of 64 or the height of 64 that needed the extra pin in hub75e, so there might be similar reasons why 128s are built this way
softhack007
added
good job
optimization
|
|
||
| void BusHub75Matrix::cleanup() { | ||
| if (display && _valid) display->stopDMAoutput(); // terminate DMA driver (display goes black) | ||
| delay(30); // give some time to finish DMA |
There was a problem hiding this comment.
(minor) it might be better for thread-safety to change the order of _valid=false and delay(30) so that _valid=false; comes before delay.
| mxconfig.chain_length = max((uint8_t) 1, min(bc.pins[2], (uint8_t) 4)); // prevent bad data preventing boot due to low memory | ||
|
|
||
| if(mxconfig.mx_height >= 64 && (mxconfig.chain_length > 1)) { | ||
| DEBUGBUS_PRINTLN("WARNING, only single panel can be used of 64 pixel boards due to memory"); | ||
| mxconfig.chain_length = 1; | ||
| } |
There was a problem hiding this comment.
Not sure it's safe to just remove this, bad for users to be in low memory boot loop
There was a problem hiding this comment.
the new heap management should take care of that and not allow buffer allocation to below the set threshold.
|
Had a quick test with a basics single panel setup. UI improvements look good. Need to test multiple panels setups. |
brightness should not change, its still set to 255 at driver level. New gamma correction handling may play a role. |
3 participants
I do not own any quarter-scan HUB75 panels nor a suitable ESP32, so please test if this is working, I tested this on 2x half-scan 64x32 panels using an S3 only.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation