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
WalkthroughRefactors HUB75 handling: adds virtual display support and memory-allocation wrappers, optimizes bitwise helpers, changes Hub75 pin reporting from 3→5, updates setPixelColor/getPins signatures/behavior, tightens HUB75 UI validation, and updates platformio documentation comments. No public data-type removals. 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
There was a problem hiding this comment.
@softhack007 logic done (including I2S bus frequency). @netmindz not PITA when you know what to do, however it is a hack really. 😉
See my fork (it even compiles now but I have no way of knowing if it works 😂 ).
| // 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.
|
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. |
|
@netmindz @DedeHai & @softhack007 I've backported @netmindz's Hub75 to my fork (0.15) where I made some similar changes as @DedeHai made (no coordination) here. 😄 One thing I'd comment here is the removal of HW assisted brightness scaling in this PR. It is a waste of CPU cycles to do any scaling on bus level IMO. For NPB it makes sense as that was purely SW. The other thing is |
I did that to be consistent with NPB handling, as I just learned that HUB75 actually support HW-brightness. I need to check that but if it is true hardware scaling, we should definitely NOT do software scaling. so thanks for pointing this out.
IIRC there may be some usermods that need bus-level gPC(), at least thats what I remember from the top of my head, because I was close to removing them when I updated the ABL & brightness scaling code. Not sure this is actually true or just coming from some outdated, lingering inline comment. |
None should do that IMO. I would remove bus level gPC/sPC from public API. There are benefits of having bus level ABL and strip (AKA WS2812FX) level ABL. I would keep both. |
i think it is "kind of" hardware scaling. the hub75 driver will PWM-modulate the "enable" signal, instead of scaling individual pixels. But this feature depends on having the correct "shift register" driver in HUB75 mxconfig. |
Hi @blazoncek, hope you are doing well :-) About your comments:
Cheers, and take care, Edit: just for the fun of it - run the below .gif in image effect on HUB75, once you have the hardware |
I did not check on how the HUB75 driver uses its internal buffer(s?), the additional CRGB buffer is needed for determining the "dirty" pixels, if we could access the HUB75 driver's internal buffer for that, we would not need it. I ran quite a few experiments when writing this PR, like removing the buffer and the "dirty" bit buffer and makeing the CRGB buffer a uint32_t buffer (slight seed increase) and even using the highest byte of that to mark "dirty" pixels but came back to the way it was i.e. seperate dirty pixel buffer (1 bit per pixel) and CRGB as it yielded highest throughput at lowest RAM cost. Fastest would be: seperate dirty pixel buffer with uint32_t colorbuffer. Best for RAM use is to grant access to the busmanager's _pixels[] buffer and not use dirty. just FYI: here are my raw notes about those experiments (translated by chatGPT): |
|
@softhack007 you are missing my point. 😉
The last statement includes improvements to LED settings page where Hub75's GPIO are taken into account (and all LED outputs get GPIO dropdowns) and ability to select shift register via dropdown. I will not make a PR for that due to lack of time, but you are free to cherry pick and adapt as needed. |
|
For anyone interested: UI code for enabling Hub75 panels has been improved and is now working as expected (limiting dimensions to multiples of 32, allowing maximum of 6 panels, adding ability to select shift register via drop-down (currently a hack but it could be made driver-supplied), allocating GPIO in UI and preventing them to be used for other outputs). One thing that is not clear to me from the MM or Netmindz's code is if it is allowed to have Hub matrix with dimensions of 16, 48, or 96 (in fact any multiple of 16) and if you can arrange panels in a similar way as 2D setup allows (multiple rows and columns, no UI for that in LED settings page). |
@blazoncek Thanks for your effort :-) About the "multiple of 16" dimensions question: yes there are still 16x16 and 16x32 panels available, especially the "outdoor" ones with high brightness and pixel spacing "P10" or greater.
|
Thanks @DedeHai for this interesting comparison :-) It matches with my observations. The effect of dirty bits tracing is even stronger in the 0.14.x-based MM codebase (no pixel buffer on bus level) - there it leads to 2x speedup in "show". Maybe i'll do some experiments with I see a few more optimizations, that were not "exploited" in MM yet:
Edit 1: Edit 2:
that's the original problem - the MrCodetastic driver directly translates each pixel into and internal DMA buffer, which stores some kind of PWM waveform for output. This is not accessible from outside, and I doubt that we could restore the original pixel even when we had acess to the DMA buffer memory. BTW, this internal DMA buffer is the reason why I called the driver "memory greedy" - the internal buffer for 64x64, 24 bit color is a full 32KB (sic!), about 3x the size of individual pixels. |
FYI it is similar with I2S in NPB. Called cadence there.
Not sure this would work with layered/overlapping segments. Do not forget that all segments are blended and until upstream has transparency (which should be supported in effects too) all pixels are updated during |
@blazoncek I see your point. Actually, the dirty bits logic can possibly still help, at least when the framebuffer on lowest level does not get too many "intermediate" updates. In a nutshell:
Edit: it could imply that the "blending" and "overlaying" logic needs to be modified, so that the final pixel color is calculated ONCE (loop over all bus pixels, blend/overlay every "upper" pixel; instead of going segment-by-segment). But the result may be worth it, because you'll always find a lot of pixels that actually did not change between two frames, so its not necessary to push them down to the NPB driver again. |
|
I'll leave optimizations for more competent people (or people with more time). 😄 However, having transparent pixels would also help (to some extent). You could have dirty flag at WS2812FX level (which would then benefit any driver) and it is rather simple to implement. Not much different than what you posted and I don't think blending would need changes (apart from comparing old vs. new). Dirty buffer can be temporary. |
I said I won't, but I did. 🤦♂️ |
|
FYI I have entirely dynamic Hub75 support ready. No need for custom compiles for different boards or configurations. |
|
@softhack007 you seem most knowledgeable about Hub75 implementation in WLED. |
Hi @blazoncek yes
In fact I was thinking about removing this mode and simply set the bus driver I don't know exactly how the HUB75 hardware driver achieves synchronisation - but when pushing all pixels in busHUB75::show(), I've never seen any sync problems. 🤔 Maybe it synchronizes implicitly on pixel (0,0), and then you just have to go fast enough to stay ahead of the "scan beam" that refreshes the display at 60-120hz. Currently (with dirty bits) busHUB75:show() takes around 3-8ms for my 192x64 pixels test setup, so we are always ahead of the "beam" = refreshing pixels in the driver fast enough (at 250 to 120 fps) so the beam is behind us. |
|
Hmm.
If what you describe is true, then |
on the contrary, it even needs a second buffer to mark "dirty" LEDs which significantly improves frame rate. |
Even if all pixels are redrawn in However if |
|
I'll dump this here. hub75.mov
|
awesome! PR when? :) |
I need someone with actual Hub75 panels to test 1st. (Until I get mine) |
@blazoncek settings page looks good :-) all the options I can imagine are there, only two panel sizes still missing: 16 (high or wide), 128 (wide only) I can't test right now, because I'll be "away from HUB75" (on business trip) for the next two weeks. |
Hi @blazoncek the main problem is speed. And as you already noticed, the last "stone" is the repeated 1D to 2D translation. But there is more: If we go through Lines 1331 to 1333 in ae37f42 It may look like tiny amounts of CPU cycles, but considering that you only have 5ms to push 128x64 pixels out to the driver, it might become a real problem. Also you did not show us yet where to place the dirty bits array - which created a major speedup - in your proposal. |
|
@softhack007 @blazoncek since HUB75 is currently pretty unusable in main without this PR: how about merging this and then take it from there with the improvements? |
@softhack007 but you do that in any case. There is no way around it unless you hack and use
As each bus pixel is only written once (be it black or not and never read) there is no need for dirty array as each pixel is considered dirty on new frame. However if one really wants dirty array I have a solution for that too. My fork uses CRGBA instead of (CRGB or uint32_t) which encodes transparency into white channel (unused on Hub75). Alpha channel can be used as a dirty flag (question remains what if previous frame had such pixel non-black). If one doesn't want CRGBA you can always encode dirty flag in WW part of uint32_t. |
I'm not willing to spend additional time adjusting the code again. Sorry. If you do merge then you'll need to pick from my fork manually if you want my modifications. |
|
RE: speed considerations: I have tested various scenarios, as I mentioned before. The code from this PR is the way to go. |
|
Got my Adafruit MatrixPortal S3 and two 64x32 panels. We'll see how it goes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wled00/bus_manager.cpp (3)
949-958: Consider adding parentheses for clarity in complex condition.The condition on line 951 relies on operator precedence (
&&binds tighter than||). While the current parsing likely matches intent, explicit parentheses would improve readability:- if (chainLength > 1 && (_rows > 1 || _cols > 1) || bc.type == TYPE_HUB75MATRIX_QS) { + if ((chainLength > 1 && (_rows > 1 || _cols > 1)) || bc.type == TYPE_HUB75MATRIX_QS) {
1066-1067: Consider moving_valid = falsebeforedelay(30)for thread safety.As noted in a prior review, setting
_valid = falsebefore the delay would prevent other threads from attempting to use the display during shutdown.if (display && _valid) display->stopDMAoutput(); // terminate DMA driver (display goes black) + _valid = false; delay(30); // give some time to finish DMA - _valid = false;
1077-1078: Misleading formatting with multiple statements on one line.The current formatting makes it unclear that
_ledBuffer = nullptr;executes unconditionally (which is correct). Split for clarity:- if (_ledBuffer != nullptr) d_free(_ledBuffer); _ledBuffer = nullptr; - if (_ledsDirty != nullptr) d_free(_ledsDirty); _ledsDirty = nullptr; + if (_ledBuffer != nullptr) d_free(_ledBuffer); + _ledBuffer = nullptr; + if (_ledsDirty != nullptr) d_free(_ledsDirty); + _ledsDirty = nullptr;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platformio_override.sample.ini(3 hunks)wled00/bus_manager.cpp(11 hunks)wled00/data/settings_leds.htm(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/bus_manager.cpp
🧠 Learnings (43)
📓 Common learnings
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.
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.
Learnt from: netmindz
Repo: wled/WLED PR: 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
Repo: wled/WLED PR: 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: 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.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: mval-sg
Repo: wled/WLED PR: 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.
Learnt from: netmindz
Repo: wled/WLED PR: 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.
📚 Learning: 2025-09-13T13:13:36.092Z
Learnt from: netmindz
Repo: wled/WLED PR: 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/data/settings_leds.htmplatformio_override.sample.iniwled00/bus_manager.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 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/data/settings_leds.htmwled00/bus_manager.cpp
📚 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 wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-13T13:13:35.817Z
Learnt from: netmindz
Repo: wled/WLED PR: 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/data/settings_leds.htmplatformio_override.sample.iniwled00/bus_manager.cpp
📚 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 wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/data/settings_leds.htmplatformio_override.sample.ini
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/settings_leds.htm
📚 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/data/settings_leds.htmplatformio_override.sample.iniwled00/bus_manager.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/data/settings_leds.htmplatformio_override.sample.iniwled00/bus_manager.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/settings_leds.htmplatformio_override.sample.iniwled00/bus_manager.cpp
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/data/settings_leds.htmwled00/bus_manager.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:
platformio_override.sample.iniwled00/bus_manager.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
platformio_override.sample.ini
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
platformio_override.sample.iniwled00/bus_manager.cpp
📚 Learning: 2025-07-06T16:15:24.051Z
Learnt from: Jason2866
Repo: wled/WLED 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`.
Applied to files:
platformio_override.sample.ini
📚 Learning: 2025-09-02T02:27:39.220Z
Learnt from: willmmiles
Repo: wled/WLED PR: 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
📚 Learning: 2025-07-06T16:11:05.896Z
Learnt from: Jason2866
Repo: wled/WLED 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.
Applied to files:
platformio_override.sample.ini
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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.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:
wled00/bus_manager.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 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.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/bus_manager.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, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/bus_manager.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, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED 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.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-08-29T15:38:46.208Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-28T09:53:42.670Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/data/index.js:2406-2413
Timestamp: 2025-09-28T09:53:42.670Z
Learning: In WLED, code paths that appear unreachable may actually be safety fallbacks for low-heap conditions. These fallbacks are intentional defensive programming measures to ensure UI functionality is preserved when system resources are constrained.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-15T19:13:56.469Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4791
File: wled00/util.cpp:737-743
Timestamp: 2025-09-15T19:13:56.469Z
Learning: In WLED's util.cpp, the *_realloc_malloc functions (p_realloc_malloc and d_realloc_malloc) are intentionally designed to free the original buffer on realloc failure and allocate a new buffer, implementing a "replace buffer" semantic rather than traditional realloc behavior. This is documented in the function comments and is the intended design by the author DedeHai.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-04-27T09:37:28.415Z
Learnt from: KrX3D
Repo: wled/WLED 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.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-14T05:48:44.673Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-05-09T18:48:21.296Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.296Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dx*dx + dy*dy to be a practical concern in particle system effects.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-02T01:56:43.841Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-21T20:44:09.502Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4928
File: wled00/bus_wrapper.h:1248-1275
Timestamp: 2025-09-21T20:44:09.502Z
Learning: In WLED bus_wrapper.h, the _useParallelI2S flag is used for selecting the correct function pointer (between parallel I2S and non-parallel I2S bus types) rather than for changing memory calculations. Both parallel and non-parallel I2S configurations have the same memory requirements, so memUsage() correctly doesn't differentiate based on this flag.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/bus_manager.cpp
📚 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:
wled00/bus_manager.cpp
📚 Learning: 2025-08-08T17:31:47.330Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:31:47.330Z
Learning: In WLED core, avoid introducing AR-specific helper wrappers for um_data access. Keep um_data untyped and, when reading samplePeak, prefer `(*(uint8_t*)um_data->u_data[3]) != 0` over `*(bool*)` to avoid alignment/aliasing issues, while staying decoupled from the AudioReactive usermod.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-08-29T01:34:34.358Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-02T01:48:16.409Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:72-85
Timestamp: 2025-09-02T01:48:16.409Z
Learning: In NeoPixelBus driver code, ESP_ERROR_CHECK (which can abort) is used in destructors when driver uninstall failures indicate hardware is in an unsafe configuration, as such errors are considered unrecoverable hardware safety issues rather than typical software errors.
Applied to files:
wled00/bus_manager.cpp
🧬 Code graph analysis (1)
wled00/bus_manager.cpp (2)
wled00/fcn_declare.h (2)
d_free(448-448)d_free(450-450)wled00/util.cpp (6)
d_malloc(663-667)d_malloc(663-663)d_malloc(695-712)d_malloc(695-695)allocate_buffer(755-816)allocate_buffer(755-755)
⏰ 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). (20)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (12)
platformio_override.sample.ini (1)
548-548: Helpful documentation for HUB75 users.The note about ghosting at full brightness with the
WLED_HUB75_MAX_BRIGHTNESSworkaround is valuable user guidance that aligns with the new configurable max brightness handling in the C++ code.wled00/data/settings_leds.htm (4)
125-138: HUB75 validation logic looks correct.The validation ensures:
- Panel count matches rows × cols (
r*c !== p)- Height ≥ 64 restricts to single panel (matches C++
mxconfig.chain_length = 1logic)The S3 reboot notice is helpful since the TODO notes this should ideally only trigger on actual config changes.
273-279: Clear and descriptive labels for HUB75 configuration.The updated labels ("Panel (width x height):", "No. of Panels:", "rows x cols:") provide better clarity for users configuring HUB75 panels.
285-285: Pin count calculation correctly updated for HUB75.The formula
Math.max(gT(t).t.length, 1) + 4*isHub75(t)correctly yields 5 fields for HUB75 (width, height, chain_length, rows, cols), matching the C++getPins()returning 5 values.
395-401: Sensible defaults and constraints for HUB75 panel configuration.The max=4 constraint aligns with the C++ backend limit (
min(chainLength, (uint8_t) 4)), and defaulting empty values to 1 prevents invalid configurations.wled00/bus_manager.cpp (7)
34-40: Good optimization using bitwise operations.Using
>> 3for division by 8 and& 0x07for modulo 8 is an appropriate micro-optimization for these frequently-called bit manipulation functions.
789-799: Panel dimension handling looks correct.The Half-Scan (HS) and Quarter-Scan (QS) configurations correctly apply their respective dimension transformations. The
_isVirtualflag is appropriately set for QS panels.
927-946: Memory allocation follows WLED patterns correctly.Using
d_malloc/d_freeandallocate_bufferwithBFRALLOC_PREFER_DRAM | BFRALLOC_CLEARis consistent with WLED's memory management approach and optimizes for performance when PSRAM is present.
998-1027: Well-optimized setPixelColor with IRAM_ATTR and dirty tracking.The
IRAM_ATTRannotation is appropriate for this hot path. The dirty-bit optimization correctly avoids unnecessary redraws, and the dual-path approach (buffered vs. direct) handles memory-constrained scenarios gracefully.
1094-1103: getPins correctly returns 5 configuration values.The 5-value return (width, height, chain_length, rows, cols) aligns with the updated UI field count and enables proper round-trip of HUB75 configuration.
1042-1061: show() correctly handles virtual display routing and dirty-bit optimization.The implementation properly routes pixels to either the virtual display or direct display based on
_isVirtual, and the dirty-bit check on line 1052 efficiently skips unchanged pixels.
762-762: The initialization at line 762 is correct and necessary.virtualDispis properly cleaned up in thecleanup()function (lines 1072–1075) where it is deleted and nullified. The TODO comment refers to a documented architectural limitation on non-ESP32S3 platforms when using the "NO_GFX" configuration (see line 1075 comment), not a missing cleanup path. This is a known trade-off rather than a bug requiring immediate correction.
| if (mxconfig.mx_height >= 64 && (mxconfig.chain_length > 1)) { | ||
| DEBUGBUS_PRINTLN(F("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.
Condition checks mxconfig.mx_height before it's assigned.
The check mxconfig.mx_height >= 64 on line 784 occurs before mxconfig.mx_height is set (lines 790-795). This will always use the uninitialized/default value.
Move this validation after the dimension assignment, or use panelHeight instead:
- if (mxconfig.mx_height >= 64 && (mxconfig.chain_length > 1)) {
+ if (panelHeight >= 64 && (mxconfig.chain_length > 1)) {
DEBUGBUS_PRINTLN(F("WARNING, only single panel can be used of 64 pixel boards due to memory"));
mxconfig.chain_length = 1;
}🤖 Prompt for AI Agents
In wled00/bus_manager.cpp around lines 784-787, the condition uses
mxconfig.mx_height before that field is set (it's assigned at ~790-795), so move
this validation after the dimension assignment or replace the check to use the
already-initialized local variable (panelHeight). Update the code so the height
check runs after mxconfig.mx_height is assigned (or change the condition to use
panelHeight) and then, if height >= 64 and chain_length > 1, log the warning and
set mxconfig.chain_length = 1.
|
|
||
| // as noted in HUB75_I2S_DMA library, some panels can show ghosting if set higher than 239, so let users override at compile time | ||
| #ifndef WLED_HUB75_MAX_BRIGHTNESS | ||
| #define WLED_HUB75_MAX_BRIGHTNESS 255 | ||
| #endif | ||
| // let's adjust default brightness (128), brightness scaling is handled by WLED | ||
| //display->setBrightness8(WLED_HUB75_MAX_BRIGHTNESS); // range is 0-255, 0 - 0%, 255 - 100% | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check how WLED_HUB75_MAX_BRIGHTNESS is used in the codebase
rg "WLED_HUB75_MAX_BRIGHTNESS" -nRepository: wled/WLED
Length of output: 469
🏁 Script executed:
# Check the context around line 1039 where setBrightness() is called
sed -n '1030,1050p' wled00/bus_manager.cpp | cat -nRepository: wled/WLED
Length of output: 989
🏁 Script executed:
# Search for HUB75-related comments about ghosting or brightness
rg -i "ghosting|setBrightness" wled00/bus_manager.cpp -B2 -A2Repository: wled/WLED
Length of output: 630
🏁 Script executed:
# Get broader context of the HUB75 initialization section
sed -n '900,1050p' wled00/bus_manager.cpp | cat -nRepository: wled/WLED
Length of output: 7599
WLED_HUB75_MAX_BRIGHTNESS macro is defined but never enforced.
The macro is defined with a default of 255 (lines 905-907) and documented in platformio_override.sample.ini to allow users to override at compile time to prevent panel ghosting. However, it is never actually applied to limit brightness. The setBrightness() method (line 1038–1041) calls display->setBrightness8(_bri) without referencing the macro, so compile-time overrides to WLED_HUB75_MAX_BRIGHTNESS have no effect. Either uncomment and use the macro in initialization, apply it as a cap in the setBrightness() method, or remove the unused macro definition.
4 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
✏️ Tip: You can customize this high-level summary in your review settings.