Preserve phase relationship when waveform updated #7192
Conversation
Preserve the phase relationship as close as possibly by taking an entire period of the waveform and copying it over to the machine state, instead of doing it on rising and falling edges.
Remove quantization artifacts at high PWM rates by using CPU clock cycles (and not microseconds) for analogWrite.
Use a doorbell and mailbox to handle updates to a waveform's duty cycle instead of attempting (but not succeeding as @mcspr noted!) to block NMI. There is no Tone race condition with this path because the expiry cycles are set to infinite or a new value immediately before the compare and assignment, so the IRQ could not disable the pin before we update things.
When globals are used, GCC needs to load an address from literal memory and then load/store the value. Even if the two globals are immediately following each other in the code, GCC can't know this at compile time so emits the indirect sequence. Move the globals to a struct which allows GCC to load the address in a single variable and then use base+offset addressing to access related globals. This reduces code size and increases code speed somewhat. IRAM usage for the ISR went from 0x263 to 0x231 bytes, a savings of 50 bytes.
|
Master, 40kHz PWM: I find PR7022 has a more narrow red line versus PR7192, which should indicate that overall the error distribution is better for PR7022. |
| if (i == 16) { | ||
| GP16O |= 1; // GPIO16 write slow as it's RMW | ||
| } else { | ||
| SetGPIO(mask); | ||
| } | ||
| wave->nextServiceCycle = now + wave->nextTimeHighCycles; | ||
| nextEventCycles = min_u32(nextEventCycles, wave->nextTimeHighCycles); | ||
| wave->nextServiceCycle = now + wave->timeHighCycles; |
There was a problem hiding this comment.
I think this is causing a drift in the waveform, because it doesn't consider the time elapsed between the timer firing and the measurement of now (assuming a single waveform).
Instead, consider:
´´´cpp
wave->nextServiceCycle += wave->timeHighCycles;
´´´
which comes from #7057, as pointed out by @dok-net.
| } else { | ||
| if (i == 16) { | ||
| GP16O &= ~1; // GPIO16 write slow as it's RMW | ||
| } else { | ||
| ClearGPIO(mask); | ||
| } | ||
| wave->nextServiceCycle = now + wave->nextTimeLowCycles; | ||
| nextEventCycles = min_u32(nextEventCycles, wave->nextTimeLowCycles); | ||
| wave->nextServiceCycle = now + wave->timeLowCycles; |
There was a problem hiding this comment.
Same as previosu comment, consider:
´´´cpp
wave->nextServiceCycle += wave->timeHighCycles;
´´´
|
To be clear: this PR is not expected to fix all recent issues with PWM. This PR is only meant to fix the most immediate and urgent issues while #7022 gets sorted out and optimized to match or improve on current performance and behavior. When #7022 is ready, and once everyone is in agreement about it, it will supersede the current implementation and then it will be merged. |
As mentioned by @devyte and others, when you calculate the time for a new edge, if you use the exact time you're doing it instead of having edges = n * period, you will have slip due to there being some delta between the ideal (n*period) and sum(nowX, period). That could cause slippage of edges in absolute terms. Simply assume the edge was handled on the exact time it wanted it to be handled. This may trade off duty cycle accuracy for edge accuracy, but it seems that the edge timing is more critical in most apps. In steady state, the delta between now(n) and (n*period) should be close to constant and therefore cancel itself out.
|
Closing in favor of #7231 which directly addresses the issue of PWM alignment. |
Preserve the phase relationship as close as possibly by taking an entire
period of the waveform and copying it over to the machine state, instead
of doing it on rising and falling edges.