@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.

@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 toanalogWrite() 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.

Hi@s-hadinger, I'm engaged in the projectWLED 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.inilib_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 likeRGB 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.

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.

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 localcore_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.

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

I can confirm: the flicker is gone !

So the desired effect is there.

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

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).

@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?

@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.

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 brightnessYouTube
b) the TFT monitor of the mini-OsciloscopeYouTube

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

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 ?

@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?

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.

@Def3nder I've checked the sources over and again, it's highly unlikely thatanalogWrite() 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?

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

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)

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

ets Jan  8 2013,rst cause:4, boot mode:(3,7)wdt resetload 0x4010f000, len 1384, room 16 tail 8chksum 0x2dcsum 0x2dvbb28d4a3

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.

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

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 thewaveform 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 acd 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, incore_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);

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 = espressif8266@2.3.2

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 ofsetTimer1Callback in the code either.

I will test your code changes ...

EDIT: so here comes the test.

First of all: we switched from espressif8266@2.3.2 to espressif8266@2.3.3 andthe 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 PRKeep PWM phases constant #7057
   
   

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

3. Thevisible 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 PRKeep 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), PRKeep PWM phases constant #7057 (YouTube)

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

Like theTasmota Team we also did include the fixedcore_esp8266_waveform.cpp from@s-hadinger.

@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.

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

I removedcyclesToGo = -((-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?

@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.

@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

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 ofcore_esp8266_waveform and use the library-internal version again.

Until then the local version does work well.

This is the advantage of usingPlatformIO: 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 😃

@Def3nderunt@s-hadinger

I need to say that our PWM usage in the project is a kind of special: we update the value every 15ms at880Hz 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.

@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:

staticinline ICACHE_RAM_ATTRint32_tmax_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 preventsnextEventCycles from taking a value is could not before, such that there is less risk of causing regressions without further code review.

@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.

@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.

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.

@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?

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.

@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 :-)

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 thebool vsint32_t, it ends up in a 32 bits register anyways.

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

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

Sure