"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. |
earlephilhower
left a comment
earlephilhower
left a comment
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.
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 :-) |
…re adjust target ccy instead of ccount.
…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). |
9 participants
startWaveformCycles(pin, timeHighCycles, timeLowCycles, runTimeCycles)), runTimeCycles gets applied to the next started FULL period, instead of randomly relative to time of call.