Skip to content

Commit

Permalink
Dithering support & bugfix in UI
Browse files Browse the repository at this point in the history
  • Loading branch information
blazoncek committed Aug 30, 2024
1 parent c51ce2e commit 0d035a0
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 40 deletions.
71 changes: 49 additions & 22 deletions wled00/bus_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <IPAddress.h>
#ifdef ARDUINO_ARCH_ESP32
#include "driver/ledc.h"
#include "soc/ledc_struct.h"
#endif
#include "const.h"
#include "pin_manager.h"
Expand Down Expand Up @@ -392,7 +393,7 @@ void BusDigital::cleanup(void) {
#define MAX_BIT_WIDTH SOC_LEDC_TIMER_BIT_WIDE_NUM
#else
// ESP32: 20 bit (but in reality we would never go beyond 16 bit as the frequency would be to low)
#define MAX_BIT_WIDTH 20
#define MAX_BIT_WIDTH 14
#endif
#endif

Expand All @@ -413,11 +414,13 @@ BusPwm::BusPwm(BusConfig &bc)
analogWriteRange((1<<_depth)-1);
analogWriteFreq(_frequency);
#else
// for 2 pin PWM CCT strip pinManager will make sure both LEDC channels are in the same speed group and sharing the same timer
_ledcStart = pinManager.allocateLedc(numPins);
if (_ledcStart == 255) { //no more free LEDC channels
pinManager.deallocateMultiplePins(pins, numPins, PinOwner::BusPwm);
return;
}
if (_needsRefresh) _depth = 8; // fixed 8 bit depth with 4 bit dithering (ESP8266 has no hardware to support dithering)

This comment has been minimized.

Copy link
@DedeHai

DedeHai Aug 31, 2024

how is the actual pwm bit depth calculated? if that is still done the same way it was before this will not work I i think. i.e. if ledc setup is set to anything besides 12 bits (you set the bits written to ledc fixed to 12 bits below if I understand correctly)

This comment has been minimized.

Copy link
@blazoncek

blazoncek Aug 31, 2024

Author Collaborator

_depth determines "physical" bit depth of the brightness value passed to ledcWrite().
This does not include fractional bits used when writing directly into LEDC duty register. scaled (below) is always sized for 12 bits (as it makes no sense to have it more detail than 12 bits, perhaps 11 would suffice as well but purists may revolt in such case).

This statement merely says "ignore calculated bit depth in relation to supplied frequency and always use 8 bit depth if user selected dithering". 8 bit depth is the lowest acceptable depth (yielding 256 distinct brightness values). 8 bit depth also ensures highest possible frequency can be used without issues (if you do not enable dithering and select highest frequency the available resolution/depth will be 8 bit).

#endif

for (unsigned i = 0; i < numPins; i++) {
Expand Down Expand Up @@ -501,38 +504,62 @@ uint32_t BusPwm::getPixelColor(uint16_t pix) const {
void BusPwm::show() {
if (!_valid) return;
const unsigned numPins = getPins();
const unsigned maxBri = (1<<_depth);

// use CIE brightness formula
unsigned pwmBri = (unsigned)_bri * 100;
if (pwmBri < 2040)
pwmBri = ((pwmBri << _depth) + 115043) / 230087; //adding '0.5' before division for correct rounding
else {
const unsigned maxBri = (1<<_depth); // possible values: 16384 (14), 8192 (13), 4096 (12), 2048 (11), 1024 (10), 512 (9) and 256 (8)

// use CIE brightness formula to fit (or approximate linearity of) human eye perceived brightness
// the formula is based on 12 bit resolution as there is no need for greater precision
unsigned pwmBri = (unsigned)_bri * 100; // enlarge to use integer math for linear response
if (pwmBri < 2040) {
// linear response for values [0-20]
pwmBri = ((pwmBri << 12) + 115043) / 230087; //adding '0.5' before division for correct rounding
} else {
// cubic response for values [21-255]
pwmBri += 4080;
float temp = (float)pwmBri / 29580.0f;
temp = temp * temp * temp * maxBri;
temp = temp * temp * temp * 4095.0f;

This comment has been minimized.

Copy link
@DedeHai

DedeHai Aug 31, 2024

why remove proper scaling here but add it back in below (which is less accurate, here we already have the float value)?

This comment has been minimized.

Copy link
@blazoncek

blazoncek Aug 31, 2024

Author Collaborator

Having resolution greater than 12 bits is of no importance and produces negligible difference.
This change ensures that pwmBri resolution is exactly 12 bit (which is needed to preserve details in range [0-32] of _bri.

This comment has been minimized.

Copy link
@DedeHai

DedeHai Aug 31, 2024

I have no objection to use 12 bits (it is, as you say more than enough) but why then support up to 14 bits which is truncated to 12bits anyways? unless I misenterpret it, I did not yet look at the full code.

This comment has been minimized.

Copy link
@blazoncek

blazoncek Aug 31, 2024

Author Collaborator

14 bits is a theoretical upper limit allowed with hardware (C3 & S2) and will also allow decent frequencies.
But, as you say, it is pointless in regards to perceived brightness. The formula that calculates used depth is based on the theoretical limits and chosen WLED_PWM_FREQ.
Currently selected WLED_PWM_FREQ will allow 12 bit depth at normal "speed" (and 14 at slow "speed" or 8 bit at fastest "speed").

Anyone having issues with slow FETs will welcome dithering support without sacrificing PWM frequency.

pwmBri = (unsigned)temp;
}
// pwmBri is in range [0-4095]

// determine phase shift
[[maybe_unused]] unsigned phaseOffset = maxBri / numPins; // (maxBri is at _depth resolution)

This comment has been minimized.

Copy link
@DedeHai

DedeHai Sep 1, 2024

this is incorrect. for CCT the shift should be 180°, this results in a fixed shift and limits the maximum CCT brightness to 50%. I will try to fix it.

This comment has been minimized.

Copy link
@blazoncek

blazoncek Sep 1, 2024

Author Collaborator

For CCT (only), numPins is 2. This will give phaseOffset == maxBri / 2, which is correct (180°).
And yes, you want maximum 50% (-1 pulse) brightness on a single channel as they must not overlap.
maxBri is unchanged and corresponds to 1<<_depth.

phaseOffset is used to set hpoint.

This comment has been minimized.

Copy link
@DedeHai

DedeHai Sep 1, 2024

phaseOffset == maxBri / 2 is actually not 180° (except on really small values it comes close). 180° means the second pulse is in the center of the off time of the first one (which is what my original code did).
also you do not want to limit to 50% brightness. if you set to full cold white for example, you want that to be 100% bright. It becomes instantly clear if looking at the wavforms of the scope (this is at full brightness setting):
image

my current fix (work in progress, phase shift is not 180° but 'cascaded' as that requires no precalculation of total on time like I did it in my original code):
image

this shows the 'cascading' if not on full brightness:
image

This comment has been minimized.

Copy link
@blazoncek

blazoncek Sep 1, 2024

Author Collaborator

also you do not want to limit to 50% brightness. if you set to full cold white for example, you want that to be 100% bright.

oops! yes, that's a bug I overlooked.

// we will be phase shifting every channel by fixed amount (i times /2 or /3 or /4 or /5)
// phase shifting is only mandatory when using H-bridge to drive reverse-polarity PWM CCT (2 wire) LED type (with 180° phase)
// CCT additive blending must be 0 (WW & CW must not overlap) in such case
// for all other cases it will just try to "spread" the load on PSU

for (unsigned i = 0; i < numPins; i++) {
unsigned scaled = (_data[i] * pwmBri) / 255;
if (_reversed) scaled = maxBri - scaled;
// adjust "scaled" value (to fit resolution bounds)
if (_depth < 12 && !_needsRefresh) scaled >>= 12 - _depth; // normalize scaled value (if not using dithering)
else if (_depth > 12) scaled <<= _depth - 12; // scale to _depth if using >12 bit
if (_reversed) scaled = maxBri - scaled;

#ifdef ESP8266
analogWrite(_pins[i], scaled);
#else
unsigned channel = _ledcStart + i;
// determine phase shift POC for PWM CCT (credit @dedehai)
// phase shifting (180°) is only available for PWM CCT LED type if _needsRefresh is true (UI hack)
// and CCT blending is 0 (WW & CW must not overlap)
// this will allow using H-bridge to drive reverse-polarity CCT LED strip (2 wires)
// NOTE/TODO: if this has no side effects we may forego UI hack and the need for _needsRefresh
// we may even use phase shift to evenly distribute power across different pins
if (_type == TYPE_ANALOG_2CH && _needsRefresh && Bus::getCCTBlend() == 0) { // hacked to determine if phase shifted PWM is requested
unsigned maxDuty = (maxBri / numPins); // numPins is 2
if (scaled >= maxDuty) scaled = maxDuty - 1; // safety check & add dead time of 1 pulse when brightness is at 50%
ledc_set_duty_and_update((ledc_mode_t)(channel / 8), (ledc_channel_t)(channel % 8), scaled, maxDuty*i);
} else
ledcWrite(channel, scaled);
if (_type == TYPE_ANALOG_2CH && Bus::getCCTBlend() == 0) {
// pinManager will make sure both LEDC channels are in the same speed group and sharing the same timer
unsigned briLimit = phaseOffset << (_needsRefresh*4); // expand limit if using dithering (_depth==8, scaled is at 12 bit)
if (scaled >= briLimit) scaled = briLimit - 1; // safety check & 1 pulse dead time when brightness is at 50%

This comment has been minimized.

Copy link
@DedeHai

DedeHai Aug 31, 2024

you should stick to the way I originally implemented dead time, that one was based off of tests I ran both with and without dithering and it made sure it had at least one pulse of dead time on both sides (one is not enough, you have two crossovers of the two pwm signals at 100% brightness)

This comment has been minimized.

Copy link
@blazoncek

blazoncek Aug 31, 2024

Author Collaborator

This formula subtracts 1 from both sides producing 1 pulse less on both sides.

I would have stuck with your formula if there was at least a bit of comment describing why deadtime = 2 + (3 << ditheringbits); was chosen. unfortunately there was none so I chose to go with theory.
Theory suggests there is no need to have more than 1 pulse width gap when switching at 50% CW/WW brightness distribution.
So I asked myself why would one want to add 50 pulses worth of dead time (and do calculations like offsetSum = scaledBri[i] + deadtime / 2 + (maxBri - total_dutycycle) / 2; each time) when you know in advance how much dead time you need (assuming 180° phase shift). 50 pulses may also produce a noticeable "dip" in perceived brightness when 50% WW/CW distribution is achieved.

Yes, my value is purely theoretical and based on the assumption that times are in sync but practical measurements only need to enlarge it to acceptable value that will satisfy no overlapping occurs.
As mentioned in previous comment (and in the code comment), scaled (at this point) has a resolution of 12 bits. I do agree that adding 1 pulse of wait time at 12 bits may be inadequate, that's where actual measurement will come into play (if need be I'll get myself a logic analyser or a scope) as I do not see this code to be finished at all.

I have tried to add as many comments into the code to describe what is happening for others to try to understand the code. If they are still inadequate I will try to make them more detailed but I want the code to be simpler in fact as simple as that reading it you would not need any comment at all.

BTW (and this is no excuse) I am still on vacation without any HW with me so once I get home I'll put my theories to test if no-one will beat me to it. 😉

This comment has been minimized.

Copy link
@DedeHai

DedeHai Aug 31, 2024

no worries, I can run some tests and put the scope on it. i just found that in higher resolution the 1 pulse on either side (which is a reasonable assumption) sometimes resulted in overlaps, so I adjusted until it never did overlap. I need to compare this new code to my original one running on actual hardware.

This comment has been minimized.

Copy link
@blazoncek

blazoncek Aug 31, 2024

Author Collaborator

I need to compare this new code to my original one running on actual hardware.

I would be very glad if you did that. Thank you.
Sometimes theories and practice disagree. That much I know. 😄

This comment has been minimized.

Copy link
@blazoncek

blazoncek Aug 31, 2024

Author Collaborator

@DedeHai you are correct, this line needs to be:

      if (scaled >= briLimit) scaled = briLimit - (1<<(_needsRefresh*4)); // safety check & 1 pulse dead time when brightness is at 50%

To accommodate 4 fractional bits when using dithering.

}
unsigned gr = channel/8; // high/low speed group
unsigned ch = channel%8; // group channel
if (_needsRefresh) {
// if _needsRefresh is true (UI hack) we are using dithering (credit @dedehai & @zalatnaicsongor)
// https://github.com/Aircoookie/WLED/pull/4115 and https://github.com/zalatnaicsongor/WLED/pull/1)
// directly write to LEDC struct as there is no HAL exposed function for dithering
// duty has 20 bit resolution with 4 fractional bits (24 bits in total)
// _depth is 8 bit in this case (and maxBri==256), scaled is still at 12 bit
LEDC.channel_group[gr].channel[ch].duty.duty = scaled; // write full 12 bit value (4 dithering bits)
LEDC.channel_group[gr].channel[ch].hpoint.hpoint = phaseOffset*i; // phaseOffset is at _depth resolution (8 bit)
ledc_update_duty((ledc_mode_t)gr, (ledc_channel_t)ch);
} else {
// scaled will be [0-((1<<_depth)-1)] and hpoint evenly distributed
ledc_set_duty_and_update((ledc_mode_t)gr, (ledc_channel_t)ch, scaled, phaseOffset*i);

This comment has been minimized.

Copy link
@DedeHai

DedeHai Aug 31, 2024

ledc_set_duty_and_update() does not seem to work. I do not get any PWM output if dithering is disabled. I have no explanation as to why that is.
but I have a working fix (and it is even better IMO):
change line 556 to:
LEDC.channel_group[gr].channel[ch].duty.duty = scaled << (!_needsRefresh*4);
and drop the if/else statement. the shift is what is done 'behind the scenes' anyway in that set_duty HAL function.

//ledcWrite(channel, scaled);
}
#endif
}
}
Expand Down
6 changes: 5 additions & 1 deletion wled00/const.h
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,11 @@
#ifdef ESP8266
#define WLED_PWM_FREQ 880 //PWM frequency proven as good for LEDs
#else
#define WLED_PWM_FREQ 19531
#ifdef SOC_LEDC_SUPPORT_XTAL_CLOCK
#define WLED_PWM_FREQ 9765 // XTAL clock is 40MHz (this will allow 12 bit resolution)
#else
#define WLED_PWM_FREQ 19531 // APB clock is 80MHz
#endif
#endif
#endif

Expand Down
27 changes: 10 additions & 17 deletions wled00/data/settings_leds.htm
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
<meta content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" name="viewport">
<title>LED Settings</title>
<script>
var d=document,laprev=55,maxB=1,maxD=1,maxA=1,maxV=0,maxM=4000,maxPB=4096,maxL=1333,maxCO=10,maxLbquot=0; //maximum bytes for LED allocation: 4kB for 8266, 32kB for 32
var d=document,laprev=55,maxB=1,maxD=1,maxA=1,maxV=0,maxM=4000,maxPB=2048,maxL=1664,maxCO=5,maxLbquot=0; //maximum bytes for LED allocation: 4kB for 8266, 32kB for 32
var oMaxB=1;
d.ledTypes = []; // filled from GetV()
d.ledTypes = [/*{i:22,c:1,t:"D",n:"WS2812"},{i:42,c:6,t:"AA",n:"PWM CCT"}*/]; // filled from GetV()
d.um_p = [];
d.rsvd = [];
d.ro_gpio = [];
Expand Down Expand Up @@ -60,7 +60,7 @@
x.className = error ? "error":"show";
clearTimeout(timeout);
x.style.animation = 'none';
timeout = setTimeout(function(){ x.className = x.className.replace("show", ""); }, 2900);
timeout = setTimeout(()=>{ x.className = x.className.replace("show", ""); }, 2900);
}
function bLimits(b,v,p,m,l,o=5,d=2,a=6) {
// maxB - max buses (can be changed if using ESP32 parallel I2S)
Expand All @@ -69,7 +69,7 @@
// maxV - min virtual buses
// maxPB - max LEDs per bus
// maxM - max LED memory
// maxL - max LEDs
// maxL - max LEDs (will serve to determine ESP >1664 == ESP32)
// maxCO - max Color Order mappings
oMaxB = maxB = b; maxD = d, maxA = a, maxV = v; maxM = m; maxPB = p; maxL = l; maxCO = o;
}
Expand Down Expand Up @@ -237,16 +237,8 @@
p0d = "Data "+p0d;
break;
case 'A': // PWM analog
switch (gT(t).t.length) { // type length determines number of GPIO used
case 1: break;
case 2: off = "Phase shift";
if (d.Sf["CB"].value != 0) gId(`rf${n}`).checked = 0; // disable phase shifting
gId(`rf${n}`).disabled = (d.Sf["CB"].value != 0); // prevent changes
// fallthrough
default: p0d = "GPIOs:"; break;
}
// PWM CCT allows phase shifting
gId(`dig${n}f`).style.display = (gT(t).t.length != 2) ? "none" : "inline";
if (gT(t).t.length > 1) p0d = "GPIOs:";
off = "Dithering";
break;
case 'N': // network
p0d = "IP address:";
Expand All @@ -259,7 +251,7 @@
gId("p1d"+n).innerText = p1d;
gId("off"+n).innerText = off;
// secondary pins show/hide (type string length is equivalent to number of pins used; except for network and on/off)
let pins = Math.min(gT(t).t.length,1) + 3*isNet(t); // fixes network pins to 4
let pins = Math.max(gT(t).t.length,1) + 3*isNet(t); // fixes network pins to 4
for (let p=1; p<5; p++) {
var LK = d.Sf["L"+p+n];
if (!LK) continue;
Expand Down Expand Up @@ -294,7 +286,7 @@
gId("dig"+n+"c").style.display = (isAna(t)) ? "none":"inline"; // hide count for analog
gId("dig"+n+"r").style.display = (isVir(t)) ? "none":"inline"; // hide reversed for virtual
gId("dig"+n+"s").style.display = (isVir(t) || isAna(t)) ? "none":"inline"; // hide skip 1st for virtual & analog
gId("dig"+n+"f").style.display = (isDig(t) || isPWM(t)) ? "inline":"none"; // hide refresh (PWM hijacks reffresh for phase shifting)
gId("dig"+n+"f").style.display = (isDig(t) || (isPWM(t) && maxL>2048)) ? "inline":"none"; // hide refresh (PWM hijacks reffresh for dithering on ESP32)
gId("dig"+n+"a").style.display = (hasW(t)) ? "inline":"none"; // auto calculate white
gId("dig"+n+"l").style.display = (isD2P(t) || isPWM(t)) ? "inline":"none"; // bus clock speed / PWM speed (relative) (not On/Off)
gId("rev"+n).innerHTML = isAna(t) ? "Inverted output":"Reversed"; // change reverse text for analog else (rotated 180°)
Expand Down Expand Up @@ -916,7 +908,8 @@ <h3>White management</h3>
<br>
Calculate CCT from RGB: <input type="checkbox" name="CR"><br>
CCT IC used (Athom 15W): <input type="checkbox" name="IC"><br>
CCT additive blending: <input type="number" class="s" min="0" max="100" name="CB" onchange="UI()" required> %
CCT additive blending: <input type="number" class="s" min="0" max="100" name="CB" onchange="UI()" required> %<br>
<i class="warn">WARNING: When using H-bridge for reverse polarity (2-wire) CCT LED strip<br><b>make sure this value is 0</b>.<br>(ESP32 variants only, ESP8266 does not support H-bridges)</i>
</div>
<h3>Advanced</h3>
Palette blending:
Expand Down

2 comments on commit 0d035a0

@DedeHai
Copy link

@DedeHai DedeHai commented on 0d035a0 Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a working version now with everything fixed (at least the basic stuff works, did not test all possible configurations).
Do you want me to make a PR with all fixes for review?
comments/changes:

  • adding actual 180° phase shift requires some pre-calculations like in my original code. it can be done but it is not really necessary. currently the two CCT signals are cascaded, on RGB or RGBW it is somwhat random due to unsynced timers.
  • dead-time calculation was incorrect, especially with dithering (there are 4 pulses of 8bit resolution required to avoid overlap)
  • maxBri calculation was wrong with dithering (resulting in bogus values when inverting the signal)
  • increased 'pwmBri' max value to 4096 (ensures full on, with 4095 there is a single pulse where signal goes low)
  • phase offset calculation fixed
  • CCT with zero blend now also goes to 100% (like with normal CCT strips), also see inline comment with screenshots
  • removed the (non working) setting of dutycycle without dithering, now dutycycle is always set accessing the ledc struct directly

bugs that remain:

  • when saving the settings, the output jumps to (almost ) full brightness every time, even though in the web UI I set a low brightness before
  • the bug I once mentioned is still there: during transitions, the PWM value is not calculated correctly. If I change brightness from 5 to 1 using transition, this is what I get in BusPwm::show():

Normal mode, no dithering (12bit) *:
duty = 9
duty = 6
duty = 4
duty = 3
duty = 1 (this would be correct for brightness = 1)
duty = 2 (this is the final value that is set)
This behaviour can be clearly seen in the light as well (and the jump happens in all transitions, not only the lowest values)

*with 'duty' I am referring to the 'scaled' value that is written to ledc.

@blazoncek
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two bugs you describe were present before these changes so they must come from elsewhere.

Let me push the changes I did yesterday and then open a PR.

Please sign in to comment.