Keep PWM phases constant

[s-hadinger]

Fixes #7054, PWM channels phases can change over time causing visible flickering on LED drivers (Tasmota).

This fix makes sure the PWM pulses are kept in sync, phases constant whatever the delay of the NMI.

[earlephilhower]

I'm traveling this week and don't have my logic analyzer or board handy to test, but I think the changes proposed here are on-the-whole beneficial. Let me throw some waveforms out and see what I can see when I get back this weekend.

[earlephilhower]

Logically, I get what you're trying to here. But it seems like this is a no-op unless we have overshot by an entire waveform cycle, no? If it's been less than a waveform cycle then the mod will be a no-op. The mod operator is a relatively expensive one, might consider just ignoring the case for code speed.

[s-hadinger]

Correct, this should be a no-op most of the time. It's a safeguard because if we overshoot one or multiple wafeforms, the PWM will oscillate at 500KHz (1ms up + 1ms down) until the required number of cycles is reached. I don't know if it's even possible, or if 500KHz for a very short amount of time is an issue at all.
We could also add a while (-cyclesToGo > wave->nextTimeHighCycles + wave->nextTimeLowCycles) { cyclesToGo += wave->nextTimeHighCycles + wave->nextTimeLowCycles)}; but that would also add code size to the precious IRAM.

I'm pretty sure this would never occur with PWM, only if someone sets a very short waveform. You can probably drop this for now.

[earlephilhower]

Could you comment it out and make a note in the code about why we're skipping, so if something pops up later we can see the reasoning behind the choice?

[s-hadinger]

I removed this line of code for nom but leaved the two variants in comments.

[earlephilhower]

What this and the other branch seem to be doing is to be preserving the waveform phase at expense of period jitter.

Could it be that the pulsing you're seeing on some LED boards is due to periods of some pins slightly increasing (when the total on-time is so small that a few 10s of 80mhz instructions would actually increase the %on-time appreciably)? And this, by introducing a feedback on the period makes things average out by continually adding jitter to the last-checked pin so that over large amounts of time the cyclesToGo overrun is subtracted off of the hi/low time?

I think it's a fair thing to do, given that when cyclesToGo < 0 then bad stuff has already happened, actually. Feels like a good idea.

[s-hadinger]

Indeed, if cyclesToGo is already significantly negative, then we already missed the right timing for the coming edge. Either we keep timing of the next edge, at the expense of phase shifting (original code), or we compensate the missed timing and shorten the next portion to keep phase stable (new version).

Missing one cycle is not visible, whereas shifting the phase creates a visible artefact.

Also min_u32(wave->nextTimeHighCycles + cyclesToGo, 1) is to make sure that we trigger the change at the next microsecond. My first version without the min_u32 resulted in missing an edge at some rare occasion and creating flashes. It looks safe with this now.

[dok-net]

Sorry to interfere, but from my work on the concurring PR #7022 I believe recomputing the nextServiceCycle based on now, instead of incrementally, is at the heart of the problem. I am also not even giving the Timer1 a head start anymore, because any delay until a specific pin is reached should in general remain the same for each transition.
So, in the context of this here PR, what happens if you don't do the cyclesToGo compensation you are implementing, but instead of
wave->nextServiceCycle = now + wave->nextTimeHighCycles;
use
wave->nextServiceCycle += wave->nextTimeHighCycles;
? (Same for low cycle)
Never mind cycle timings that are too short to run reliably.

[s-hadinger]

Thanks. I will try your PR #7022 not before 2-3 days.
@dok-net IRAM is a very precious resource, I realized your patch is actually smaller in IRAM that the current code with my patch. So that's a super good news.

[dok-net]

@s-hadinger I'm sorry, though mitigated by fix and feature, the IRAM size is actually larger in my PR, from master

IROM   *: 228428          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27400   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1252  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 680   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25168 )         - zeroed variables      (global, static) in RAM\HEAP

to PR #7022

IROM   *: 228460          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27492   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1252  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 680   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25240 )         - zeroed variables      (global, static) in RAM\HEAP

A prominent fix is that 100% duty or off cycle now work without pulsating on the output.
A specified duration now also leaves the output at the state it is at that moment, so no more force to off when stopped during duty cycle. One can always force thereafter via digitalWrite(pin, <state>), after all. One might call this a breaking change, I consider it a bug fix.
Harder to fix without further increasing RAM usage is maintaining phase when duty cycle changes, but period not: (duty_before+off_before) == (duty_new + off_new). Is this something you consider a requirement?

[dok-net]

@s-hadinger I think this PR should not be merged, on the grounds that

min_u32(nextEventCycles, min_u32(wave->nextTimeHighCycles + cyclesToGo, 1));

is near 100% of the time 1, but otherwise always 0.
A quick inspection seems to indicate that this causes another forced loop-iteration, executing

       } else {
          uint32_t deltaCycles = wave->nextServiceCycle - now;
          nextEventCycles = min_u32(nextEventCycles, deltaCycles);
        }

which more or less revokes the effect of the change in this PR a few CPU cycles later.
So whatever is observed as an improvement is a side-effect of iterating inside the NMI handler.
Am I seriously missing something here headscratch?

[earlephilhower]

A prominent fix is that 100% duty or off cycle now work without pulsating on the output.

Err, how would a 100% duty cycle actually get into this section of code, though? The Tone is by definition a 50% cycle and analogWrite checks for 100% duty cycle and just does digitalWrites to avoid any CPU use at all.

A specified duration now also leaves the output at the state it is at that moment, so no more force to off when stopped during duty cycle. One can always force thereafter via digitalWrite(pin, <state>), after all. One might call this a breaking change, I consider it a bug fix.

That would be a breaking change. The original code special-cased it and there are probably HW installations out there depending on this behavior. A 3.0 tweak, not a 2.x one.

Harder to fix without further increasing RAM usage is maintaining phase when duty cycle changes, but period not: (duty_before+off_before) == (duty_new + off_new). Is this something you consider a requirement?

The API specifies a frequency and has no guarantees about phase relationships. You can't know what phases there are, in general, since the app code is running async to the timer...

min_u32(nextEventCycles, min_u32(wave->nextTimeHighCycles + cyclesToGo, 1));

As @dok-net says, this does always equal 0 or 1. I think you meant 1 to be microsecondsToClockCycles(1). However, that still means it hits the NMI at 1us and not only on edges which will eat CPU like mad.

[s-hadinger]

What I know is that min_u32(nextEventCycles, wave->nextTimeHighCycles + cyclesToGo); does not work, because can introduce a negative number causing an edge transition to be skipped. So there's maybe a more elegant way to force it to be non-negative.

[dok-net]

@s-hadinger a much reworked version is in #7022 now. Changing duty cycle maintains phase if period duration remains the same. Runtime is honored precisely, not only on next incidental timer event. Overall stability, correctness and performance fixes, "works for me" (buzzer, servo, PWM PC fan control). I think that's good for a start.
I don't have measuring equipment to graph timings etc, so basically, my observations that the PR is a major improvement for my use cases is all I can provide in the way of proofing.

[dok-net]

@earlephilhower

Err, how would a 100% duty cycle actually get into this section of code, though?

First, in the case of direct use of waveform :-) Second, loss of phase for analogWrite()ing min or max values seemed unacceptable to me (magnitude: bug), this here PR seems to confirm that sentiment. I think an explicit from-sketch digitalWrite() to fully stop the PWM generator is correct use.

That would be a breaking change. The original code special-cased it and there are probably HW installations out there depending on this behavior.

Hem. stopWaveform() in master just removes the pin from signal generator, effectively, asynchronously, leaving the pin in a random state, right? OTOH, startWaveform() to set a finish time to an already running waveform cancels the generation at an unpredictable time in master, but is well defined in PR #7022 - at least per my objective. In master, this has nasty effects like in PR #7084 whenever the duty cycle is cut short, for instance. If the expectation is that startWaveform() with a running time is always used on first call for a given inactive pin, and the running time is a multiple of period, then there is no functional change between master and #7022.
Is there a need to stay compatible to broken uses?

[earlephilhower]

@s-hadinger, I'm still not convinced that analogWrite phase relationships need to be guaranteed, but this is an indirect way of getting there. A more direct and more easily guaranteed way would be to go back and use the original concept and install a timer callback at 1/analoghz which sets all analogwrite pins to 1 at t=0 then pulls pins down as appropriate. By construction all analog pins would then go high at exactly the same nanosecond (module GPIO16). That was what @me-no-dev 's original code did.

[s-hadinger]

@earlephilhower Don't get me wrong, I actually don't think that all PWM should start at the same nanosecond. Ideally it would be better to spread them in time which would stress less the power supply. We could even think of an additional parameter to analogWrite() specifying the phase, this would allow to evenly spread PWM pulses over time during a complete cycle.

The only feature needed is that relative phases between PWM channels remain constant in time - which is NOT the case with the current implementation.

[Def3nder]

Hi @s-hadinger, I'm engaged in the project WLED and added support for analog (5050) LED strips. We have the same flicker especially at low brightness.

I did try really a lot and it definitively has nothing to do with the PSU (neither the power nor the quality). I really like to test your PR #7057 but did not "replace" the arduino core before.

I did include

https://github.com/s-hadinger/Arduino.git#patch-1

in the platformio.ini lib_deps_external section and the dependency tree does show

|-- <Arduino> #7f2c4e9

Is this the right way doing it ?

However: the effect is kind of "slower" but there is still a slight color variation if you use RGB-only strips and try to emulate a warm white like RGB 0xFFBF8E.
I don't know if it's important, but I initialise the analog PINs with

analogWriteRange(255);
analogWriteFreq(880);

and the refresh is programmed to be not faster than every 15 ms.

[s-hadinger]

Just copy core_esp8266_waveform.cpp in your project. The linker will take your local version over the Arduino lib.

What do you mean refresh every 15ms. Are you calling analogWrite every 15ms? We don't refresh in Tasmota unless the value changed.

[Def3nder]

Hi @s-hadinger,
yes - as there is the NeoPixelBus library running all the timw (and changing LED values) as well as FastLED, we refresh in every loop (about 4000 times per second) and skip if the last analogWrite is closer than 15ms.

WLED is a NeoPixel LED driver project and the support for analog LED stripes was added later.
So, there are mostly digital LED effects (we have 100 different ones right now). As the LED stripes can sync to other devices, I want to stay with all the effects even though most of them make no sense whe using a one-color-at-a-time strip.

I will try the local core_esp8266_waveform.cpp - does this make any changes compared to including the complete "patch-1" branch ?

...unfortunately this did not have the desired effect for me.

[Def3nder]

Hi @s-hadinger, hi @earlephilhower
I've included the core_esp8266_waveform.cpp in our project.

I can confirm: the flicker is gone !

So the desired effect is there.

[dok-net]

@Def3nder Have you checked the complete discussion to this PR? You've also just bought into thrashing TIMER1 at 7 or 8µs interval :-)

[Def3nder]

Hi @dok-net,

yes, I did - let me say "try to understand" the discussion.
And I can also do the same kind of check with your PR #7022 .

Let me do this and then I report back what the effect on analog LEDs is.
(I do have an osciloscope by hand so I will try to catch some jitter / out-of-phase events if any are still there).

[dok-net]

@Def3nder great, thanks. My PR has turned into such a major rewrite that any proof that it doesn't break previous contracts for major deployments, plus provides noticeable improvements in operation and additional features, is a must, for it to go anywhere. My little "Tone sounds better and servos and PWM fans jitter a bit less" may not cut it alone.
Reading about the color LED stripes, I imagine keeping phase while changing duty cycle could be a major and visible improvement, not just in flicker, but also in color. Can you confirm such an effect?

[s-hadinger]

@Def3nder Thanks for reporting. The flicker being gone shows that phases are kept constant while calling analogWrite at sustained frequency.

I will not be able to test before next week.

[Def3nder]

Hi @s-hadinger, hi @dok-net, hi @earlephilhower,

summary:
both PRs do remove a lot of flicker but both leave some of it visible.
With my eyes I cannot judge which is the best - they are simmilar.
Both solutions lead to not having any phase changes anymore.

details:
I did include the two files from PR #7022 in the same manner and recorded two things:
a) the LED strip at low brightness YouTube
b) the TFT monitor of the mini-Osciloscope YouTube

the same repeated for PR #7057:
a) the LED strip at low brightness YouTube
b) the TFT monitor of the mini-Osciloscope YouTube

Just as a comparison this had been the situation before: YouTube

If we just look at the Osziloskope screens, PR #7022 looks more "constant", however this cannot be seen with the eyes only.

I need to say that our PWM usage in the project is a kind of special: we update the value every 15ms at 880Hz PWM frequency, because we "grab" the colors from a NeoPixelLibrary for digital LED strips.

Now my final questions would be:

1. what could I further do to test if the waveform generator does work as expected ?
   Are there good example sketches ? I have quite a lot of ESP,
   so I could run some test "in parallel" and compare both solutions.
2. what is a good use case to determine that something is broken with the changes ?

[s-hadinger]

@Def3nder I suspect that setting the PWM value every 15 ms adds some unnecessary stress to keeping phase constant. Did you consider comparing the pwm value to the previous one and avoiding repeated analogWrite when it's not necessary?

[Def3nder]

Hi @s-hadinger, yes - it's definitivly worth doing that.

So we considder the actual implementation as a kind of stress-test.
And I do have the first results:

The code (PR #7022) does run "live" in one of my WLED ceiling LED strips and the LEDs are switched on it does reboot after about 6 to 12 minutes. I did repeat it like 10 times and every time I switch on the lights it will crash after a short time. The crash is not dependant on the number of channels used for PWM: it crashes even when using the white channel on 50% only.

So: PR #7022 is not doing well when analogWrite is call every 15ms.

I will do the same test with PR #7057 this evening.

[dok-net]

@Def3nder I've checked the sources over and again, it's highly unlikely that analogWrite() with the same parameter values, called every 15ms, has any part in effecting crashes. Perhaps it's just a case of the WDT triggering in a loop that doesn't yield soon enough?

[s-hadinger]

Do you have the stack trace of the crash? It would tell us more and whether it's WDT

[Def3nder]

Hi @s-hadinger, I started with the other test: PR #7057 with all the other unchanged: this did not crash for hours. When no WLED effect is running we have about 4000 loops per second - If a WDT would trigger the reset because of enabling PR #7022, this would mean that 4 analogWrite (RGB + W channel) would take too long - hmmm - let's see what the dump says.

The device currently is in my dauther's room (and she sleeps), so I will grab it tomorrow and plug it to USB to connect the serial monitor with PR #7022. Then I will post a crash dump.

By the way: I hooked up my big oscilloscope: this is the unpatched version: YouTube (the two channel constantly change phases)
...and this one with PR #7057: YouTube (phases don't change)

[Def3nder]

Hi @s-hadinger, you were right: the watchdog restarted the ESP:

ets Jan  8 2013,rst cause:4, boot mode:(3,7)

wdt reset
load 0x4010f000, len 1384, room 16 
tail 8
chksum 0x2d
csum 0x2d
vbb28d4a3

But the question is: why does he do this only with PR #7022 implemented and in no other cases ?
The program does run with about 4000 loops per second and there are several yield() in the loop.

So I do not have any clue how a WDT can be triggered.

[s-hadinger]

I suppose that the new code takes too long, either in analogWrite or in the interrupt handler. @dok-net what do you think?

[dok-net]

I was going to suggest something to try, based on the same assumption that the timings in my PR cause the loop() to slow down too much. We should definitely start working with and posting MCVEs here, otherwise it's all easily just hearsay.
@Def3nder how did you use PR #7022, and how are you using the ESP8266 Arduino core? I would like to ask you to git checkout either master from this repository and apply either PR in orderly fashion, or just use the waveform branch from my personal repository. You'll have to familiarize yourself, if you have never done so before, on how to get the toolchain, basically, after cloning, do a cd tools; py get.py from the command line - it's in the README, IIRC. Then whip up a minimal sketch that drives waveform generators, that will work without special hardware input so we can all test it locally, too.

For a quick check, you can, before taking the long route above, check two things.
Is your sketch using the timer callback, setTimer1Callback, in any way?
Locate, in core_esp8266_waveform.cpp,

  // Firing timer too soon, the NMI occurs before ISR has returned.
  // But, must fire timer early to reach deadlines for waveforms.
  if (nextTimerCcys < microsecondsToClockCycles(4))
    nextTimerCcys = microsecondsToClockCycles(2);
  else
    nextTimerCcys -= microsecondsToClockCycles(2);

and change the minimum trigger rate, like

if (nextTimerCcys < microsecondsToClockCycles(11))
    nextTimerCcys = microsecondsToClockCycles(8);
  else
    nextTimerCcys -= microsecondsToClockCycles(3);

[Def3nder]

Hi @dok-net,

I did not use the whole Arduino Core with included PR#7022 - I only included the files changed in the PR and added them locally. I did the same with PR #7057.

This is what I did:
I use VScode with PlatformIO extension - the platformio.ini defines the arduino core:
platform = [email protected]

Then I did add the three files from PR #7022 to the dependencies subfolder, so PlatformIO will notice and compile the files. Just to be sure I added an include in the main file:

#include "src/dependencies/arduino/core_esp8266_waveform.h"

That's all.

Comming to your question: WLED does not use any hardware timers, but it uses the NeoPixelBus library. But I just checked this - the NeoPixelBus library has no occurence of setTimer1Callback in the code either.

I will test your code changes ...

EDIT: so here comes the test.

First of all: we switched from [email protected].2 to [email protected].3 and the WDT reset is gone with PR #7022 (both versions: without the code change and with it).

Differences:

1. The phases between channels are constant in PR "Phase Locked" Waveform: fix significant jitter, that stresses servos and is clearly audible in Tone output #7022
   
   while they are always at a random level with PR Keep PWM phases constant #7057
   
   

2. However the phase remains constant when the PWM isn't changed.

3. The visible flicker is higher with PR "Phase Locked" Waveform: fix significant jitter, that stresses servos and is clearly audible in Tone output #7022 (Youtube) than with PR Keep PWM phases constant #7057 (YouTube)
   the Osziloscope somehow shows this, too: PR "Phase Locked" Waveform: fix significant jitter, that stresses servos and is clearly audible in Tone output #7022 (YouTube), PR Keep PWM phases constant #7057 (YouTube)

So, the good thing: no WDT reboot when using latest Arduino Core.

Like the Tasmota Team we also did include the fixed core_esp8266_waveform.cpp from @s-hadinger.

[dok-net]

@Def3nder Thank you for the very detailed analysis. Not using platform.io myself, I can't make any sense of their version numbering.
Let me repeat that PR #7057 is illegal in that it causes the ISR to thrash, so any findings applicable to that change are irrelevant until that is fixed. I am somewhat surprised that there's no impact on your use case, but perhaps that's just because your tests (and Tasmota's, for that matter?) are strictly focused on the PWM phase, and are missing how much performance degradation this is causing overall.
Without further evidence, I'd like to assume that the difference in flicker between #7022 and #7057 are due to that thrashing.
Really relevant to me would be a comparison between stock ESP8266 Arduino core and #7022, if the better phase handling is at the expense of any worse timing in other aspects, I'd like to hear about it.

[s-hadinger]

@dok-net I'm not aware of the thrashing you are mentioning.

I removed cyclesToGo = -((-cyclesToGo) % (wave->nextTimeHighCycles + wave->nextTimeLowCycles)); as asked by @earlephilhower. Indeed the modulus is probably too CPU intensive to do in the interrupt handler.

The latest version has lower IRAM impact, and shouldn't do any thrashing. Did I miss something else?

[dok-net]

@s-hadinger With regard to thrashing, @earlephilhower mentioned this, too:

As @dok-net says, this does always equal 0 or 1. I think you meant 1 to be microsecondsToClockCycles(1). However, that still means it hits the NMI at 1us and not only on edges which will eat CPU like mad.

@Def3nder You have me confused, how can you use PR #7022 and yet, at the same time, say

Like the Tasmota Team we also did include the fixed core_esp8266_waveform.cpp from @s-hadinger.

? Which I consider a bad idea, because now ANY change to core_esp8266_waveform.cpp in core is ignored completely unless manually patched into the local copy. Why not use git, then at least conflicts get reported on merge. Besides, it's not fixed, it thrashes inside the ISR's loop and between IRQs per @earlephilhower's and my analysis.

[Jason2866]

@dok-net there is no core patch necessary to override a Arduino core file with PlatformIO.
We are doing this since core 2.4.x

[Def3nder]

Hi @dok-net, as soon as a solution to the PWM phases is implemented in the Arduino Core, we will just remove the local copy of core_esp8266_waveform and use the library-internal version again.

Until then the local version does work well.

This is the advantage of using PlatformIO: you can include part of a library locally and the linker will care about prefering local copies.

I hope that my test did help somehow and that you all find a good solution for the Arduino library 😃

[Tech-TX]

@Def3nder unt @s-hadinger

I need to say that our PWM usage in the project is a kind of special: we update the value every 15ms at 880Hz PWM frequency, because we "grab" the colors from a NeoPixelLibrary for digital LED strips.

For your info, if you run the PWM at non-harmonic multiples of (1/(1E-6 * 1023)) then you're missing steps in the PWM range. At low brightness that might look like an obvious intensity change (or flicker) as the duty cycle changes, where more steps would smooth that out. Just for fun, you might try increasing your PWM to 977Hz and see what that does for the flicker with the current git. I haven't pulled s-hadinger's changes yet, but I'm about to do so. I only have a short 2 meter length of RGBW LEDs and no decent demo code to test them with yet so I can't see what you are seeing.

I don't know if crystal frequency (the accuracy of 80MHz) makes a difference to the PWM timing, or if it's based solely off of the number of ets_timer clocks. I'd guess it's a fixed divisor. That 977Hz should be a stable number with all of the edges, from testing I've done with 3 different boards.

Here's the note from my observation on PR7022:

All 1022 PWM steps are available at 977Hz, 488Hz, 325Hz, 244Hz, 195Hz, 162Hz, 139Hz, 122Hz, 108Hz, 97Hz, 88Hz, 81Hz, 75Hz, etc. Calculation = truncate(1/(1E-6 * 1023)) for the PWM frequencies with all (or most) discrete PWM steps. (master)

The variable in that is 1us, 2us, 3us, 4us etc. There are some missing steps in the lowest end of the range no matter the PWM frequency, but it's down in the 1% duty cycle range. The LEDs are effectively off at that level, I'd imagine.

[dok-net]

@Def3nder, @s-hadinger Please let me point out once more that this should and can be easily fixed:

@earlephilhower said:

min_u32(nextEventCycles, min_u32(wave->nextTimeHighCycles + cyclesToGo, 1));

As @dok-net says, this does always equal 0 or 1. I think you meant 1 to be microsecondsToClockCycles(1). However, that still means it hits the NMI at 1us and not only on edges which will eat CPU like mad.

@s-hadinger
s-hadinger 23 days ago Author Contributor
What I know is that min_u32(nextEventCycles, wave->nextTimeHighCycles + cyclesToGo); does not work, because can introduce a negative number causing an edge transition to be skipped. So there's maybe a more elegant way to force it to be non-negative.

I suggest the effected code fragments be changed to this to keep the ISR from thrashing, and to reduce some obfuscation, which saves us 12 bytes in IRAM cache as a nice side effect:

static inline ICACHE_RAM_ATTR int32_t max_32(int32_t a, int32_t b) {
    if (a < b) {
        return b;
    }
    return a;
}

wave->nextServiceCycle += wave->nextTimeHighCycles;
nextEventCycles = min_u32(nextEventCycles,
  max_32(wave->nextTimeHighCycles + cyclesToGo, microsecondsToClockCycles(1)));

and

wave->nextServiceCycle += wave->nextTimeLowCycles;
nextEventCycles = min_u32(nextEventCycles,
  max_32(wave->nextTimeLowCycles + cyclesToGo, microsecondsToClockCycles(1)));

respectively. As a side note, the typing of ...Cycles as unsigned is universally wrong, which causes unsigned arithmetic despite a negative cyclesToGo, breaking stuff in unexpected places - which I missed in the first version of this code fragment, too.

Probably 1 cycle instead of microseconds would be OK, or 0, the way above just prevents nextEventCycles from taking a value is could not before, such that there is less risk of causing regressions without further code review.

[dok-net]

@earlephilhower To be fair, I've amended this PR, please review the code fragments above, and if you approve, please ask the PR's author to adopt them. My servo is very (not perfectly) quiet with this.

[s-hadinger]

@dok-net Thanks a lot, this is indeed a very good improvement. Let me adopt it in this PR.

@Tech-TX I agree that 977Hz would be better than 880Hz, to make sure not to skip any step. For LEDs this is less of an issue, because we apply Gamma correction meaning that we don't actually use all steps. Any non-linearity due to skipped steps is actually not visible.

[Tech-TX]

What I see in this latest commit looks OK. Channel-to-channel phase jitter is ~12.6us, and that includes the 0 and 1023 endpoints where it has to stop and restart the PWM. I don't have a 4-channel scope any longer so I can't see it with the equivalent of an RGBW LED strip. Servo jitter looks to be a little better than current master.

[devyte]

@Tech-TX @s-hadinger I'm inclined to close this in favor of #7192 once my comment there about drift gets addressed. It is my understanding that #7192 is a strict improvement on current master, and that it doesn't introduce any new issue.
@Tech-TX could you please confirm that about #7192?
@s-hadinger could you please test #7192 for phase shift correctness?

[s-hadinger]

This last change was introduced in the latest Tasmota version.

When wifi connection happens, the PWM interrupt handler is given much less CPU windows. The current code tries to keep phase constant sacrificing PWM level. During Wifi connection this sometimes almost shut downs PWM.

When the interrupt handler has a delay of more than 25% of the overall cycle, it switches back to previous mode, favoring PWM level rather than keep phase constant. This result in slight dimming.

[dok-net]

@s-hadinger If phase has any meaning in your use case, the latest commits don't make things any better. Correcting the overshoot would have to be proportional, I believe, so for your 25% in one cycle, you'd have to add 25% to the other cycle (duty/idle), no just let the phase drift. But if you keep throwing special handling at the ISR, it will bloat and become slower all the time.
I've measured a hardware minimum phase offset of 1µs between two adjacents GPIOs. The PR #7022 has a minimum execution time of 4.5µs for a single channel PWM. If traveling phases at plus/minus 5µs are visible with these LED strips, what I think is needed is low-pass filtering of the signal in external hardware, instead of trying to make the ESP8266 perform real time beyond its performance envelope.
That said, please use bool for Boolean values instead of int32_t, my first impulse was OMG :-)

[s-hadinger]

Sure, this is still a quick and dirty patch to make sure I'm back to the original behavior when wifi connection happens. I'm patiently waiting for the definitive solution from you or Earle. I tested all three solutions and they all had the same flaw during wifi connection.

Agree on the bool vs int32_t, it ends up in a 32 bits register anyways.

I'm sharing here how Tasmota code has patched PWM, if this helps others.

[earlephilhower]

@s-hadinger any objection to closing this in deference to #7022 or #7231 ?

[s-hadinger]

Sure