Experimental: H-bridge safe mode for TYPE_ANALOG_2CH (cheap hardware?)#5442
Experimental: H-bridge safe mode for TYPE_ANALOG_2CH (cheap hardware?)#5442bsvdoom wants to merge 4 commits intowled:V5-C6from
Conversation
…clusive PWM outputs)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
did you model this off the current main implementation where I already had implemented this? |
|
@bsvdoom what's the reason to make this PR against our btw, analog output currently is totally broken in the V5 branch ... not sure how that works for you? |
softhack007
added
the
rebase needed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
383-386:⚠️ Potential issue | 🔴 CriticalConstructor early-return prevents TYPE_ANALOG_2CH_HBRIDGE initialization.
This is a direct consequence of the type value issue:
isPWM(96)returnsfalsebecause 96 is outside the analog range (41-47). The constructor returns at line 386, leaving the bus invalid.This confirms that the
TYPE_ANALOG_2CH_HBRIDGEvalue must be changed to fall within the analog type range, orisPWM()must be updated to recognize value 96.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 383 - 386, The BusPwm constructor is early-returning because isPWM(...) doesn't recognize the current numeric value of TYPE_ANALOG_2CH_HBRIDGE (96); fix by either changing the TYPE_ANALOG_2CH_HBRIDGE enum/constant to a value inside the analog range (41–47) or update isPWM(...) to explicitly treat TYPE_ANALOG_2CH_HBRIDGE as a PWM/analog type; locate BusPwm::BusPwm and the isPWM implementation and apply one of these two fixes so the constructor does not return prematurely.
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
491-493: Minor: Trailing whitespace on line 491.Line 491 has trailing whitespace after the comment. Consider removing it for consistency.
Also, the fallthrough to
TYPE_ANALOG_2CHlogic is intentional and works correctly since H-bridge mode always has one channel at 0, so_data[0] + _data[1]still yields the correct total brightness.🧹 Proposed fix
- case TYPE_ANALOG_2CH_HBRIDGE: //warm white + cold white, relies on auto white calculation, uses H-bridge for better dimming performance at low brightness + case TYPE_ANALOG_2CH_HBRIDGE: //warm white + cold white, relies on auto white calculation, uses H-bridge for better dimming performance at low brightness🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 491 - 493, Remove the trailing whitespace at the end of the comment on the case label for TYPE_ANALOG_2CH_HBRIDGE in bus_manager.cpp; locate the case TYPE_ANALOG_2CH_HBRIDGE block (the lines returning RGBW32(0,0,0, _data[0]) and RGBW32(0,0,0, _data[0] + _data[1])) and trim the extra space after the comment to keep formatting consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 457-470: The TYPE_ANALOG_2CH_HBRIDGE case in bus_manager.cpp is
missing a break, causing fallthrough that lets subsequent cases
(TYPE_ANALOG_5CH, TYPE_ANALOG_4CH, TYPE_ANALOG_3CH) overwrite the H-bridge
outputs; inside the switch case handling TYPE_ANALOG_2CH_HBRIDGE (the block that
reads Bus::_cct, computes level and brightness and writes _data[0]/_data[1]) add
a break statement at the end of that case to stop fallthrough so the H-bridge
logic for _data[0] and _data[1] remains intact.
- Around line 462-468: The H-bridge CCT transition produces a dead zone at
level==127/128 because both branches yield zero (see level, _data, map,
brightness); fix by ensuring the midpoint does not produce zero on both
channels—e.g., adjust the branch threshold or map ranges so one branch includes
the midpoint (use >= or <= consistently), or add a small overlap/smoothing
around the midpoint (compute a non-zero fallback using brightness when map
returns 0) so that either _data[0] or _data[1] is non-zero during the transition
to avoid a momentary blackout.
In `@wled00/const.h`:
- Line 328: The defined TYPE_ANALOG_2CH_HBRIDGE (value 96) lives in the reserved
range and is not treated as an analog/PWM type by Bus::isPWM, causing BusPwm to
skip initialization and pin counting to omit its pins; fix by either (A) change
the macro TYPE_ANALOG_2CH_HBRIDGE to a value inside the analog PWM range (41–47)
so Bus::isPWM and Bus::numPWMPins handle it normally, or (B) update the
classification helpers (Bus::isPWM and Bus::numPWMPins) to explicitly treat
TYPE_ANALOG_2CH_HBRIDGE as a valid analog/PWM type (and ensure BusPwm
constructor recognizes it) so the type is initialized and counted correctly.
---
Outside diff comments:
In `@wled00/bus_manager.cpp`:
- Around line 383-386: The BusPwm constructor is early-returning because
isPWM(...) doesn't recognize the current numeric value of
TYPE_ANALOG_2CH_HBRIDGE (96); fix by either changing the TYPE_ANALOG_2CH_HBRIDGE
enum/constant to a value inside the analog range (41–47) or update isPWM(...) to
explicitly treat TYPE_ANALOG_2CH_HBRIDGE as a PWM/analog type; locate
BusPwm::BusPwm and the isPWM implementation and apply one of these two fixes so
the constructor does not return prematurely.
---
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Around line 491-493: Remove the trailing whitespace at the end of the comment
on the case label for TYPE_ANALOG_2CH_HBRIDGE in bus_manager.cpp; locate the
case TYPE_ANALOG_2CH_HBRIDGE block (the lines returning RGBW32(0,0,0, _data[0])
and RGBW32(0,0,0, _data[0] + _data[1])) and trim the extra space after the
comment to keep formatting consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ffc2d63f-470d-46c7-8ed1-16a866aafe3a
📒 Files selected for processing (3)
wled00/bus_manager.cppwled00/bus_manager.hwled00/const.h
wled00/bus_manager.cpp
Outdated
| uint8_t brightness = w; | ||
|
|
||
| if (level <= 127) { | ||
| _data[0] = (map(level, 0, 127, 255, 0) * brightness) / 255; |
There was a problem hiding this comment.
The use of map() is not ideal here, because it leads to jumps in the final brightness result that can be avoided:
if (level <= 127) {
data[0] = (127-level) * brightness) / 127; // ramp down, brightest at 0
[...]
} else {
data[1] = ((level-128) * brightness) / 127; // ramp up, brightest at 255
[...]
}EDIT: I've initially overlooked the direction of your "map". I've corrected my proposal accordingly.
wled00/bus_manager.cpp
Outdated
| break; | ||
| case TYPE_ANALOG_2CH_HBRIDGE: //warm white + cold white, relies on auto white calculation, uses H-bridge for better dimming performance at low brightness | ||
| { | ||
| uint8_t level = (Bus::_cct < 0 || Bus::_cct > 255) ? 127 : Bus::_cct; |
There was a problem hiding this comment.
int16_t Bus::_cct = -1; // -1 means use approximateKelvinFromRGB(), 0-255 is standard, >1900 use colorBalanceFromKelvin()The white from auto-white calculation is in w; Bus::_cct has the color temperature, either in kelvin or directly from the "cct" slider.
To be honest with you, I don't think the code that I see is usable as it is, sorry. It doesn't support the same CCT modes as the normal "2channel WW+CW" mode, however its not clear to me if this can be achieved at all with a H-Bridge; @DedeHai might be able to better comment on this aspect. In general, adding a new mode to the switch-case is OK, just that the mode number must be aligned with the range reserved for PWM output (see comments from coderabbitAI). |
No, I just happened to be using the XIAO C6 for my project, that's why I made it against V5-C6, no special code here. I am not familiar with WLED code in general, that's why i could not add this as a new mode, just improvised the new LED_TYPE from the current code. Btw it works just fine if i replace the old TYPE_ANALOG_2CH with my new TYPE_ANALOG_2CH_HBRIDGE case contents. I just finished the project, and it drives my custom made h-bridge (witch lights up the 2-pin charlieplexed Christmas lights) as expected. I just use this setup in a couple of projects, and it would be nice if someone could help me implement this, so i don't have to custom build modify the code every time. (And it could be helpful to others too) I also used this method to drive the same circuit on esp8266, but that PR was declined for technical reasons. |
No, I did not see that. Can you provide me a link? |
I understand, this was just my way to solve this problem, and I am more than happy to help if someone can show me a way to achieve this in a proper way. |
wled00/bus_manager.cpp
Outdated
| unsigned deadTime = 0; | ||
|
|
||
| if (_type == TYPE_ANALOG_2CH && Bus::_cctBlend == 0) { | ||
| if ((_type == TYPE_ANALOG_2CH || _type == TYPE_ANALOG_2CH_HBRIDGE) && Bus::_cctBlend == 0) { |
There was a problem hiding this comment.
this is what I meant, you add a new type that is already supported by simply setting cctblend=0.
There was a problem hiding this comment.
Regardless of setting cctblend=0, the signal on the tow outputs (A and B) are not a complimentary inverted signal that could drive a H-bridge, the truth table is not satisfied.
There was a problem hiding this comment.
they are complementary, I spent quite a few hours writing that code. If they are not complementary for the C6, changing setpixelcolor() will not make it so, you need to implement/change the driver.
There was a problem hiding this comment.
Regardless of setting cctblend=0
@bsvdoom did you test the settings suggested by @DedeHai? Did you measure (logic analyzer needed) how it behaves with respect to avoiding shot-through?
@DedeHai I'd say lets short the discussion - my understanding is
- PWM output on C6 (and C5, too) needs a complete re-write due to major changes in esp-idf. Building anything on top of the broken "V5-C6" analog driver code is like building a house in quicksand - wasted effort.
- I'm not sure about the specifics of H-Bridges, but the code in this PR just changes the color temperature slider into a left-or-right slider for two LEDs.
- The output will be BLACK in the middle values, Left = 100% WW, right = 100% CW.
- Advanced white handling is not supported.
I don't see added value for WLED users. Especially when we already have a working solution for H-Bridge in main.
There was a problem hiding this comment.
agreed, as with all C6 / V5 code, it should be done properly to avoid a patchwork of fixes that then have to be re-adjusted later. By properly I mean write a driver that does the same as current main.
|
There is really no need to add a new type as h-bridge is already supported. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wled00/bus_manager.cpp (1)
458-459:⚠️ Potential issue | 🟠 MajorRaw
_cctbreaks the non-slider CCT modes.Line 458 collapses every
_cctvalue outside0..255to127, so Kelvin-based CCT and_cct < 0(“derive from RGB”) both land on the polarity boundary and drive both outputs to0. Please normalize_cctthrough the same relative-CCT conversion path ascalculateCCT(), or explicitly reject those modes for this bus type instead of silently falling back to midpoint/off.Based on learnings,
Bus::_cctcan hold Kelvin values, direct slider values, or-1for RGB-derived CCT, while the white intensity from auto-white is already carried inw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 458 - 459, The current assignment collapses Bus::_cct out-of-range values to 127 which breaks Kelvin and RGB-derived modes; instead, normalize _cct using the same conversion as calculateCCT() (or explicitly reject unsupported Kelvin/RGB modes). Update the level computation: if Bus::_cct == -1 call calculateCCT() or the RGB-derivation path to get the relative 0..255 CCT level, if Bus::_cct > 255 treat it as Kelvin and convert to the relative scale via calculateCCT()/the existing Kelvin->relative logic, otherwise use the raw 0..255 value; keep brightness = w as-is and do not silently fall back to midpoint.
🧹 Nitpick comments (2)
wled00/bus_manager.cpp (2)
595-595: Surface that this mode is experimental in the user-facing name or docs.
getLEDTypes()is what the UI exposes, so right now the warning about unsynchronized H-bridge switching only exists in the PR description. Adding an(experimental)marker here, or a nearby doc note, would make that risk visible before users enable it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` at line 595, The UI-exposed LED type for the H-bridge mode should indicate it's experimental: update the entry for TYPE_ANALOG_2CH_HBRIDGE (the mapping that currently uses "AA" and PSTR("PWM CCT (H-bridge)")) so the user-facing name includes an "(experimental)" marker (e.g., PSTR("PWM CCT (H-bridge) (experimental)")), and add a brief adjacent doc/UI tooltip or comment accessible via getLEDTypes() to surface the unsynchronized H-bridge switching warning to users before they enable it.
541-547: Please do a small FMEA on this polarity-flip path before merging.This branch is the software mitigation against overlap/shoot-through, but the PR notes that channel updates are still sequential and not hardware-synchronized. A quick software FMEA over frame-to-frame direction changes, dithering, timer resync, and brownout/reset cases would make it much clearer whether the current protection is adequate for typical WLED H-bridge installs.
Based on learnings, WLED maintainers have found software FMEAs useful when a PR adds a safety or reliability mitigation whose real-world coverage is still uncertain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 541 - 547, Add a short FMEA for the polarity-flip path around the analog 2-channel H-bridge logic (the branch gated by _type == TYPE_ANALOG_2CH || TYPE_ANALOG_2CH_HBRIDGE and Bus::_cctBlend == 0) that evaluates frame-to-frame direction changes, dithering effects, timer resynchronization, and brownout/reset scenarios and documents whether the current deadTime/duty adjustments (deadTime = (1+dithering) << bitShift; and the _bri/duty shortening logic) fully prevent overlap/shoot-through; list concrete failure modes, likelihood/impact, tests to reproduce (e.g., rapid direction flips, high-dithering, simulated timer drift, power cycling during flip), expected safe/un-safe outcomes, and recommended code changes if gaps are found (for example increasing deadTime, enforcing full-off windows during flips, or adding atomic update sequencing). Ensure this FMEA is added to the PR description or as a comment in bus_manager.cpp near the affected logic so reviewers can validate safety decisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/bus_manager.cpp`:
- Around line 458-459: The current assignment collapses Bus::_cct out-of-range
values to 127 which breaks Kelvin and RGB-derived modes; instead, normalize _cct
using the same conversion as calculateCCT() (or explicitly reject unsupported
Kelvin/RGB modes). Update the level computation: if Bus::_cct == -1 call
calculateCCT() or the RGB-derivation path to get the relative 0..255 CCT level,
if Bus::_cct > 255 treat it as Kelvin and convert to the relative scale via
calculateCCT()/the existing Kelvin->relative logic, otherwise use the raw 0..255
value; keep brightness = w as-is and do not silently fall back to midpoint.
---
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Line 595: The UI-exposed LED type for the H-bridge mode should indicate it's
experimental: update the entry for TYPE_ANALOG_2CH_HBRIDGE (the mapping that
currently uses "AA" and PSTR("PWM CCT (H-bridge)")) so the user-facing name
includes an "(experimental)" marker (e.g., PSTR("PWM CCT (H-bridge)
(experimental)")), and add a brief adjacent doc/UI tooltip or comment accessible
via getLEDTypes() to surface the unsynchronized H-bridge switching warning to
users before they enable it.
- Around line 541-547: Add a short FMEA for the polarity-flip path around the
analog 2-channel H-bridge logic (the branch gated by _type == TYPE_ANALOG_2CH ||
TYPE_ANALOG_2CH_HBRIDGE and Bus::_cctBlend == 0) that evaluates frame-to-frame
direction changes, dithering effects, timer resynchronization, and
brownout/reset scenarios and documents whether the current deadTime/duty
adjustments (deadTime = (1+dithering) << bitShift; and the _bri/duty shortening
logic) fully prevent overlap/shoot-through; list concrete failure modes,
likelihood/impact, tests to reproduce (e.g., rapid direction flips,
high-dithering, simulated timer drift, power cycling during flip), expected
safe/un-safe outcomes, and recommended code changes if gaps are found (for
example increasing deadTime, enforcing full-off windows during flips, or adding
atomic update sequencing). Ensure this FMEA is added to the PR description or as
a comment in bus_manager.cpp near the affected logic so reviewers can validate
safety decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a4e8964-2866-414b-a453-9665a30a2f3f
📒 Files selected for processing (1)
wled00/bus_manager.cpp
|
Looking back at the discussion flow ... I still get the feeling that multiple discussion paths were mixed together, and that makes it hard for us (as maintainers) to judge added value. It also does not feel satisfying to me personally 😉 Maybe lets put source code quality aspects aside for the moment. If we align first on common reference behavior and measurable evidence, that might help to make a decision that is maintainable and ensures added value for users. From maintainers perspective, I would like to separate overlapping discussions into a clear sequence:
Edit: The |
softhack007
changed the title
softhack007
changed the title
softhack007
changed the title
current main, on C3. PWM CCT with blending set to 0.
|
remove TYPE_ANALOG_2CH_HBRIDGE
|
I removed new type as requested, and updated the code to work as I originally desired. Maybe I overlooked somewhere, but I could not achieve such a behavior in WLED, without modifying the code. @softhack007 Since the signals are PWM, a logic analyzer won't do. And I do not have a scope. I can make a new PR aginst Thank you for your attention. |
which ESP does not exhibit this behavior and how do you know without a scope? |
Why? I have one of these cheap 8-channel logic analyzers here, and it's no problem to measure two output signals in parallel at high frequency (even 1MHz is no problem). The logic analyzer is definitely fast enough to capture two PWM output signals (digital signal) at the neccessarily time resolution. You don't need to attach you XMas lights, measuring the esp32 output pins is sufficient. Edit: @bsvdoom just to be sure - you know how PWM works, right? It's actually a digital on-off signal at high frequency, nothing analogue. To control a H-Bridge, the only thing to do is make sure that two PWM outputs (channels) are interlocked so they are never "digital high" at the same point in time. |
I am not sure if we need a new PR - we are still at step 1 "feature parity check", to understand if there is a problem / gap in the existing behaviour of WLED. |
|
@softhack007 Yes, my bad. 0: ~63: 127: ~192: 255: This is the expected/desired behavior for my x-mas lights. |
|
The correct way to do it is to implement the driver as it is in main. This is just a hack so closing. |
|
@DedeHai Thanks for the clarification — that makes sense. I realize this implementation is more of a workaround than a proper integration with the current driver architecture. My goal was mainly to explore a way to support H-bridge–driven 2-wire setups without having to maintain a custom build. I’m not very familiar with the internal driver structure in WLED yet, so I wasn’t sure how to implement this in the “correct” way. Would you recommend opening a feature request for this use case, or is there already a proper extension point where this kind of behavior could be implemented? I’d be happy to rework this in a cleaner way if you can point me in the right direction. |
the help is appreciated, its good explore the C6 and the way forward. If you want to push on: the implementation of "the correct way" is just a few lines below the ones you edited, the PWM driver is quite simple (compared to the digital drivers) but in IDF V5 I hear everything changed and its a new API. |
|
the whole "magic" happens in anything before that are just pre-calculations (CIE brightness formula approximation math, determining bit depth from frequency etc.) For starts a fixed frequency would be acceptable if that eases things but since the hardware is still the same and just the API changed, its just a question of finding out how to make the new V5 API do the same as the V4. if you want to take a step back: implement the driver as an Arduino sketch as that is much quicker to compile and test and once that works, port it into WLED. Unfortunatel AI are not really helpful with driver development, sometimes they come up with a working framework but mostly they lead you down a wrong path. |
3 participants
Add H-bridge compatible mode for TYPE_ANALOG_2CH_HBRIDGE
(mutually exclusive PWM outputs)
Summary
This PR modifies the behavior of TYPE_ANALOG_2CH in WLED to better support 2-wire CCT LED strips driven via an H-bridge (reverse polarity).
Instead of driving both channels simultaneously (as in standard WW/CW blending), this change ensures that only one channel is active at a time, based on the CCT value.
Motivation
When using an H-bridge for polarity-based CCT control:
Driving both outputs simultaneously can lead to short circuits (shoot-through) inside the H-bridge
The current implementation (Bus::calculateCCT) assumes independent LED channels, which is unsafe for this use case
This PR introduces a safer mapping:
CCT range 0–127 → Channel A active
CCT range 128–255 → Channel B active
Never both at the same time
Implementation
Adds the new TYPE_ANALOG_2CH_HBRIDGE handling with:
case TYPE_ANALOG_2CH_HBRIDGE:
{
uint8_t level = (Bus::_cct < 0 || Bus::_cct > 255) ? 127 : Bus::_cct;
uint8_t brightness = w;
if (level <= 127) {
_data[0] = (map(level, 0, 127, 255, 0) * brightness) / 255;
_data[1] = 0;
} else {
_data[0] = 0;
_data[1] = (map(level, 128, 255, 0, 255) * brightness) / 255;
}
}
break;
Behavior
Ensures mutually exclusive outputs
Maps CCT slider directly to polarity direction
Preserves brightness via w scaling
Current Status (Needs Review)
This implementation works in principle, but is not perfect:
Output switching is not hardware-synchronized
GPIO/PWM updates happen sequentially
There may still be very short overlap periods (µs range) between channels
Behavior near the midpoint (127/128) may not be perfectly smooth
Because of this, I consider this:
Experimental / partial solution
Request for Review
I would really appreciate feedback from maintainers or contributors on:
Whether this approach fits WLED’s design philosophy
If there is a better way to integrate H-bridge support
Possibility of:
synchronized PWM updates
using ESP32 LEDC features for safer switching
or introducing a dedicated “H-bridge mode” instead of modifying default behavior
Safety Notes
This change reduces the risk of shoot-through by avoiding simultaneous activation of both outputs in software
However, perfect synchronization between GPIO updates is not guaranteed
On ESP32, PWM channels are updated sequentially, which may still introduce very short overlap periods (µs range)
Users should still:
Prefer H-bridge drivers with built-in shoot-through protection / dead-time
Avoid high-power configurations without proper hardware safeguards
Scope / Limitations
Applies only to TYPE_ANALOG_2CH_HBRIDGE
Intended for H-bridge-driven 2-wire CCT setups
Not suitable for motor control or general bidirectional loads
Notes
Ignores Bus::calculateCCT() intentionally
Uses Bus::_cct directly for deterministic channel selection
This aligns with the existing warning in LED settings regarding H-bridge usage and enforces safer behavior at the firmware level.
Summary by CodeRabbit