"Phase Locked" Waveform: fix significant jitter, that stresses servos and is clearly audible in Tone output #7022
Conversation
dok-net
force-pushed
the
waveform
branch
2 times, most recently
from
9938de8
to
1ccd820
Compare
|
Dear @earlephilhower , I would like to ask to you to pay attention to this major change to the waveform lib, which resolves serious problems I was/am having using it as it is in master/release 2.6.x. This PR shall (I'll fix if not ;-) ) supersede #7057. |
dok-net
changed the title
|
I tested this PR with Tasmota and confirm it solved the flikering LEDs issues. |
|
@dok-net Actually the phase change when changing duty cycle is visible. When fading from bright to low-brightness, the fading seems to bounce on the low value and increase a bit when stabilizing. |
There was a problem hiding this comment.
Thanks for the contribution! I know you've got some neat ideas, but the style changes do add friction when reviewing. I've gone over the code w/Mk1 eyeballs, but need to build and run it to see how it performs. I'm uncertain about the timeout stuff and how it adds to IRQ runtime and for what purpose. Some comments in the code as the the state transitions/etc. would be helpful.
| continue; | ||
| } | ||
| // Check for toggles etc. | ||
| if (ExpiryState::UPDATE == wave->hasExpiry) { |
There was a problem hiding this comment.
There's a whole lot of new logic for expiry detection here. What's the intended use case? The major use seems like it would be the Tone() call, which seems to not warrant so much precision.
There was a problem hiding this comment.
There were two requirements that had set myself: more precise timing (actually Servo rather than Tone, but it comes down to the same issues).
I also wanted to be able to stop a waveform precisely, and keep the line level at the moment instead of always going LOW.
All this works now - if someone could put an oscilloscope to the new code and check max frequency etc, that would be great.
There was a problem hiding this comment.
All this works now - if someone could put an oscilloscope to the new code and check max frequency etc, that would be great.
With your current code as of 4 days ago (last change), here's what I'm seeing:
The phase between 2 pins PWM-controlled jumps by 60us when the duty cycle hits the rails (0 & 1023). Maybe my little test program below is in error. Scope is triggered on the rising edge of the top trace.
Here's a short 1.5meg/8sec video of it hitting the limits and suddenly jumping by 60us: http://www.mediafire.com/file/mnc28ogkzc366or/VID_PWM_%25237022.mp4/file
(sorry for the poor quality, my phone won't focus at that distance)
const int ch1Pin = 14;
const int ch2Pin = 2;
void setup() {
}
void loop() {
// increase duty cycle, decrease for second channel
for(int dutyCycle = 0; dutyCycle < 1023; dutyCycle++){
analogWrite(ch1Pin, 1023 - dutyCycle);
analogWrite(ch2Pin, dutyCycle);
delay(3);
}
// decrease duty cycle, increase for second channel
for(int dutyCycle = 1023; dutyCycle > 0; dutyCycle--){
analogWrite(ch1Pin, 1023 - dutyCycle);
analogWrite(ch2Pin, dutyCycle);
delay(3);
}
}The duty cycle didn't have any visible jitter, only the phase between multiple channels was changing.
Further test: I split that 3ms delay, putting 1ms between PWM updates and 2ms after, and it jumps phase by about 50%,
http://www.mediafire.com/file/3czuei8qaciwjj0/VID_PWM_%25237022_2.mp4/file
There was a problem hiding this comment.
@Tech-TX Absolutely fantastic, awesome, thank you so much!
What happens proves my point - and now I should adapt analogWrite to make use of the zero-duration cycle, hold level, enhancement. Right now it calls stopWaveform, so phase can only be lost at 0 and 1023. EDIT: done.
Wit the latest change now, waveform has to be stopped by explicit digitalWrite instead of analogWrite(0) or analogWrite(PWMRANGE), to indicate that phase no longer matters. I've also checked all other uses of waveform included in core (plus core libraries), no further changes are necessary.
As for the performance impact of my PR, are there any known-good min and max frequency values for analogWrite(), that you could also graph for accuracy, whether better, same, or worse than before?
There was a problem hiding this comment.
I'm seeing strange results at all frequencies, even with the current git. For a simple test (if you don't have a 'scope), hook up an LED to a port pin and drive it with 20KHz PWM, slowly increasing from 1 to 1023 duty cycle with at least a few ms between duty cycle changes. It's not a smooth progression. Most LEDs will be able to handle 20KHz PWM (a few will even hit 200KHz). The aberrant behavior at higher frequency is pretty obvious on an LED.
It's less noticeable at 1KHz, but there aren't 1023 steps there, more like 170-180 discrete steps from full-off to full-on. At 20KHz it's more like 8 to 10 steps, with lots of jitter above 80% duty cycle, and the frequency jumps around above 90% duty cycle. I think Earle is going to look at it to make sure I haven't made a dumb newbie mistake. ;-)
There was a problem hiding this comment.
I understand the PWM resolution is 1 microsecond (80 cycles). So to get full resolution you need a complete cycle of 1024 microseconds or 976Hz. That's why in Tasmota we chose 880Hz which is close enough.
I believe linearity tests should be done at a similar frequency.
20KHz base pwm frequency means roughly being able to drive at 20MHz which is not realistic with an esp at 80MHz - unless you spend all your CPU time in the PWM loop.
There was a problem hiding this comment.
@Tech-TX when you say current git, is that the master branch or the latest push to this PR? Besides that, I am left a bit clueless how I am not seeing what you described in yesterday's oscilloscope videos. Anyway, is the phase loss part fixed now?
There was a problem hiding this comment.
Current git, not your PR. I'll start doing a more exhaustive look at yours tomorrow. I had a weird error here, and thought the current git was all screwy. Git said I was even with master, but it wouldn't work right, so I mistrusted anything I'd seen on yours. I eventually tried current git on my main desktop and it turns out the laptop had one or more corrupt files that git didn't notice. Wipe, reinstall, re-pull, and it's back again. I hate days like that...
There was a problem hiding this comment.
Sorry, I was having problems and then Real Life rudely intruded. Looking at your latest code (I just pulled it this morning, Feb 29) I see significant jitter if edges on other pins are walking past the servo edges.
For comparison, I see around 6.5us total jitter with the current master. I've checked that my master code is correct, but grabbing a copy of your PR files may be iffy as I haven't tried that before.
Update: I had a bad compile initially, but I'm still seeing ~25us jitter when other pin edges walk past each other. It's down to a rare 3.4us jitter (otherwise stable) if it's only the servo running. 99% of the time or better it's within a 900ns range, which is reasonable. I can't hear the servo when it's like that.
Here's the revised test code I'm using:
https://gist.github.com/Tech-TX/572aab942d44638566e07036fe77a4c6
If you only want to run servos, hit 'c ' for single channel PWM mode, then 'h ' to freeze the sweep, then '0d ' to disable CH1 PWM. 90s turns on the servo to mid-range. With no asynchronous edges, here's what I see:
There was a problem hiding this comment.
Next issue looks like a math error on setting the next edge. The current master works in multiples of 1us step size for the PWM duty cycle. That means the fastest PWM that has all 1022 steps will be 977Hz; at 978Hz it drops the 1/1022 step and the signal goes away. At slower PWM frequency the step size changes from 1us to 2us, and in between that it misses up to half of the PWM steps before it shifts to the next longer / shorter 1us multiple.
Your PR on the other hand is running 7.76us step sizes at 977Hz, resulting in about 1/8th the total number of steps. You can actually see it if you hook up an LED to the PWM pin, set the pause between duty cycle changes to 20ms, and then watch closely as the duty cycle approaches minimum or maximum, depending on whether you ground the other leg or put it at 3.3V. As the LED is going very dim to nearly off, you can see the last 5 or 6 levels before it goes to minimum brightness. At higher levels you can't perceive the change. With the current master the step size is smaller, and there's no visual step-wise drop in brightness. Here's the screen caps from the scope showing the difference (all settings were identical):
That last one is zoomed in to get a more accurate number on the PWM step size.
The final capture is from a logic analyzer. The top 2 traces are PWM channels with the decoded PWM duty cycle below each one. The bottom trace toggles each time the duty cycle is changed. You can see that the actual duty cycle is only changing about every 8 increments, which is that 7.6us step size.
dok-net
force-pushed
the
waveform
branch
2 times, most recently
from
65cc3f9
to
ceca205
Compare
dok-net
force-pushed
the
waveform
branch
2 times, most recently
from
4a911c4
to
fc15ce8
Compare
|
@Tech-TX Otherwise, I've found a bug and commited a fix to this PR. I am sorry, I am bit overwhelmed by your signal traces, but what I grasp pointed to a timing error and that bug would cause the transitions from duty to off cycle to happen at rather random times. I can only ask you to re-test, thanks for the great support. |
No worries! If this was easy, it'd be called 'playtime'. 😄 I'm having some more strange issues here, so I'm going to do another complete wipe and then pull your latest changes. I suddenly don't trust anything on the PC as git isn't tracking when I make changes to files any longer, or at least it's not doing what it used to. The scope is set for 'infinite persistence', so that it shows a history over about a minute's time. I'm triggered on the rising edge of the CH1 PWM, then I've moved out into the middle of the cycle so that the edges or steps in PWM change should be roughly even. The granularity of the current master is 1us, so at 977KHz PWM = 1 the pin is high for 1us, then low for 1023us. The next step PWM = 2 the pin is high for 2us. On your PR as of yesterday, that 1us step size was 7.76us, so you have/had roughly 1/8th as many steps between 0 and 1023 as the master has. Since the scope is set to STORE, it captures all of the edges as the PWM increases or decreases, showing the PWM step size. @dok-net Dirk, here's a short video showing what it looks like. First it's zoomed out on infinite persistence to show the whole waveform (I'm triggering on the bottom channel) and then zoomed in to show the edges near the end-points of (PWM = 1 / 1022). I didn't let it go to zero or 1023 so that the phase would stay constant. I have it set for 20ms at each PWM step, slowly going up and then down. Every place you see a line on the scope, the signal spent enough time at that level for the scope to record it. In this case, 20ms on each discrete PWM edge as it moves through time. Here's what I've seen so far, working back and forth between the current master and this 7022 PR:
Please understand that I haven't used PWM on the ESP8266 before, so some of those observations may be "well, of course it does that" to the rest of you. ;-) |
|
Quick update (still testing): your latest changes seem to have fixed the timing until the next edge, so it's running integer multiples of 1us for the PWM step size (depending on PWM frequency) just like the master branch. I haven't seen any really bad things thus far. It looks like you may be missing more edges near 1 and 1022, I'll have to dig into it to see for sure. |
|
Bad news: as far as your original intent (reduce servo jitter) it's not doing what you'd hoped. Your code is roughly the same overall range of jitter as the current git, but it hits a wider range more frequently. Both run around 12us maximum jitter, but the git does most of it's jitter within about a 2us window, about 1/2 of a degree of servo travel. Your code is mostly jittering over a range of 6us, with occasional excursions beyond that. 1 degree is 5.55us, and I can hear my old Hitec micro servo buzz more frequently with your code than with the current git / master. If you fire up my test program and drop the PWM sweep delay to 1 (1ms between changes, 1p ) then turn on both servos (90s and 90a) you can hear the clicks or buzz as the jitter hits the servo(s). Even if you don't have a speaker or piezo connected, turning on a tone will add more edges. The jitter happens when edges cross each other. Here's my latest test code, simplified a bit more with devyte's suggestion and with most of the delay() elements replaced by his polledTimer: Here's some screen captures of the jitter for both your PR and the current master. The delta-t at the bottom is showing the total excursion over about a minute's worth of time. After running for several minutes, they both have around 12.75us worst-case jitter, min to max. I get nearly identical results for the tone jitter. Master is primarily within a 2us band with occasional excursions wider than that (probably up to 12.75us total), and yours is within a 6us band with occasional wider excursions. I can't hear any difference on the piezo, however. I've pushed too many rounds down my Colt 45 1911 to have decent hearing. Ignore the grass on that second capture: I had a loose ground connection. If I turn off all of the 'walking edges' and have only tone output running, here's the results: |
|
@Tech-TX I've made a few improvements, but the biggest thing is that I have brought back the gcc optimize pragmas from master, and now my servo is very, if not perfectly, quiet. I hope you can retest and will find that this is now a real improvement over master (phase, precision). |
|
@Tech-TX Just letting you know that there is a problem in master HEAD related to testing #7022, if you merge the PR instead of checking out my branch, please maybe revert #7036, otherwise I am getting WDT resets all the time in my sketch, which includes the OTA update feature. I guess this doesn't hit your sketch, though. |
|
@devyte I know you've been waiting for this a while, now that I think everything works, here are some timings compared. |
|
@dok-net I've pulled your branch for testing, Dirk. I'll re-pull tonight when I get home. GMT-6 so you'll be asleep. No OTA in that stress test I'm running. :-) |
|
Way to go! Please keep an eye on the mysterious slowness every other reset or so, that I have seen earlier. I suspect it's a boot order problem, that may not hit your sketch without WiFi, though :-) |
…een consecutive periods.
…iting and stealing CPU cycles from userland.
…emove this code. Maximum period duration limit is implicit to timer, consider it documented constraint, don't runtime check in ISR.
|
This work is going to be merged. We will have two waveform implementations, together with #7231. Another PR (#7712) will immediately follow this one. It will install a new Arduino IDE menu (and a PIO #define). |
d-a-v
changed the title
* Re-implement PWM generator logic Add special-purpose PWM logic to preserve alignment of PWM signals for things like RGB LEDs. Keep a sorted list of GPIO changes in memory. At time 0 of the PWM cycle, set all pins to high. As time progresses bring down the additional pins as their duty cycle runs out. This way all PWM signals are time aligned by construction. This also reduces the number of PWM interrupts by up to 50%. Before, both the rising and falling edge of a PWM pin required an interrupt (and could shift arround accordingly). Now, a single IRQ sets all PWM rising edges (so 1 no matter how many PWM pins) and individual interrupts generate the falling edges. The code favors duty cycle accuracy over PWM period accuracy (since PWM is simulating an analog voltage it's the %age of time high that's the critical factor in most apps, not the refresh rate). Measurements give it about 35% less total error over full range at 20khz than master. @me-no-dev used something very similar in the original PWM generator. * Adjust running PWM when analogWriteFreq changed Use fixed point math to adjust running PWM channels to the new frequency. * Also preserve phase of running tone/waveforms Copy over full high/low periods only on the falling edge of a cycle, ensuring phase alignment for Tone and Servo. * Clean up signed/unsigned mismatch, 160MHz operat'n * Turn off PWM on a Tone or digitalWrite Ensure both the general purpose waveform generator and the PWM generator are disabled on a pin used for Tone/digitalWrite. * Remove hump due to fixed IRQ delta A hump in the dueling PWMs was very prominent in prior pulls. The hump was caused by having a PWM falling edge just before the cycle restart, while having the other channel requesting a 1->0 transition just outside the busy-loop window of 10us. So it gets an IRQ for channel B 0->1, then waits 2..8us for the next PWM full cycle 0->1, and ends up returning from interrupt and not scheduling another IRQ for 10us...hence the horizontal leg of the bump... Reduce the minimum IRQ latency a little bit to minimize this effect. There will still be a (significantly smaller) hump when things cross, but it won't be anywhere near as bad or detectable. * Speed PWM generator by reordering data struct Breaking out bitfields required a load and an AND, slowing things down in the PWM loop. Convert the bitfield into two separate natural-sized arrays to reduce code size and increase accuracy. * Remove if() that could never evaluate TRUE * Add error feedback to waveform generation Apply an error term to generated waveform phase times to adjust for any other ongoing processes/waveforms. Take the actual edge generation times, subtract them from the desired, and add 1/4 of that (to dampen any potential oscillations) to the next similar phase of that waveform. Allows the waveform to seek its proper period and duty cycle without hardcoding any specific calibrations (which would change depending on the codepaths, compiler options, etc.) in the source. * Move _stopPWM and _removePWMEntry to IRAM Thanks to @dok-net for noticing these need to be in IRAM as they may be called by digitalWrites in an IRQ. * Avoid long wait times when PWM freq is low * Fix bug where tone/pwm could happen on same pin * Adjust for random 160MHZ operation The WiFi stack sometimes changes frequency behind our backs, so ESP's cycle counter does not count constant ticks. We can't know how long it's been at a different than expected frequency, so do the next best thing and make sure we adjust any ESP cycles we're waiting for by the current CPU speed. This can lead to a blip in the waveform for 1 period when the frequency toggles from normal, and when it toggles back, but it should remain for the intervening periods. Should avoid a lot of LED shimmering and servo errors during WiFi connection (and maybe transmission). * Clean up leftover debugs in ISR * Subtract constant-time overhead for PWM, add 60khz PWM has a constant minimum time between loops with a single pin, so pull that time out of the desired PWM period and shift the center of the PWM frequency closer to the desired without any dynamic feedback needed. Enable 60khz PWM, even though it's not terribly useful as it causes an IRQ every ~8us (and each IRQ is 2-3us). The core can still run w/o WDT, but it's performance is about 5x slower than unloaded. * Fix GPIO16 not toggling properly. * Remove constant offset to PWM period analogWrite doesn't know about the change in total PWM cycles, so it is possible for it to send in a value that's beyond the maximum adjusted PWM cycle count, royally messing up things. Remove the offset. Also, fix bug with timer callback functions potentially disabling the timer if PWM was still active. * Remove volatiles, replace with explicit membarrier Volatiles are expensive in flash/IRAM as well as in runtime because they introduce `memw` instructions everywhere their values are used. Remove the volatiles and manually mark handshake signals for re-read/flush to reduce code and runtime in the waveform generator/PWM. * Consolidate data into single structure Save IRAM and flash by using a class to hold waveform generator state. Allows for bast+offset addressing to be used in many cases, removing `l32r` and literals from the assembly code. * Factor out common timer shutdown code * Remove unneeded extra copy on PWM start * Factor out common edge work in waveform loop * Factor out waveform phase feedback loop math * Reduce PWM size by using 32b count, indexes Byte-wide operations require extra instructions, so make index and count a full 32-bits wide. * GP16O is a 1-bit register, just write to it Testing indicates that GP16O is just a simple 1-bit wide register in the RTC module. Instead of |= and &- (i.e. RmW), use direct assignment in PWM generator. * Increase PWM linearity in low/high regions By adjusting the PWM cycle slightly to account for the fixed time through the compute loop, increase the linear response near the min and max areas. * Remove redundant GetCycleCount (non-IRQ) * Factor out common timer setup operations * Fix clean-waveform transition, lock to tone faster New startWaveform waveforms were being copied over on the falling edge of the cycle, not the rising edge. Everything else is based on rising edge, so adjust accordingly. Also, feedback a larger % of the error term in standard waveform generation. Balances the speed at which it locks to tones under changing circumstances with it not going completely bonkers when a transient error occurs due to some other bit. * Reduce IRAM by pushing more work to _setPWM Simply mark pins as inactive, don't adjust the ordered list until the next _startPWM call (in IROM). * Fix typo in PWM pin 1->0 transition Actually check the pin mask is active before setting the PWM pin low. D'oh. * Combine cleanup and pin remove, save 50 bytes IROM The cleanup (where marked-off pins are removed from the PWM time map) and remove (where a chosen pin is taken out of the PWM map) do essentially the same processing. Combine them and save ~50 bytes of code and speed things up a tiny bit. * Remove unused analogMap, toneMap Save ~100 bytes of IROM by removing the tone/analog pin tracking from the interface functions. They were completely unused. * Save IRAM/heap by adjusting WVF update struct The waveform update structure included 2 32-bit quantities (so, used 8 * 17 = 136 bytes of RAM) for the next cycle of a waveform. Replace that with a single update register, in a posted fashion. The logic now sets the new state of a single waveform and returns immediately (so, no need to wait 1ms if you've got an existing waveform of 1khz). The waveform NMI will pick up the changed value on its next cycle. Reduces IRAM by 40 bytes, and heap by 144 bytes. * Don't duplicate PWM period calculation Let the waveform generator be the single source of truth for the PWM period in clock cycles. Reduces IRAM by 32 bytes and makes things generally saner. * Factor out common PWM update code Replace repeated PWM update logic with a subroutine, and move the PWMUpdate pointer into the state itself. Reduces IROM and IRAM, removes code duplication. Also remove single-use macros and ifdef configurable options as the IRAM and IROM impact of them are now not very large. * Fix regression when analogWrite done cold Lost an `initTimer()` call in a refactoring, resulting in the core hanging forever while waiting for the NMI which will never happen. Re-add as appropriate. * Save 16b of IRAM by not re-setting edge intr bit Per @dok-net, drop the rewrite of the edge trigger flag in the timer interrupt register. It's set on startup and never cleared, so this is redundant. Drops ~16 bytes of IRAM. * Allow on-the-fly PWM frequency changes When PWM is running and analogWriteFreq is called, re-calculate the entire set of PWM pins to the new frequency. Preserve the raw numerator/denominator in an unused bit of the waveform structure to avoid wasting memory. * Adjust for fixed overhead on PWM period Pulls the actual PWM period closer to the requested one with a simple, 0-overhead static adjustment. * Fix value reversal when analogWrite out of range Silly mistake, swapped high and low values when checking analogWrite for over/under values. Fixed * Don't optimize the satopWaveform call Save a few bytes of IRAM by not using -O2 on the stopWaveform call. It is not a speed-critical function. * Avoid side effects in addPWMtoList * Adjust PWM period as fcn of # of PWM pins Results in much closer PWM frequency range over any number of PWM pins, while taking 0 add'l overhead in IRAM or in the IRQ. * Fix occasional Tone artifacts When _setPWMFreq was called the initial PWM mask was not set to 0 leading to occasional issues where non-PWM pins would be set to 1 on the nextPWM cycle. Manifested itself as an overtone at the PWM frequency +/-. * Reduce CPU usage and enhance low range PWM output Borrow a trick from #7022 to exit the busy loop when the next event is too far out. Also reduce the IRQ delta subtraction because it was initially not NMI so there was much more variation than now. Keep the PWM state machine active at a higher prio than the standard tone generation when the next edge is very close (i.e. when we're at the max or min of the range and have 2 or more near edges). Adds a lot of resolution to the response at low and high ranges. Go from relative to absolute cycle counts in the main IRQ loop so that we don't mingle delta-cycles when the delta start was significantly different. * Update min IRQ time to remove humps in PWM linearity Keep PWM error <2.0% on entire range, from 0-100%, and remove the hump seen in testC by fixing the min IRQ delay setting. * Remove minor bump at high PWM frequencies The IRQ lead time was a tiny bit undersized, causing IRQs to come back too late for about .25us worth of PWM range. Adjust the constant accordingly
startWaveformCycles(pin, timeHighCycles, timeLowCycles, runTimeCycles)), runTimeCycles gets applied to the next started FULL period, instead of randomly relative to time of call.