Use global Buffer for HUB75#5744
Conversation
its faster but it breaks "getPixelColor()" but that is unused anyway.
WalkthroughChangesThe PR clears reused segment data, simplifies segment memory accounting, changes HUB75 matrix rendering to direct DMA-backed writes without Memory and rendering changes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
🧹 Nitpick comments (2)
wled00/bus_manager.cpp (2)
1097-1104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant null check.
Since this block is already guarded by
if (_ledsDirty != nullptr), the ternary operator checking_ledsDirtyagain is redundant and can be simplified.♻️ Proposed refactor
if (_ledsDirty != nullptr) { DEBUGBUS_PRINTLN(F("MatrixPanel_I2S_DMA LEDS dirty bit optimization enabled.")); DEBUGBUS_PRINT(F("MatrixPanel_I2S_DMA LEDS dirty buffer uses ")); - DEBUGBUS_PRINT((_ledsDirty? getBitArrayBytes(_len) :0)); + DEBUGBUS_PRINT(getBitArrayBytes(_len)); DEBUGBUS_PRINTLN(F(" bytes.")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/bus_manager.cpp` around lines 1097 - 1104, In the _ledsDirty initialization/logging block, remove the redundant _ledsDirty ternary inside the DEBUGBUS_PRINT call and directly pass getBitArrayBytes(_len), preserving the existing outer null guard and logging behavior.
1115-1124: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsolidate pixel coordinate calculations and use fast types.
To remove code duplication and adhere to the hot-path optimization guidelines, you can extract the
xandycalculations outside the conditional and useuint_fast16_tinstead ofint. As per coding guidelines, useuint_fast16_tin hot-path code like pixel set operations.♻️ Proposed refactor
- if (virtualDisp != nullptr) { - int x = pix % _panelWidth; // TODO: check if using & and shift would be faster here, it limits to power-of-2 widths though - int y = pix / _panelWidth; - virtualDisp->drawPixelRGB888(int16_t(x), int16_t(y), r, g, b); - } else { - int x = pix % _panelWidth; - int y = pix / _panelWidth; - display->drawPixelRGB888(int16_t(x), int16_t(y), r, g, b); - } + uint_fast16_t x = pix % _panelWidth; // TODO: check if using & and shift would be faster here, it limits to power-of-2 widths though + uint_fast16_t y = pix / _panelWidth; + + if (virtualDisp != nullptr) { + virtualDisp->drawPixelRGB888(int16_t(x), int16_t(y), r, g, b); + } else { + display->drawPixelRGB888(int16_t(x), int16_t(y), r, g, b); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/bus_manager.cpp` around lines 1115 - 1124, In the pixel-setting routine containing the virtualDisp/display branch, compute the pixel coordinates once before the conditional and reuse them in both drawPixelRGB888 calls. Declare the coordinates as uint_fast16_t instead of int, while preserving the existing modulo/division calculations and rendering behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Around line 1097-1104: In the _ledsDirty initialization/logging block, remove
the redundant _ledsDirty ternary inside the DEBUGBUS_PRINT call and directly
pass getBitArrayBytes(_len), preserving the existing outer null guard and
logging behavior.
- Around line 1115-1124: In the pixel-setting routine containing the
virtualDisp/display branch, compute the pixel coordinates once before the
conditional and reuse them in both drawPixelRGB888 calls. Declare the
coordinates as uint_fast16_t instead of int, while preserving the existing
modulo/division calculations and rendering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84cb121d-e06f-4a21-846b-ded9648d8baa
📒 Files selected for processing (4)
wled00/FX_fcn.cppwled00/bus_manager.cppwled00/const.hwled00/util.cpp
|
@softhack007 is there a reson we do not use the NO_FAST_FUNCTIONS buildflag? see https://github.com/tidbyt/ESP32-HUB75-MatrixPanel-I2S-DMA/blob/master/doc/BuildOptions.md |
@DedeHai I think we can enabled that, WLED-MM builds already use the NO_FAST_FUNCTIONS buildflag. |
|
@DedeHai I'll take a closer look in the next days. Question: did you test with a larger panel, like 128x64 or 64x64? One reason for the HUB75 driver-level buffer was to prevent redraw flickering - if the show() loop ist faster than the screen draw "beam", we can be sure that tearing/flickering will only happen rarely. Maybe it's only visible with effects that have lots of motion and color changes. The other reason - and we surely don't need that any more - was to support getPixelColor on the driver level. |
|
I did test on 64x64 and saw no tearing, but I also did not push it. Wheter the DMA driver is fed from global buffer or a local buffer should not make any difference should it? at the point where the (old) local buffer was pushed, the contents of that buffer and global buffer is identical and not changing. Or what am I missing that gets "out of sync"? |
True, that's not the difference. My concern was more on how fast can we update the DMA driver data? A local loop (in |
|
valid point, I see the difference now. Could maybe grant show() access to the _pixels[] buffer (if it does not already have it) and push pixels from there. That would break ledmaps though. |
2 participants
Saves on RAM and is faster.
Dirty buffer is kept as it is faster if there are black pixels and not much slower if there are not.
Also changed PSRAM allocation rule to use PSRAM for large buffers which allows larger setups to run. Tested this part on S3 only using 128x128. 64x64 still uses DRAM mostly. Did not check what is possible on ESP32 with PSRAM.
Also fixed a bug perventing to actually use MAX_LEDS (off by 1)
Summary by CodeRabbit
Bug Fixes
Performance
Documentation