From 37d9fceb78bc55bd59706990c5d168c325583dee Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 4 Apr 2020 14:39:58 -0700 Subject: [PATCH 1/8] Preserve phase relationship when waveform updated 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. --- cores/esp8266/core_esp8266_waveform.cpp | 37 ++++++++++++++++++------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 59fce8695e..5614a0ed58 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -54,8 +54,10 @@ extern "C" { typedef struct { uint32_t nextServiceCycle; // ESP cycle timer when a transition required uint32_t expiryCycle; // For time-limited waveform, the cycle when this waveform must stop - uint32_t nextTimeHighCycles; // Copy over low->high to keep smooth waveform - uint32_t nextTimeLowCycles; // Copy over high->low to keep smooth waveform + uint32_t timeHighCycles; // Currently running waveform period + uint32_t timeLowCycles; // + uint32_t gotoTimeHighCycles; // Copied over on the next period to preserve phase + uint32_t gotoTimeLowCycles; // } Waveform; static Waveform waveform[17]; // State of all possible pins @@ -116,16 +118,28 @@ int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t return false; } Waveform *wave = &waveform[pin]; - // Adjust to shave off some of the IRQ time, approximately - wave->nextTimeHighCycles = microsecondsToClockCycles(timeHighUS); - wave->nextTimeLowCycles = microsecondsToClockCycles(timeLowUS); + wave->expiryCycle = runTimeUS ? GetCycleCount() + microsecondsToClockCycles(runTimeUS) : 0; if (runTimeUS && !wave->expiryCycle) { wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it } uint32_t mask = 1<gotoTimeLowCycles = microsecondsToClockCycles(timeLowUS); + wave->gotoTimeHighCycles = microsecondsToClockCycles(timeHighUS); + + // Restore interrupt state + xt_wsr_ps(_state); + } else { // if (!(waveformEnabled & mask)) + wave->timeHighCycles = microsecondsToClockCycles(timeHighUS); + wave->timeLowCycles = microsecondsToClockCycles(timeLowUS); + wave->gotoTimeHighCycles = wave->timeHighCycles; + wave->gotoTimeLowCycles = wave->timeLowCycles; // Actually set the pin high or low in the IRQ service to guarantee times wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1); waveformToEnable |= mask; @@ -263,16 +277,19 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { SetGPIO(mask); } - wave->nextServiceCycle = now + wave->nextTimeHighCycles; - nextEventCycles = min_u32(nextEventCycles, wave->nextTimeHighCycles); + wave->nextServiceCycle = now + wave->timeHighCycles; + nextEventCycles = min_u32(nextEventCycles, wave->timeHighCycles); } 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; + nextEventCycles = min_u32(nextEventCycles, wave->timeLowCycles); + // Copy over next full-cycle timings + wave->timeHighCycles = wave->gotoTimeHighCycles; + wave->timeLowCycles = wave->gotoTimeLowCycles; } } else { uint32_t deltaCycles = wave->nextServiceCycle - now; From 64509898b1439ecfb4d1810ea1db910cb63c93f5 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 4 Apr 2020 16:22:29 -0700 Subject: [PATCH 2/8] Allow clock-cycle granularity for analogWrite Remove quantization artifacts at high PWM rates by using CPU clock cycles (and not microseconds) for analogWrite. --- cores/esp8266/core_esp8266_waveform.cpp | 16 ++++++++++------ cores/esp8266/core_esp8266_waveform.h | 2 ++ cores/esp8266/core_esp8266_wiring_pwm.cpp | 3 ++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 5614a0ed58..a88df4076c 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -114,13 +114,17 @@ void setTimer1Callback(uint32_t (*fn)()) { // waveform smoothly on next low->high transition. For immediate change, stopWaveform() // first, then it will immediately begin. int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t runTimeUS) { + return startWaveformCycles(pin, microsecondsToClockCycles(timeHighUS), microsecondsToClockCycles(timeLowUS), microsecondsToClockCycles(runTimeUS)); +} + +int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCycles, uint32_t runTimeCycles) { if ((pin > 16) || isFlashInterfacePin(pin)) { return false; } Waveform *wave = &waveform[pin]; - wave->expiryCycle = runTimeUS ? GetCycleCount() + microsecondsToClockCycles(runTimeUS) : 0; - if (runTimeUS && !wave->expiryCycle) { + wave->expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; + if (runTimeCycles && !wave->expiryCycle) { wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it } @@ -130,14 +134,14 @@ int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t // Need to manually do RSIL here because this is "C" code and can't use the InterruptLock class uint32_t _state = xt_rsil(15); - wave->gotoTimeLowCycles = microsecondsToClockCycles(timeLowUS); - wave->gotoTimeHighCycles = microsecondsToClockCycles(timeHighUS); + wave->gotoTimeLowCycles = timeLowCycles; + wave->gotoTimeHighCycles = timeHighCycles; // Restore interrupt state xt_wsr_ps(_state); } else { // if (!(waveformEnabled & mask)) - wave->timeHighCycles = microsecondsToClockCycles(timeHighUS); - wave->timeLowCycles = microsecondsToClockCycles(timeLowUS); + wave->timeHighCycles = timeHighCycles; + wave->timeLowCycles = timeLowCycles; wave->gotoTimeHighCycles = wave->timeHighCycles; wave->gotoTimeLowCycles = wave->timeLowCycles; // Actually set the pin high or low in the IRQ service to guarantee times diff --git a/cores/esp8266/core_esp8266_waveform.h b/cores/esp8266/core_esp8266_waveform.h index 24ce91fb36..9a5f2ce372 100644 --- a/cores/esp8266/core_esp8266_waveform.h +++ b/cores/esp8266/core_esp8266_waveform.h @@ -50,6 +50,8 @@ extern "C" { // If runtimeUS > 0 then automatically stop it after that many usecs. // Returns true or false on success or failure. int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t runTimeUS); +// Same as above, but pass in CPU clock cycles instead of microseconds +int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCycles, uint32_t runTimeCycles); // Stop a waveform, if any, on the specified pin. // Returns true or false on success or failure. int stopWaveform(uint8_t pin); diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index 5a3481bbd8..42fc25fea0 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -25,6 +25,7 @@ #include "core_esp8266_waveform.h" extern "C" { +#include "user_interface.h" static uint32_t analogMap = 0; static int32_t analogScale = PWMRANGE; @@ -50,7 +51,7 @@ extern void __analogWrite(uint8_t pin, int val) { if (pin > 16) { return; } - uint32_t analogPeriod = 1000000L / analogFreq; + uint32_t analogPeriod = (1000000L * system_get_cpu_freq()) / analogFreq; if (val < 0) { val = 0; } else if (val > analogScale) { From e75da8750a6c7b3cdfd1ffd61d3b80122a9585ea Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 4 Apr 2020 16:58:07 -0700 Subject: [PATCH 3/8] Actually use the Cycles not the US call in analogWrite --- cores/esp8266/core_esp8266_wiring_pwm.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index 42fc25fea0..b15b95f828 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -69,7 +69,7 @@ extern void __analogWrite(uint8_t pin, int val) { stopWaveform(pin); digitalWrite(pin, LOW); } else { - if (startWaveform(pin, high, low, 0)) { + if (startWaveformCycles(pin, high, low, 0)) { analogMap |= (1 << pin); } } From 6fc6b8019e69c865dbcd2553a2102c37b8e080a0 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 5 Apr 2020 09:02:40 -0700 Subject: [PATCH 4/8] Use InterruptLock and not manual locking --- cores/esp8266/core_esp8266_waveform.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index a88df4076c..6506f43ee8 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -38,7 +38,7 @@ */ #include -#include "ets_sys.h" +#include "interrupts.h" #include "core_esp8266_waveform.h" extern "C" { @@ -130,15 +130,9 @@ int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCy uint32_t mask = 1<gotoTimeLowCycles = timeLowCycles; wave->gotoTimeHighCycles = timeHighCycles; - - // Restore interrupt state - xt_wsr_ps(_state); } else { // if (!(waveformEnabled & mask)) wave->timeHighCycles = timeHighCycles; wave->timeLowCycles = timeLowCycles; From 393cc855cc660911c5f9764732188e06763e6b1f Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 5 Apr 2020 14:13:10 -0700 Subject: [PATCH 5/8] Fix potential race condition when Tone() updating --- cores/esp8266/core_esp8266_waveform.cpp | 57 ++++++++++++++----------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 6506f43ee8..461f837f9d 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -122,37 +122,42 @@ int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCy return false; } Waveform *wave = &waveform[pin]; + uint32_t mask = 1<expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; - if (runTimeCycles && !wave->expiryCycle) { - wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it - } + do { + esp8266::InterruptLock il; // Can't have the IRQ changing variables behind our backs for this - uint32_t mask = 1<gotoTimeLowCycles = timeLowCycles; - wave->gotoTimeHighCycles = timeHighCycles; - } else { // if (!(waveformEnabled & mask)) - wave->timeHighCycles = timeHighCycles; - wave->timeLowCycles = timeLowCycles; - wave->gotoTimeHighCycles = wave->timeHighCycles; - wave->gotoTimeLowCycles = wave->timeLowCycles; - // Actually set the pin high or low in the IRQ service to guarantee times - wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1); - waveformToEnable |= mask; - if (!timerRunning) { - initTimer(); - timer1_write(microsecondsToClockCycles(10)); - } else { - // Ensure timely service.... - if (T1L > microsecondsToClockCycles(10)) { + wave->expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; + if (runTimeCycles && !wave->expiryCycle) { + wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it + } + + if (waveformEnabled & mask) { + // Just update the values to be copied on the next full period + wave->gotoTimeLowCycles = timeLowCycles; + wave->gotoTimeHighCycles = timeHighCycles; + } else { // if (!(waveformEnabled & mask)) + wave->timeHighCycles = timeHighCycles; + wave->timeLowCycles = timeLowCycles; + wave->gotoTimeHighCycles = wave->timeHighCycles; + wave->gotoTimeLowCycles = wave->timeLowCycles; + // Actually set the pin high or low in the IRQ service to guarantee times + wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1); + waveformToEnable |= mask; + if (!timerRunning) { + initTimer(); timer1_write(microsecondsToClockCycles(10)); + } else { + // Ensure timely service.... + if (T1L > microsecondsToClockCycles(10)) { + timer1_write(microsecondsToClockCycles(10)); + } } } - while (waveformToEnable) { - delay(0); // Wait for waveform to update - } + } while (0); // Run above if()/else exactly once + + while (waveformToEnable) { + delay(0); // Wait for waveform to update } return true; From 4b2562a00ec9500ac4de3eb55491d48946f58293 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 6 Apr 2020 08:15:51 -0700 Subject: [PATCH 6/8] Adjust to a lockfree cycle update logic 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. --- cores/esp8266/core_esp8266_waveform.cpp | 68 ++++++++++++++----------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 461f837f9d..9110cc7208 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -68,6 +68,10 @@ static volatile uint32_t waveformEnabled = 0; // Is it actively running, updated static volatile uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin static volatile uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation +static volatile int32_t waveformToChange = -1; +static volatile uint32_t waveformNewHigh = 0; +static volatile uint32_t waveformNewLow = 0; + static uint32_t (*timer1CB)() = NULL; @@ -122,42 +126,40 @@ int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCy return false; } Waveform *wave = &waveform[pin]; - uint32_t mask = 1<expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; + if (runTimeCycles && !wave->expiryCycle) { + wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it + } - wave->expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; - if (runTimeCycles && !wave->expiryCycle) { - wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it + uint32_t mask = 1<= 0) { + delay(0); // Wait for waveform to update } - - if (waveformEnabled & mask) { - // Just update the values to be copied on the next full period - wave->gotoTimeLowCycles = timeLowCycles; - wave->gotoTimeHighCycles = timeHighCycles; - } else { // if (!(waveformEnabled & mask)) - wave->timeHighCycles = timeHighCycles; - wave->timeLowCycles = timeLowCycles; - wave->gotoTimeHighCycles = wave->timeHighCycles; - wave->gotoTimeLowCycles = wave->timeLowCycles; - // Actually set the pin high or low in the IRQ service to guarantee times - wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1); - waveformToEnable |= mask; - if (!timerRunning) { - initTimer(); + } else { // if (!(waveformEnabled & mask)) + wave->timeHighCycles = timeHighCycles; + wave->timeLowCycles = timeLowCycles; + wave->gotoTimeHighCycles = wave->timeHighCycles; + wave->gotoTimeLowCycles = wave->timeLowCycles; + // Actually set the pin high or low in the IRQ service to guarantee times + wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1); + waveformToEnable |= mask; + if (!timerRunning) { + initTimer(); + timer1_write(microsecondsToClockCycles(10)); + } else { + // Ensure timely service.... + if (T1L > microsecondsToClockCycles(10)) { timer1_write(microsecondsToClockCycles(10)); - } else { - // Ensure timely service.... - if (T1L > microsecondsToClockCycles(10)) { - timer1_write(microsecondsToClockCycles(10)); - } } } - } while (0); // Run above if()/else exactly once - - while (waveformToEnable) { - delay(0); // Wait for waveform to update + while (waveformToEnable) { + delay(0); // Wait for waveform to update + } } return true; @@ -228,7 +230,11 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { uint32_t nextEventCycles = microsecondsToClockCycles(MAXIRQUS); uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); - if (waveformToEnable || waveformToDisable) { + if (waveformToChange >=0) { + waveform[waveformToChange].gotoTimeHighCycles = waveformNewHigh; + waveform[waveformToChange].gotoTimeLowCycles = waveformNewLow; + waveformToChange = -1; + } else if (waveformToEnable || waveformToDisable) { // Handle enable/disable requests from main app. waveformEnabled = (waveformEnabled & ~waveformToDisable) | waveformToEnable; // Set the requested waveforms on/off waveformState &= ~waveformToEnable; // And clear the state of any just started From 545fa1078e620d99a2fa0a1c0932951a3c0cd7bd Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 6 Apr 2020 09:09:14 -0700 Subject: [PATCH 7/8] Move waveform vars to a struct to save IRAM 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. --- cores/esp8266/core_esp8266_waveform.cpp | 100 ++++++++++++------------ 1 file changed, 51 insertions(+), 49 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 9110cc7208..15be8bc362 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -61,19 +61,27 @@ typedef struct { } Waveform; static Waveform waveform[17]; // State of all possible pins -static volatile uint32_t waveformState = 0; // Is the pin high or low, updated in NMI so no access outside the NMI code -static volatile uint32_t waveformEnabled = 0; // Is it actively running, updated in NMI so no access outside the NMI code -// Enable lock-free by only allowing updates to waveformState and waveformEnabled from IRQ service routine -static volatile uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin -static volatile uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation +static struct WaveformState { + volatile uint32_t waveformState = 0; // Is the pin high or low, updated in NMI so no access outside the NMI code + volatile uint32_t waveformEnabled = 0; // Is it actively running, updated in NMI so no access outside the NMI code -static volatile int32_t waveformToChange = -1; -static volatile uint32_t waveformNewHigh = 0; -static volatile uint32_t waveformNewLow = 0; + // Enable lock-free by only allowing updates to waveformState and waveformEnabled from IRQ service routine + volatile uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin + volatile uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation -static uint32_t (*timer1CB)() = NULL; + volatile int32_t waveformToChange = -1; + volatile uint32_t waveformNewHigh = 0; + volatile uint32_t waveformNewLow = 0; + uint32_t (*timer1CB)() = NULL; + + // Optimize the NMI inner loop by keeping track of the min and max GPIO that we + // are generating. In the common case (1 PWM) these may be the same pin and + // we can avoid looking at the other pins. + int startPin = 0; + int endPin = 0; +} state; // Non-speed critical bits #pragma GCC optimize ("Os") @@ -105,11 +113,11 @@ static void ICACHE_RAM_ATTR deinitTimer() { // Set a callback. Pass in NULL to stop it void setTimer1Callback(uint32_t (*fn)()) { - timer1CB = fn; + state.timer1CB = fn; if (!timerRunning && fn) { initTimer(); timer1_write(microsecondsToClockCycles(1)); // Cause an interrupt post-haste - } else if (timerRunning && !fn && !waveformEnabled) { + } else if (timerRunning && !fn && !state.waveformEnabled) { deinitTimer(); } } @@ -133,21 +141,21 @@ int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCy } uint32_t mask = 1<= 0) { + if (state.waveformEnabled & mask) { + state.waveformNewHigh = timeHighCycles; + state.waveformNewLow = timeLowCycles; + state.waveformToChange = pin; + while (state.waveformToChange >= 0) { delay(0); // Wait for waveform to update } - } else { // if (!(waveformEnabled & mask)) + } else { // if (!(state.waveformEnabled & mask)) wave->timeHighCycles = timeHighCycles; wave->timeLowCycles = timeLowCycles; wave->gotoTimeHighCycles = wave->timeHighCycles; wave->gotoTimeLowCycles = wave->timeLowCycles; // Actually set the pin high or low in the IRQ service to guarantee times wave->nextServiceCycle = GetCycleCount() + microsecondsToClockCycles(1); - waveformToEnable |= mask; + state.waveformToEnable |= mask; if (!timerRunning) { initTimer(); timer1_write(microsecondsToClockCycles(10)); @@ -157,7 +165,7 @@ int startWaveformCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t timeLowCy timer1_write(microsecondsToClockCycles(10)); } } - while (waveformToEnable) { + while (state.waveformToEnable) { delay(0); // Wait for waveform to update } } @@ -193,18 +201,18 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { // If user sends in a pin >16 but <32, this will always point to a 0 bit // If they send >=32, then the shift will result in 0 and it will also return false uint32_t mask = 1< microsecondsToClockCycles(10)) { timer1_write(microsecondsToClockCycles(10)); } - while (waveformToDisable) { + while (state.waveformToDisable) { /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ } - if (!waveformEnabled && !timer1CB) { + if (!state.waveformEnabled && !state.timer1CB) { deinitTimer(); } return true; @@ -221,40 +229,34 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { static ICACHE_RAM_ATTR void timer1Interrupt() { - // Optimize the NMI inner loop by keeping track of the min and max GPIO that we - // are generating. In the common case (1 PWM) these may be the same pin and - // we can avoid looking at the other pins. - static int startPin = 0; - static int endPin = 0; - uint32_t nextEventCycles = microsecondsToClockCycles(MAXIRQUS); uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); - if (waveformToChange >=0) { - waveform[waveformToChange].gotoTimeHighCycles = waveformNewHigh; - waveform[waveformToChange].gotoTimeLowCycles = waveformNewLow; - waveformToChange = -1; - } else if (waveformToEnable || waveformToDisable) { + if (state.waveformToChange >=0) { + waveform[state.waveformToChange].gotoTimeHighCycles = state.waveformNewHigh; + waveform[state.waveformToChange].gotoTimeLowCycles = state.waveformNewLow; + state.waveformToChange = -1; + } else if (state.waveformToEnable || state.waveformToDisable) { // Handle enable/disable requests from main app. - waveformEnabled = (waveformEnabled & ~waveformToDisable) | waveformToEnable; // Set the requested waveforms on/off - waveformState &= ~waveformToEnable; // And clear the state of any just started - waveformToEnable = 0; - waveformToDisable = 0; + state.waveformEnabled = (state.waveformEnabled & ~state.waveformToDisable) | state.waveformToEnable; // Set the requested waveforms on/off + state.waveformState &= ~state.waveformToEnable; // And clear the state of any just started + state.waveformToEnable = 0; + state.waveformToDisable = 0; // Find the first GPIO being generated by checking GCC's find-first-set (returns 1 + the bit of the first 1 in an int32_t) - startPin = __builtin_ffs(waveformEnabled) - 1; + state.startPin = __builtin_ffs(state.waveformEnabled) - 1; // Find the last bit by subtracting off GCC's count-leading-zeros (no offset in this one) - endPin = 32 - __builtin_clz(waveformEnabled); + state.endPin = 32 - __builtin_clz(state.waveformEnabled); } bool done = false; - if (waveformEnabled) { + if (state.waveformEnabled) { do { nextEventCycles = microsecondsToClockCycles(MAXIRQUS); - for (int i = startPin; i <= endPin; i++) { + for (int i = state.startPin; i <= state.endPin; i++) { uint32_t mask = 1<expiryCycle - now; if (expiryToGo < 0) { // Done, remove! - waveformEnabled &= ~mask; + state.waveformEnabled &= ~mask; if (i == 16) { GP16O &= ~1; } else { @@ -279,8 +281,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { // Check for toggles int32_t cyclesToGo = wave->nextServiceCycle - now; if (cyclesToGo < 0) { - waveformState ^= mask; - if (waveformState & mask) { + state.waveformState ^= mask; + if (state.waveformState & mask) { if (i == 16) { GP16O |= 1; // GPIO16 write slow as it's RMW } else { @@ -312,10 +314,10 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { int32_t cyclesLeftTimeout = timeoutCycle - now; done = (cycleDeltaNextEvent < 0) || (cyclesLeftTimeout < 0); } while (!done); - } // if (waveformEnabled) + } // if (state.waveformEnabled) - if (timer1CB) { - nextEventCycles = min_u32(nextEventCycles, timer1CB()); + if (state.timer1CB) { + nextEventCycles = min_u32(nextEventCycles, state.timer1CB()); } if (nextEventCycles < microsecondsToClockCycles(10)) { From 371ef22ab8750e7ddf5f25260675e4815020ef5c Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 6 Apr 2020 17:15:42 -0700 Subject: [PATCH 8/8] Favor phase over duty cycle 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. --- cores/esp8266/core_esp8266_waveform.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 15be8bc362..6d2ecbb75d 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -288,7 +288,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { SetGPIO(mask); } - wave->nextServiceCycle = now + wave->timeHighCycles; + wave->nextServiceCycle += wave->timeHighCycles; nextEventCycles = min_u32(nextEventCycles, wave->timeHighCycles); } else { if (i == 16) { @@ -296,7 +296,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { ClearGPIO(mask); } - wave->nextServiceCycle = now + wave->timeLowCycles; + wave->nextServiceCycle += wave->timeLowCycles; nextEventCycles = min_u32(nextEventCycles, wave->timeLowCycles); // Copy over next full-cycle timings wave->timeHighCycles = wave->gotoTimeHighCycles;