HUB75 bugfixes for 4-scan and chained panels + new "Seengreat" pinout#5662
HUB75 bugfixes for 4-scan and chained panels + new "Seengreat" pinout#5662softhack007 wants to merge 8 commits into
Conversation
align with settings page that also allows up to 64x128 per panel
* prevent panels going flatter each time that cfg.json is saved * correct VirtualMatrixPanel setup: provide real panel dimensions * only set chainType when chain length > 1 * use a chaintype that does not flip the display upside-down
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughConstructor initializes a virtual-display flag and adjusts matrix sizing for half- and quarter-scan HUB75 types, adds an ESP32‑S3 PSRAM pinout variant, selects a chaining orientation for virtual displays, constructs the virtual panel with scaled dimensions, and updates getPins() to report scaled dimensions when virtual. ChangesHUB75 Virtual Display Configuration and Chaining
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
enhancement
hardware
change local variables to unsigned (=32bit)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| _isVirtual = true; | ||
| chainType = CHAIN_BOTTOM_LEFT_UP; // TODO: is there any need to support other chaining types? | ||
| DEBUGBUS_PRINTF_P(PSTR("Using virtual matrix: %ux%u panels of %ux%u pixels\n"), _cols, _rows, mxconfig.mx_width, mxconfig.mx_height); | ||
| if (chainLength > 1 && (_rows > 1 || _cols > 1)) chainType = CHAIN_TOP_RIGHT_DOWN; // we need to use a _DOWN chainType, otherwise the display is upside-down |
There was a problem hiding this comment.
the condition is a duplicate, better to change the initial condition bc.type == TYPE_HUB75MATRIX_QS
also what effect does chainType have? CHAIN_BOTTOM_LEFT_UP works fine on my chained panel.
There was a problem hiding this comment.
The chain type is typically used for multiple physical panels and is a common technique of physically instal panels upside down to keep cables shorter, see the driver docs for details.
I've yet to see a true 128 wide panel myself, the ones I've seen have been two 64x64 mounted side by side, so you need a chain length of 2 per panel, but that could get tricky once you then try and use multiple physical panels to get both halves of the panel the right way up
There was a problem hiding this comment.
also what effect does chainType have? CHAIN_BOTTOM_LEFT_UP works fine on my chained panel.
This should - normally - only be relevant when you have chained panels. Similar our 2D setup, chained panels can be connected from top down, left to right or even "sepentine" to save wire length.
For my 4-scan panel, CHAIN_BOTTOM_LEFT_UP always results in 180° rotated display where everything is upside-down, even without chaining panels. TOP_RIGHT_DOWN is the only one where I still had the "correct" orientation. I've compared to CHAIN_NONE and to 2-scan mode, just to be sure I'm not holding the panel wrongly.
| if (pinArray) { | ||
| pinArray[0] = mxconfig.mx_width; | ||
| pinArray[1] = mxconfig.mx_height; | ||
| pinArray[0] = _isVirtual ? mxconfig.mx_width /2 : mxconfig.mx_width; |
There was a problem hiding this comment.
Originally I was tracking QS virtual panels separately from regular use of virtual panels, we possibly need to use separate flag not the isVirtual
| // HUB75_I2S_CFG::i2s_pins _pins={R1_PIN, G1_PIN, B1_PIN, R2_PIN, G2_PIN, B2_PIN, A_PIN, B_PIN, C_PIN, D_PIN, E_PIN, LAT_PIN, OE_PIN, CLK_PIN}; | ||
| mxconfig.gpio = {4, 5, 6, 7, 15, 16, 18, 8, 3, 42, 9, 40, 2, 41}; | ||
|
|
||
| #elif defined(SEENGREAT_V2_PINOUT) |
There was a problem hiding this comment.
Probably worth adding the S3 check to that define
There was a problem hiding this comment.
More to come - the board has a second pinout for esp32 classic, and it even comes in "V1" and "V2" pinout variants. 4 new printouts in total 😅
ensure that BusHub75Matrix::getPixelColor() returns RGBW format, not RGBA. Currently busses.getPixelColor() is not used by the WLED core.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/bus_manager.cpp (1)
828-845:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the 64px chain-memory clamp below the real height assignment.
This guard now reads
mxconfig.mx_heightbefore the constructor copiespanelHeightinto it, so the single-panel fallback is based on stale/default state instead of the current bus config. That can let 64px chained panels bypass the safeguard, or clamp smaller panels unnecessarily depending on the library default.Suggested fix
- if (mxconfig.mx_height >= 64 && (mxconfig.chain_length > 1)) { - `#if` defined(BOARD_HAS_PSRAM) // limitation to one panel only applies to boards without PSRAM - if (!psramFound() || ESP.getPsramSize() == 0) // PSRAM sanity check - `#endif` - { - DEBUGBUS_PRINTLN(F("WARNING, only single panel can be used of 64 pixel boards due to memory")); - mxconfig.chain_length = 1; - } - } - - if (bc.type == TYPE_HUB75MATRIX_HS) { - mxconfig.mx_width = min(128U, panelWidth); // UI limit is 128 - mxconfig.mx_height = min(64U, panelHeight); + const unsigned clampedPanelWidth = min(128U, panelWidth); + const unsigned clampedPanelHeight = min(64U, panelHeight); + + if (bc.type == TYPE_HUB75MATRIX_HS) { + mxconfig.mx_width = clampedPanelWidth; // UI limit is 128 + mxconfig.mx_height = clampedPanelHeight; } else if (bc.type == TYPE_HUB75MATRIX_QS) { _isVirtual = true; - mxconfig.mx_width = min(128U, panelWidth) * 2; - mxconfig.mx_height = min(64U, panelHeight) / 2; + mxconfig.mx_width = clampedPanelWidth * 2; + mxconfig.mx_height = clampedPanelHeight / 2; mxconfig.driver = HUB75_I2S_CFG::FM6124; } else { DEBUGBUS_PRINTLN("Unknown type"); return; } + + if (clampedPanelHeight >= 64 && mxconfig.chain_length > 1) { + `#if` defined(BOARD_HAS_PSRAM) + if (!psramFound() || ESP.getPsramSize() == 0) + `#endif` + { + 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
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 828 - 845, The 64-pixel chain-memory clamp is checking mxconfig.mx_height before panelHeight is copied in, so move the entire guard that inspects mxconfig.mx_height and possibly forces mxconfig.chain_length = 1 to after the code that assigns mxconfig.mx_height from panelHeight (the branches that handle bc.type == TYPE_HUB75MATRIX_HS and TYPE_HUB75MATRIX_QS where mxconfig.mx_height is set); keep the PSRAM conditional and its psramFound()/ESP.getPsramSize() check intact but run it after the mx_height assignment so the clamp uses the real panel height.
♻️ Duplicate comments (1)
wled00/bus_manager.cpp (1)
1019-1029:⚠️ Potential issue | 🟠 MajorSplit “uses virtual panel” from “quarter-scan geometry was rescaled.”
This is the same
_isVirtualconflation called out earlier: regular chained panels now enter the virtual path too, butVirtualMatrixPanel(...)andgetPins()always undo QS scaling with/2and*2. For non-QS chains that corrupts both runtime mapping and the dimensions written back to config.Suggested direction
- if (chainLength > 1 && (_rows > 1 || _cols > 1) || bc.type == TYPE_HUB75MATRIX_QS) { - _isVirtual = true; + const bool isQuarterScan = (bc.type == TYPE_HUB75MATRIX_QS); + const bool usesVirtualPanel = (chainLength > 1 && (_rows > 1 || _cols > 1)) || isQuarterScan; + if (usesVirtualPanel) { + _isVirtual = true; if (chainLength > 1 && (_rows > 1 || _cols > 1)) chainType = CHAIN_TOP_RIGHT_DOWN; - DEBUGBUS_PRINTF_P(PSTR("Using virtual matrix: %ux%u panels of %ux%u pixels\n"), _cols, _rows, mxconfig.mx_width/2, mxconfig.mx_height*2); + const unsigned physicalWidth = isQuarterScan ? (mxconfig.mx_width / 2) : mxconfig.mx_width; + const unsigned physicalHeight = isQuarterScan ? (mxconfig.mx_height * 2) : mxconfig.mx_height; + DEBUGBUS_PRINTF_P(PSTR("Using virtual matrix: %ux%u panels of %ux%u pixels\n"), _cols, _rows, physicalWidth, physicalHeight); } ... - virtualDisp = new VirtualMatrixPanel((*display), _rows, _cols, mxconfig.mx_width/2, mxconfig.mx_height*2, chainType); + virtualDisp = new VirtualMatrixPanel((*display), _rows, _cols, physicalWidth, physicalHeight, chainType);That same “quarter-scan only” flag should also drive
getPins().Also applies to: 1165-1166
🤖 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 1019 - 1029, The code conflates "virtual chain" with "quarter-scan geometry" causing non-QS chains to be rescaled; introduce a separate boolean (e.g., _isQuarterScan or _isQS) set from (bc.type == TYPE_HUB75MATRIX_QS) and keep _isVirtual for chained panels, then change the VirtualMatrixPanel(...) call and any getPins() usage to use conditional scaling only when _isQS (use mxconfig.mx_width / ( _isQS ? 2 : 1 ) and mxconfig.mx_height * ( _isQS ? 2 : 1 )), and ensure the chainType assignment (chainType = CHAIN_TOP_RIGHT_DOWN) remains based on chainLength and rows/cols only; apply the same split/conditional scaling at the other VirtualMatrixPanel/getPins call sites mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wled00/bus_manager.cpp`:
- Around line 828-845: The 64-pixel chain-memory clamp is checking
mxconfig.mx_height before panelHeight is copied in, so move the entire guard
that inspects mxconfig.mx_height and possibly forces mxconfig.chain_length = 1
to after the code that assigns mxconfig.mx_height from panelHeight (the branches
that handle bc.type == TYPE_HUB75MATRIX_HS and TYPE_HUB75MATRIX_QS where
mxconfig.mx_height is set); keep the PSRAM conditional and its
psramFound()/ESP.getPsramSize() check intact but run it after the mx_height
assignment so the clamp uses the real panel height.
---
Duplicate comments:
In `@wled00/bus_manager.cpp`:
- Around line 1019-1029: The code conflates "virtual chain" with "quarter-scan
geometry" causing non-QS chains to be rescaled; introduce a separate boolean
(e.g., _isQuarterScan or _isQS) set from (bc.type == TYPE_HUB75MATRIX_QS) and
keep _isVirtual for chained panels, then change the VirtualMatrixPanel(...) call
and any getPins() usage to use conditional scaling only when _isQS (use
mxconfig.mx_width / ( _isQS ? 2 : 1 ) and mxconfig.mx_height * ( _isQS ? 2 : 1
)), and ensure the chainType assignment (chainType = CHAIN_TOP_RIGHT_DOWN)
remains based on chainLength and rows/cols only; apply the same
split/conditional scaling at the other VirtualMatrixPanel/getPins call sites
mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e96884a1-51d6-4c4a-9caa-e51f368ebf4c
📒 Files selected for processing (1)
wled00/bus_manager.cpp
|
@softhack007 offtopic: what is up with the rabbit adding random labels? |
That was an experiment, I've allowed it to directly add the labels it suggests. Initially it looked meaningful, but it's true we see to many "random" labels. I'll switch that feature off. |
softhack007
removed
hardware
3 participants
Improvements:
Bugfixes for 4-scan (aka QS) panels
mxconfigdimensionsSummary by CodeRabbit
New Features
Bug Fixes