From 2c010d69c2dd174bf7fcbb7cbca0380af47e879f Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 19 Apr 2020 07:42:22 -0700 Subject: [PATCH 01/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 180 +++++++++++++++++++++- cores/esp8266/core_esp8266_waveform.h | 7 + cores/esp8266/core_esp8266_wiring_pwm.cpp | 11 +- 3 files changed, 188 insertions(+), 10 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index e3924cf08a..ddd2da6d8c 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -40,7 +40,7 @@ #include #include "ets_sys.h" #include "core_esp8266_waveform.h" - +#include "user_interface.h" extern "C" { // Maximum delay between IRQs @@ -68,7 +68,6 @@ static volatile uint32_t waveformToDisable = 0; // Message to the NMI handler to static uint32_t (*timer1CB)() = NULL; - // Non-speed critical bits #pragma GCC optimize ("Os") @@ -108,6 +107,133 @@ void setTimer1Callback(uint32_t (*fn)()) { } } +// PWM implementation using special purpose state machine +// +// Keep an ordered list of pins with the delta in cycles between each +// element, with a terminal entry making up the remainder of the PWM +// period. With this method sum(all deltas) == PWM period clock cycles. +// +// At t=0 set all pins high and set the timeout for the 1st edge. +// On interrupt, if we're at the last element reset to t=0 state +// Otherwise, clear that pin down and set delay for next element +// and so forth. + +constexpr int maxPWMs = 8; + +// PWM edge definition +typedef struct { + unsigned int pin : 8; + unsigned int delta : 24; +} PWMEntry; + +// PWM machine state +typedef struct { + uint32_t mask; // Bitmask of active pins + uint8_t cnt; // How many entries + uint8_t idx; // Where the state machine is along the list + PWMEntry edge[maxPWMs + 1]; // Include space for terminal element + uint32_t nextServiceCycle; // Clock cycle for next step +} PWMState; + +static PWMState pwmState; +static volatile PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR +static int pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; + +// Called when analogWriteFreq() changed to update the PWM total period +void _setPWMPeriodCC(int cc) { + pwmPeriod = cc; + // TODO - should we drop all waveforms? +} + +// Helper routine to remove an entry from the state machine +static void _removePWMEntry(int pin, PWMState *p) { + if (!((1<mask)) { + return; + } + + int delta = 0; + int i; + for (i=0; i < p->cnt; i++) { + if (p->edge[i].pin == pin) { + delta = p->edge[i].delta; + break; + } + } + // Add the removed previous pin delta to preserve absolute position + p->edge[i+1].delta += delta; + // Move everything back one and clean up + for (i++; i <= p->cnt; i++) { + p->edge[i-1] = p->edge[i]; + } + p->mask &= ~(1<cnt--; +} + +// Called by analogWrite(0/100%) to disable PWM on a specific pin +bool _stopPWM(int pin) { + if (!((1<= maxPWMs) { + return false; // No space left + } else if (p.cnt == 0) { + // Starting up from scratch, special case 1st element and PWM period + p.edge[0].pin = pin; + p.edge[0].delta = cc; + p.edge[1].pin = 0xff; + p.edge[1].delta = pwmPeriod - cc; + p.cnt = 1; + p.mask = 1<high transition. For immediate change, stopWaveform() // first, then it will immediately begin. @@ -189,7 +315,7 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { while (waveformToDisable) { /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ } - if (!waveformEnabled && !timer1CB) { + if (!waveformEnabled && !pwmState.cnt && !timer1CB) { deinitTimer(); } return true; @@ -216,7 +342,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); if (waveformToEnable || waveformToDisable) { - // Handle enable/disable requests from main app. + // 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; @@ -225,12 +351,56 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { startPin = __builtin_ffs(waveformEnabled) - 1; // Find the last bit by subtracting off GCC's count-leading-zeros (no offset in this one) endPin = 32 - __builtin_clz(waveformEnabled); + } else if (!pwmState.cnt && pwmUpdate) { + // Start up the PWM generator by copying from the mailbox + pwmState = *(PWMState*)pwmUpdate; + pwmUpdate = nullptr; + pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! + pwmState.idx = pwmState.cnt; // Cause it to start at t=0 } bool done = false; - if (waveformEnabled) { + if (waveformEnabled || pwmState.cnt) { do { nextEventCycles = microsecondsToClockCycles(MAXIRQUS); + + // PWM state machine implementation + if (pwmState.cnt) { + uint32_t now = GetCycleCountIRQ(); + int32_t cyclesToGo = pwmState.nextServiceCycle - now; + if (cyclesToGo <= 10) { + if (pwmState.idx == pwmState.cnt) { // Start of pulses, possibly copy new + if (pwmUpdate) { + // Do the memory copy from temp to global and clear mailbox + pwmState = *(PWMState*)pwmUpdate; + pwmUpdate = nullptr; + } + GPOS = pwmState.mask; // Set all active pins high + // GPIO16 isn't the same as the others + if (pwmState.mask & 0x100) { + GP16O |= 1; + } + pwmState.idx = 0; + } else { + do { + // Drop the pin at this edge + GPOC = 1< analogScale) { @@ -63,15 +65,14 @@ extern void __analogWrite(uint8_t pin, int val) { uint32_t low = analogPeriod - high; pinMode(pin, OUTPUT); if (low == 0) { - stopWaveform(pin); + _stopPWM(pin); digitalWrite(pin, HIGH); } else if (high == 0) { - stopWaveform(pin); + _stopPWM(pin); digitalWrite(pin, LOW); } else { - if (startWaveformCycles(pin, high, low, 0)) { - analogMap |= (1 << pin); - } + _setPWM(pin, high); + analogMap |= (1 << pin); } } From 118103b23604ccc6e359d729cffd0f2dd744e898 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 19 Apr 2020 13:30:11 -0700 Subject: [PATCH 02/56] Adjust running PWM when analogWriteFreq changed Use fixed point math to adjust running PWM channels to the new frequency. --- cores/esp8266/core_esp8266_waveform.cpp | 25 +++++++++++++++++++++-- cores/esp8266/core_esp8266_wiring_pwm.cpp | 3 ++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 858b61a02c..1f6871bbdf 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -138,12 +138,33 @@ typedef struct { static PWMState pwmState; static volatile PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR -static int pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; +static uint32_t pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(int cc) { + if (pwmState.cnt) { + // Adjust any running ones to the best of our abilities by scaling them + // Used FP math for speed and code size + uint64_t oldCC64p0 = ((uint64_t)pwmPeriod); + uint64_t newCC64p16 = ((uint64_t)cc) << 16; + uint64_t ratio64p16 = (newCC64p16 / oldCC64p0); + PWMState p; // The working copy since we can't edit the one in use + p = pwmState; + uint32_t ttl = 0; + for (auto i = 0; i < p.cnt; i++) { + uint64_t val64p16 = ((uint64_t)p.edge[i].delta) << 16; + uint64_t newVal64p32 = val64p16 * ratio64p16; + p.edge[i].delta = newVal64p32 >> 32; + ttl += p.edge[i].delta; + } + p.edge[p.cnt].delta = cc - ttl; // Final cleanup exactly cc total cycles + // Update and wait for mailbox to be emptied + pwmUpdate = &p; + while (pwmUpdate) { + delay(0); + } + } pwmPeriod = cc; - // TODO - should we drop all waveforms? } // Helper routine to remove an entry from the state machine diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index 23d4c4e9a7..37f5cc8c8b 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -45,6 +45,8 @@ extern void __analogWriteFreq(uint32_t freq) { } else { analogFreq = freq; } + uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / analogFreq; + _setPWMPeriodCC(analogPeriod); } extern void __analogWrite(uint8_t pin, int val) { @@ -52,7 +54,6 @@ extern void __analogWrite(uint8_t pin, int val) { return; } uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / analogFreq; - _setPWMPeriodCC(analogPeriod); if (val < 0) { val = 0; } else if (val > analogScale) { From 5cd145e85d08f05b2593145984f07298908a64aa Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 19 Apr 2020 14:09:03 -0700 Subject: [PATCH 03/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 42 ++++++++++++++++++------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 1f6871bbdf..a2b4820b6d 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -55,8 +55,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 @@ -67,6 +69,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 +volatile int32_t waveformToChange = -1; +volatile uint32_t waveformNewHigh = 0; +volatile uint32_t waveformNewLow = 0; + static uint32_t (*timer1CB)() = NULL; // Non-speed critical bits @@ -268,17 +274,24 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time return false; } Waveform *wave = &waveform[pin]; - // Adjust to shave off some of the IRQ time, approximately - wave->nextTimeHighCycles = timeHighCycles; - wave->nextTimeLowCycles = timeLowCycles; 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 + } + } 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) { @@ -379,6 +392,10 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { pwmUpdate = nullptr; pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! pwmState.idx = pwmState.cnt; // Cause it to start at t=0 + } else if (waveformToChange >=0) { + waveform[waveformToChange].gotoTimeHighCycles = waveformNewHigh; + waveform[waveformToChange].gotoTimeLowCycles = waveformNewLow; + waveformToChange = -1; } bool done = false; @@ -459,16 +476,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 4ce623dc2b79e9a6e973ea249f4b81246f5db57e Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 19 Apr 2020 14:58:14 -0700 Subject: [PATCH 04/56] Clean up signed/unsigned mismatch, 160MHz operat'n --- cores/esp8266/core_esp8266_waveform.cpp | 11 +++++++---- cores/esp8266/core_esp8266_waveform.h | 4 ++-- cores/esp8266/core_esp8266_wiring_pwm.cpp | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index a2b4820b6d..432a1376d5 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -147,7 +147,10 @@ static volatile PWMState *pwmUpdate = nullptr; // Set by main code, cleared by I static uint32_t pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; // Called when analogWriteFreq() changed to update the PWM total period -void _setPWMPeriodCC(int cc) { +void _setPWMPeriodCC(uint32_t cc) { + if (cc == pwmPeriod) { + return; + } if (pwmState.cnt) { // Adjust any running ones to the best of our abilities by scaling them // Used FP math for speed and code size @@ -219,7 +222,7 @@ bool _stopPWM(int pin) { } // Called by analogWrite(1...99%) to set the PWM duty in clock cycles -bool _setPWM(int pin, int cc) { +bool _setPWM(int pin, uint32_t cc) { PWMState p; // Working copy p = pwmState; // Get rid of any entries for this pin @@ -236,8 +239,8 @@ bool _setPWM(int pin, int cc) { p.cnt = 1; p.mask = 1< 16) { return; } + uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / analogFreq; + _setPWMPeriodCC(analogPeriod); if (val < 0) { val = 0; } else if (val > analogScale) { From 3a6f6c14293ca4ff8df43de12bf3a37292f3b9b8 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 20 Apr 2020 14:38:34 -0700 Subject: [PATCH 05/56] 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. --- cores/esp8266/Tone.cpp | 5 +++++ cores/esp8266/core_esp8266_wiring_digital.cpp | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cores/esp8266/Tone.cpp b/cores/esp8266/Tone.cpp index 064fdad5df..d4df0d779f 100644 --- a/cores/esp8266/Tone.cpp +++ b/cores/esp8266/Tone.cpp @@ -34,6 +34,11 @@ static void _startTone(uint8_t _pin, uint32_t high, uint32_t low, uint32_t durat return; } + // Stop any analogWrites (PWM) because they are a different generator + _stopPWM(_pin); + // If there's another Tone or startWaveform on this pin + // it will be changed on-the-fly (no need to stop it) + pinMode(_pin, OUTPUT); high = std::max(high, (uint32_t)microsecondsToClockCycles(25)); // new 20KHz maximum tone frequency, diff --git a/cores/esp8266/core_esp8266_wiring_digital.cpp b/cores/esp8266/core_esp8266_wiring_digital.cpp index ab6ed5dee9..29d63ace35 100644 --- a/cores/esp8266/core_esp8266_wiring_digital.cpp +++ b/cores/esp8266/core_esp8266_wiring_digital.cpp @@ -82,7 +82,8 @@ extern void __pinMode(uint8_t pin, uint8_t mode) { } extern void ICACHE_RAM_ATTR __digitalWrite(uint8_t pin, uint8_t val) { - stopWaveform(pin); + stopWaveform(pin); // Disable any tone + _stopPWM(pin); // ...and any analogWrite if(pin < 16){ if(val) GPOS = (1 << pin); else GPOC = (1 << pin); From 27ee6f81d146968930da217cac94150891cab442 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 20 Apr 2020 17:33:53 -0700 Subject: [PATCH 06/56] 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. --- 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 432a1376d5..8317e972a4 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -511,8 +511,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { nextEventCycles = min_u32(nextEventCycles, timer1CB()); } - if (nextEventCycles < microsecondsToClockCycles(10)) { - nextEventCycles = microsecondsToClockCycles(10); + if (nextEventCycles < microsecondsToClockCycles(5)) { + nextEventCycles = microsecondsToClockCycles(5); } nextEventCycles -= DELTAIRQ; From 5faf6be202ab97b68fd1f7f7cca9108f67851f9d Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 22 Apr 2020 14:26:50 -0700 Subject: [PATCH 07/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 61 ++++++++++++------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 8317e972a4..a9ba12ec11 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -127,18 +127,13 @@ void setTimer1Callback(uint32_t (*fn)()) { constexpr int maxPWMs = 8; -// PWM edge definition -typedef struct { - unsigned int pin : 8; - unsigned int delta : 24; -} PWMEntry; - // PWM machine state typedef struct { uint32_t mask; // Bitmask of active pins - uint8_t cnt; // How many entries - uint8_t idx; // Where the state machine is along the list - PWMEntry edge[maxPWMs + 1]; // Include space for terminal element + uint8_t cnt; // How many entries + uint8_t idx; // Where the state machine is along the list + uint8_t pin[maxPWMs + 1]; + uint32_t delta[maxPWMs + 1]; uint32_t nextServiceCycle; // Clock cycle for next step } PWMState; @@ -161,12 +156,12 @@ void _setPWMPeriodCC(uint32_t cc) { p = pwmState; uint32_t ttl = 0; for (auto i = 0; i < p.cnt; i++) { - uint64_t val64p16 = ((uint64_t)p.edge[i].delta) << 16; + uint64_t val64p16 = ((uint64_t)p.delta[i]) << 16; uint64_t newVal64p32 = val64p16 * ratio64p16; - p.edge[i].delta = newVal64p32 >> 32; - ttl += p.edge[i].delta; + p.delta[i] = newVal64p32 >> 32; + ttl += p.delta[i]; } - p.edge[p.cnt].delta = cc - ttl; // Final cleanup exactly cc total cycles + p.delta[p.cnt] = cc - ttl; // Final cleanup exactly cc total cycles // Update and wait for mailbox to be emptied pwmUpdate = &p; while (pwmUpdate) { @@ -185,16 +180,17 @@ static void _removePWMEntry(int pin, PWMState *p) { int delta = 0; int i; for (i=0; i < p->cnt; i++) { - if (p->edge[i].pin == pin) { - delta = p->edge[i].delta; + if (p->pin[i] == pin) { + delta = p->delta[i]; break; } } // Add the removed previous pin delta to preserve absolute position - p->edge[i+1].delta += delta; + p->delta[i+1] += delta; // Move everything back one and clean up for (i++; i <= p->cnt; i++) { - p->edge[i-1] = p->edge[i]; + p->pin[i-1] = p->pin[i]; + p->delta[i-1] = p->delta[i]; } p->mask &= ~(1<cnt--; @@ -232,25 +228,26 @@ bool _setPWM(int pin, uint32_t cc) { return false; // No space left } else if (p.cnt == 0) { // Starting up from scratch, special case 1st element and PWM period - p.edge[0].pin = pin; - p.edge[0].delta = cc; - p.edge[1].pin = 0xff; - p.edge[1].delta = pwmPeriod - cc; + p.pin[0] = pin; + p.delta[0] = cc; + p.pin[1] = 0xff; + p.delta[1] = pwmPeriod - cc; p.cnt = 1; p.mask = 1< Date: Fri, 24 Apr 2020 11:07:08 -0700 Subject: [PATCH 08/56] Remove if() that could never evaluate TRUE --- cores/esp8266/core_esp8266_waveform.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index a9ba12ec11..5b9553f8f7 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -405,9 +405,9 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { // PWM state machine implementation if (pwmState.cnt) { - uint32_t now = GetCycleCountIRQ(); + uint32_t now = GetCycleCountIRQ(); int32_t cyclesToGo = pwmState.nextServiceCycle - now; - if (cyclesToGo <= 10) { + if (cyclesToGo < 0) { if (pwmState.idx == pwmState.cnt) { // Start of pulses, possibly copy new if (pwmUpdate) { // Do the memory copy from temp to global and clear mailbox @@ -434,8 +434,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } // Preserve duty cycle over PWM period by using now+xxx instead of += delta pwmState.nextServiceCycle = now + pwmState.delta[pwmState.idx]; - cyclesToGo = pwmState.nextServiceCycle - now; - if (cyclesToGo<0) cyclesToGo = 0; + cyclesToGo = pwmState.nextServiceCycle - now; // Guaranteed to be >= 0 always } nextEventCycles = min_u32(nextEventCycles, cyclesToGo); } From 1182cd0b330e50c2120a832c6c171b57cf2a0f58 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 27 Apr 2020 10:24:32 -0700 Subject: [PATCH 09/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 27 ++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 5b9553f8f7..c9008be158 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -57,8 +57,11 @@ typedef struct { uint32_t expiryCycle; // For time-limited waveform, the cycle when this waveform must stop uint32_t timeHighCycles; // Currently running waveform period uint32_t timeLowCycles; // + uint32_t desiredHighCycles; // Currently running waveform period + uint32_t desiredLowCycles; // uint32_t gotoTimeHighCycles; // Copied over on the next period to preserve phase uint32_t gotoTimeLowCycles; // + uint32_t lastEdge; // } Waveform; static Waveform waveform[17]; // State of all possible pins @@ -290,6 +293,9 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time } else { // if (!(waveformEnabled & mask)) { wave->timeHighCycles = timeHighCycles; wave->timeLowCycles = timeLowCycles; + wave->desiredHighCycles = wave->timeHighCycles; + wave->desiredLowCycles = wave->timeLowCycles; + wave->lastEdge = 0; 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); @@ -475,6 +481,11 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { SetGPIO(mask); } + if (wave->lastEdge) { + int32_t err = wave->desiredLowCycles - (now - wave->lastEdge); + err /= 4; + wave->timeLowCycles += err; + } wave->nextServiceCycle = now + wave->timeHighCycles; nextEventCycles = min_u32(nextEventCycles, wave->timeHighCycles); } else { @@ -483,12 +494,22 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { ClearGPIO(mask); } + if (wave->gotoTimeHighCycles) { + // Copy over next full-cycle timings + wave->timeHighCycles = wave->gotoTimeHighCycles; + wave->desiredHighCycles = wave->gotoTimeHighCycles; + wave->timeLowCycles = wave->gotoTimeLowCycles; + wave->desiredLowCycles = wave->gotoTimeLowCycles; + wave->gotoTimeHighCycles = 0; + } else { + int32_t err = wave->desiredHighCycles - (now - wave->lastEdge); + err /= 4; + wave->timeHighCycles += err; // Feedback 1/4 of the error + } 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; } + wave->lastEdge = now; } else { uint32_t deltaCycles = wave->nextServiceCycle - now; nextEventCycles = min_u32(nextEventCycles, deltaCycles); From 7ee7d194a0699543e8b1d6a04cc48f8cea54ed5b Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 27 Apr 2020 11:28:59 -0700 Subject: [PATCH 10/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index c9008be158..b0c7d8aa9c 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -175,11 +175,7 @@ void _setPWMPeriodCC(uint32_t cc) { } // Helper routine to remove an entry from the state machine -static void _removePWMEntry(int pin, PWMState *p) { - if (!((1<mask)) { - return; - } - +static ICACHE_RAM_ATTR void _removePWMEntry(int pin, PWMState *p) { int delta = 0; int i; for (i=0; i < p->cnt; i++) { @@ -200,7 +196,7 @@ static void _removePWMEntry(int pin, PWMState *p) { } // Called by analogWrite(0/100%) to disable PWM on a specific pin -bool _stopPWM(int pin) { +ICACHE_RAM_ATTR bool _stopPWM(int pin) { if (!((1<= maxPWMs) { return false; // No space left From c12e961d6884b609f2664f941143bd4787089b83 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 27 Apr 2020 12:45:06 -0700 Subject: [PATCH 11/56] Avoid long wait times when PWM freq is low --- cores/esp8266/core_esp8266_waveform.cpp | 46 ++++++++++++++++--------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index b0c7d8aa9c..9df4dee729 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -106,6 +106,12 @@ static void ICACHE_RAM_ATTR deinitTimer() { timerRunning = false; } +static ICACHE_RAM_ATTR void forceTimerInterrupt() { + if (T1L > microsecondsToClockCycles(10)) { + timer1_write(microsecondsToClockCycles(10)); + } +} + // Set a callback. Pass in NULL to stop it void setTimer1Callback(uint32_t (*fn)()) { timer1CB = fn; @@ -141,7 +147,7 @@ typedef struct { } PWMState; static PWMState pwmState; -static volatile PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR +static volatile PWMState * volatile pwmUpdate = nullptr; // Set by main code, cleared by ISR static uint32_t pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; // Called when analogWriteFreq() changed to update the PWM total period @@ -167,6 +173,7 @@ void _setPWMPeriodCC(uint32_t cc) { p.delta[p.cnt] = cc - ttl; // Final cleanup exactly cc total cycles // Update and wait for mailbox to be emptied pwmUpdate = &p; + forceTimerInterrupt(); while (pwmUpdate) { delay(0); } @@ -176,21 +183,21 @@ void _setPWMPeriodCC(uint32_t cc) { // Helper routine to remove an entry from the state machine static ICACHE_RAM_ATTR void _removePWMEntry(int pin, PWMState *p) { - int delta = 0; int i; - for (i=0; i < p->cnt; i++) { - if (p->pin[i] == pin) { - delta = p->delta[i]; - break; - } - } + + // Find the pin to pull out... + for (i = 0; p->pin[i] != pin; i++) { /* no-op */ } + auto delta = p->delta[i]; + // Add the removed previous pin delta to preserve absolute position p->delta[i+1] += delta; - // Move everything back one and clean up + + // Move everything back one for (i++; i <= p->cnt; i++) { p->pin[i-1] = p->pin[i]; p->delta[i-1] = p->delta[i]; } + // Remove the pin from the active list p->mask &= ~(1<cnt--; } @@ -200,16 +207,19 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { if (!((1< microsecondsToClockCycles(10)) { - timer1_write(microsecondsToClockCycles(10)); - } + forceTimerInterrupt(); } while (waveformToEnable) { delay(0); // Wait for waveform to update @@ -406,7 +418,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { if (waveformEnabled || pwmState.cnt) { do { nextEventCycles = microsecondsToClockCycles(MAXIRQUS); - + // PWM state machine implementation if (pwmState.cnt) { uint32_t now = GetCycleCountIRQ(); From a78362106e914fcdfc2b7f1bb0555d32e892ae0d Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 27 Apr 2020 17:09:07 -0700 Subject: [PATCH 12/56] Fix bug where tone/pwm could happen on same pin --- cores/esp8266/core_esp8266_waveform.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index dbdf277e1f..4c277c6a50 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -228,6 +228,7 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { // Called by analogWrite(1...99%) to set the PWM duty in clock cycles bool _setPWM(int pin, uint32_t cc) { + stopWaveform(pin); PWMState p; // Working copy p = pwmState; // Get rid of any entries for this pin @@ -295,6 +296,8 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it } + _stopPWM(pin); // Make sure there's no PWM live here + uint32_t mask = 1< Date: Tue, 28 Apr 2020 10:41:20 -0700 Subject: [PATCH 13/56] 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). --- cores/esp8266/core_esp8266_waveform.cpp | 79 ++++++++++++++++++++----- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 4c277c6a50..421149be42 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -78,6 +78,9 @@ volatile uint32_t waveformNewLow = 0; static uint32_t (*timer1CB)() = NULL; +uint32_t at160 = 0; +uint32_t at80 = 0; + // Non-speed critical bits #pragma GCC optimize ("Os") @@ -382,6 +385,21 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { #define DELTAIRQ (microsecondsToClockCycles(2)) #endif +static inline ICACHE_RAM_ATTR uint32_t turboAdjust(uint32_t cycles, bool turbo) { +#if F_CPU == 80000000 + if (turbo) { + return cycles * 2; + } else { + return cycles; + } +#else + if (turbo) { + return cycles; + } else { + return cycles / 2; + } +#endif +} static ICACHE_RAM_ATTR void timer1Interrupt() { // Optimize the NMI inner loop by keeping track of the min and max GPIO that we @@ -390,6 +408,11 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { static int startPin = 0; static int endPin = 0; + bool turbo = (*(uint32_t*)0x3FF00014) & 1 ? true : false; + if (turbo) at160++; + else at80++; + //if (turbo) GPOS = 1<<14; else GPOC = 1<<14; + uint32_t nextEventCycles = microsecondsToClockCycles(MAXIRQUS); uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); @@ -450,7 +473,11 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } while (pwmState.delta[pwmState.idx] == 0); } // Preserve duty cycle over PWM period by using now+xxx instead of += delta - pwmState.nextServiceCycle = now + pwmState.delta[pwmState.idx]; +#if F_CPU == 80000000 + pwmState.nextServiceCycle = now + (turbo ? 2 : 1) * pwmState.delta[pwmState.idx]; +#else + pwmState.nextServiceCycle = now + pwmState.delta[pwmState.idx] / (turbo ? 1 : 2); +#endif cyclesToGo = pwmState.nextServiceCycle - now; // Guaranteed to be >= 0 always } nextEventCycles = min_u32(nextEventCycles, cyclesToGo); @@ -493,12 +520,24 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { SetGPIO(mask); } if (wave->lastEdge) { - int32_t err = wave->desiredLowCycles - (now - wave->lastEdge); - err /= 4; - wave->timeLowCycles += err; +#if F_CPU == 80000000 + auto desiredLowCycles = wave->desiredLowCycles << (turbo ? 1 : 0); +#else + auto desiredLowCycles = wave->desiredLowCycles >> (turbo ? 0 : 1); +#endif + int32_t err = desiredLowCycles - (now - wave->lastEdge); + if (abs(err) < desiredLowCycles) { // If we've lost > the entire phase, ignore this error signal + err /= 4; + wave->timeLowCycles += err; + } } - wave->nextServiceCycle = now + wave->timeHighCycles; - nextEventCycles = min_u32(nextEventCycles, wave->timeHighCycles); +#if F_CPU == 80000000 + auto timeHighCycles = wave->timeHighCycles << (turbo ? 1 : 0); +#else + auto timeHighCycles = wave->timeHighCycles >> (turbo ? 0 : 1); +#endif + wave->nextServiceCycle = now + timeHighCycles; + nextEventCycles = min_u32(nextEventCycles, timeHighCycles); } else { if (i == 16) { GP16O &= ~1; // GPIO16 write slow as it's RMW @@ -513,12 +552,24 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wave->desiredLowCycles = wave->gotoTimeLowCycles; wave->gotoTimeHighCycles = 0; } else { - int32_t err = wave->desiredHighCycles - (now - wave->lastEdge); - err /= 4; - wave->timeHighCycles += err; // Feedback 1/4 of the error +#if F_CPU == 80000000 + auto desiredHighCycles = wave->desiredHighCycles << (turbo ? 1 : 0); +#else + auto desiredHighCycles = wave->desiredHighCycles >> (turbo ? 0 : 1); +#endif + int32_t err = desiredHighCycles - (now - wave->lastEdge); + if (abs(err) < desiredHighCycles) { // If we've lost > the entire phase, ignore this error signal + err /= 4; + wave->timeHighCycles += err; // Feedback 1/4 of the error + } } - wave->nextServiceCycle = now + wave->timeLowCycles; - nextEventCycles = min_u32(nextEventCycles, wave->timeLowCycles); +#if F_CPU == 80000000 + auto timeLowCycles = wave->timeLowCycles << (turbo ? 1 : 0); +#else + auto timeLowCycles = wave->timeLowCycles >> (turbo ? 0 : 1); +#endif + wave->nextServiceCycle = now + timeLowCycles; + nextEventCycles = min_u32(nextEventCycles, timeLowCycles); } wave->lastEdge = now; } else { @@ -545,11 +596,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { nextEventCycles -= DELTAIRQ; // Do it here instead of global function to save time and because we know it's edge-IRQ -#if F_CPU == 160000000 - T1L = nextEventCycles >> 1; // Already know we're in range by MAXIRQUS -#else - T1L = nextEventCycles; // Already know we're in range by MAXIRQUS -#endif + T1L = nextEventCycles >> (turbo ? 1 : 0); TEIE |= TEIE1; // Edge int enable } From dc3296125fb0a315f91ec80ed6133932a4ace7db Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 28 Apr 2020 12:16:01 -0700 Subject: [PATCH 14/56] Clean up leftover debugs in ISR --- cores/esp8266/core_esp8266_waveform.cpp | 72 ++++++++----------------- 1 file changed, 21 insertions(+), 51 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 421149be42..2ea82c4d0f 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -5,23 +5,23 @@ Copyright (c) 2018 Earle F. Philhower, III. All rights reserved. The core idea is to have a programmable waveform generator with a unique - high and low period (defined in microseconds or CPU clock cycles). TIMER1 is - set to 1-shot mode and is always loaded with the time until the next edge - of any live waveforms. + high and low period (defined in microseconds or CPU clock cycles). TIMER1 + is set to 1-shot mode and is always loaded with the time until the next + edge of any live waveforms. Up to one waveform generator per pin supported. - Each waveform generator is synchronized to the ESP clock cycle counter, not the - timer. This allows for removing interrupt jitter and delay as the counter - always increments once per 80MHz clock. Changes to a waveform are + Each waveform generator is synchronized to the ESP clock cycle counter, not + the timer. This allows for removing interrupt jitter and delay as the + counter always increments once per 80MHz clock. Changes to a waveform are contiguous and only take effect on the next waveform transition, allowing for smooth transitions. This replaces older tone(), analogWrite(), and the Servo classes. Everywhere in the code where "cycles" is used, it means ESP.getCycleCount() - clock cycle count, or an interval measured in CPU clock cycles, but not TIMER1 - cycles (which may be 2 CPU clock cycles @ 160MHz). + clock cycle count, or an interval measured in CPU clock cycles, but not + TIMER1 cycles (which may be 2 CPU clock cycles @ 160MHz). This library is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public @@ -379,27 +379,19 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { // The SDK and hardware take some time to actually get to our NMI code, so // decrement the next IRQ's timer value by a bit so we can actually catch the // real CPU cycle counter we want for the waveforms. + +// The SDK also sometimes is running at a different speed the the Arduino core +// so the ESP cycle counter is actually running at a variable speed. +// adjust(x) takes care of adjusting a delta clock cycle amount accordingly. #if F_CPU == 80000000 #define DELTAIRQ (microsecondsToClockCycles(3)) + #define adjust(x) ((x) << (turbo ? 1 : 0)) #else #define DELTAIRQ (microsecondsToClockCycles(2)) + #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif -static inline ICACHE_RAM_ATTR uint32_t turboAdjust(uint32_t cycles, bool turbo) { -#if F_CPU == 80000000 - if (turbo) { - return cycles * 2; - } else { - return cycles; - } -#else - if (turbo) { - return cycles; - } else { - return cycles / 2; - } -#endif -} + static ICACHE_RAM_ATTR void timer1Interrupt() { // Optimize the NMI inner loop by keeping track of the min and max GPIO that we @@ -408,10 +400,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { static int startPin = 0; static int endPin = 0; + // Flag if the core is at 160 MHz, for use by adjust() bool turbo = (*(uint32_t*)0x3FF00014) & 1 ? true : false; - if (turbo) at160++; - else at80++; - //if (turbo) GPOS = 1<<14; else GPOC = 1<<14; uint32_t nextEventCycles = microsecondsToClockCycles(MAXIRQUS); uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); @@ -473,11 +463,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } while (pwmState.delta[pwmState.idx] == 0); } // Preserve duty cycle over PWM period by using now+xxx instead of += delta -#if F_CPU == 80000000 - pwmState.nextServiceCycle = now + (turbo ? 2 : 1) * pwmState.delta[pwmState.idx]; -#else - pwmState.nextServiceCycle = now + pwmState.delta[pwmState.idx] / (turbo ? 1 : 2); -#endif + pwmState.nextServiceCycle = now + adjust(pwmState.delta[pwmState.idx]); cyclesToGo = pwmState.nextServiceCycle - now; // Guaranteed to be >= 0 always } nextEventCycles = min_u32(nextEventCycles, cyclesToGo); @@ -520,22 +506,14 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { SetGPIO(mask); } if (wave->lastEdge) { -#if F_CPU == 80000000 - auto desiredLowCycles = wave->desiredLowCycles << (turbo ? 1 : 0); -#else - auto desiredLowCycles = wave->desiredLowCycles >> (turbo ? 0 : 1); -#endif + auto desiredLowCycles = adjust(wave->desiredLowCycles); int32_t err = desiredLowCycles - (now - wave->lastEdge); if (abs(err) < desiredLowCycles) { // If we've lost > the entire phase, ignore this error signal err /= 4; wave->timeLowCycles += err; } } -#if F_CPU == 80000000 - auto timeHighCycles = wave->timeHighCycles << (turbo ? 1 : 0); -#else - auto timeHighCycles = wave->timeHighCycles >> (turbo ? 0 : 1); -#endif + auto timeHighCycles = adjust(wave->timeHighCycles); wave->nextServiceCycle = now + timeHighCycles; nextEventCycles = min_u32(nextEventCycles, timeHighCycles); } else { @@ -552,22 +530,14 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wave->desiredLowCycles = wave->gotoTimeLowCycles; wave->gotoTimeHighCycles = 0; } else { -#if F_CPU == 80000000 - auto desiredHighCycles = wave->desiredHighCycles << (turbo ? 1 : 0); -#else - auto desiredHighCycles = wave->desiredHighCycles >> (turbo ? 0 : 1); -#endif + auto desiredHighCycles = adjust(wave->desiredHighCycles); int32_t err = desiredHighCycles - (now - wave->lastEdge); if (abs(err) < desiredHighCycles) { // If we've lost > the entire phase, ignore this error signal err /= 4; wave->timeHighCycles += err; // Feedback 1/4 of the error } } -#if F_CPU == 80000000 - auto timeLowCycles = wave->timeLowCycles << (turbo ? 1 : 0); -#else - auto timeLowCycles = wave->timeLowCycles >> (turbo ? 0 : 1); -#endif + auto timeLowCycles = adjust(wave->timeLowCycles); wave->nextServiceCycle = now + timeLowCycles; nextEventCycles = min_u32(nextEventCycles, timeLowCycles); } From ec62ee3d9b89da2bef7634861cd69c5b07fce287 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 28 Apr 2020 12:48:26 -0700 Subject: [PATCH 15/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 4 ++++ cores/esp8266/core_esp8266_wiring_pwm.cpp | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 2ea82c4d0f..d01da95713 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -155,6 +155,10 @@ static uint32_t pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(uint32_t cc) { + // Simple constant shift in period to overcome fixed ISR start time + if (cc > microsecondsToClockCycles(3)) { + cc -= microsecondsToClockCycles(3) >> 1; // Go back 1.5us + } if (cc == pwmPeriod) { return; } diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index 6ec8a82d18..fd297310d1 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -40,8 +40,8 @@ extern void __analogWriteRange(uint32_t range) { extern void __analogWriteFreq(uint32_t freq) { if (freq < 100) { analogFreq = 100; - } else if (freq > 40000) { - analogFreq = 40000; + } else if (freq > 60000) { + analogFreq = 60000; } else { analogFreq = freq; } From a41890b97c7f72bb65dd1f93815ea9ea95d9c2b6 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 28 Apr 2020 16:39:52 -0700 Subject: [PATCH 16/56] Fix GPIO16 not toggling properly. --- cores/esp8266/core_esp8266_waveform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index d01da95713..b19bff8a8b 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -450,7 +450,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } GPOS = pwmState.mask; // Set all active pins high // GPIO16 isn't the same as the others - if (pwmState.mask & 0x100) { + if (pwmState.mask & (1<<16)) { GP16O |= 1; } pwmState.idx = 0; From 42bbede63650d4c98f94754c939088acde34ce2c Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 28 Apr 2020 18:57:29 -0700 Subject: [PATCH 17/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 36 +++++++++++-------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index b19bff8a8b..dba12fe9e1 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -78,9 +78,6 @@ volatile uint32_t waveformNewLow = 0; static uint32_t (*timer1CB)() = NULL; -uint32_t at160 = 0; -uint32_t at80 = 0; - // Non-speed critical bits #pragma GCC optimize ("Os") @@ -115,16 +112,6 @@ static ICACHE_RAM_ATTR void forceTimerInterrupt() { } } -// Set a callback. Pass in NULL to stop it -void setTimer1Callback(uint32_t (*fn)()) { - timer1CB = fn; - if (!timerRunning && fn) { - initTimer(); - timer1_write(microsecondsToClockCycles(1)); // Cause an interrupt post-haste - } else if (timerRunning && !fn && !waveformEnabled) { - deinitTimer(); - } -} // PWM implementation using special purpose state machine // @@ -151,14 +138,10 @@ typedef struct { static PWMState pwmState; static volatile PWMState * volatile pwmUpdate = nullptr; // Set by main code, cleared by ISR -static uint32_t pwmPeriod = (1000000L * system_get_cpu_freq()) / 1000; +static uint32_t pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(uint32_t cc) { - // Simple constant shift in period to overcome fixed ISR start time - if (cc > microsecondsToClockCycles(3)) { - cc -= microsecondsToClockCycles(3) >> 1; // Go back 1.5us - } if (cc == pwmPeriod) { return; } @@ -270,6 +253,7 @@ bool _setPWM(int pin, uint32_t cc) { p.cnt++; p.mask |= 1<high transition. For immediate change, stopWaveform() // first, then it will immediately begin. @@ -337,6 +320,19 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time return true; } + +// Set a callback. Pass in NULL to stop it +void setTimer1Callback(uint32_t (*fn)()) { + timer1CB = fn; + if (!timerRunning && fn) { + initTimer(); + timer1_write(microsecondsToClockCycles(1)); // Cause an interrupt post-haste + } else if (timerRunning && !fn && !waveformEnabled && !pwmState.cnt) { + deinitTimer(); + } +} + + // Speed critical bits #pragma GCC optimize ("O2") // Normally would not want two copies like this, but due to different @@ -395,8 +391,6 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif - - 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 From 79a7f7d78537755e27414a6a5ad96743ac29de8d Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 03:14:59 -0700 Subject: [PATCH 18/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 43 +++++++++++++++++-------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index dba12fe9e1..9ce4209fba 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -65,19 +65,22 @@ 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 +static uint32_t waveformState = 0; // Is the pin high or low, updated in NMI so no access outside the NMI code +static 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 uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin +static uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation -volatile int32_t waveformToChange = -1; -volatile uint32_t waveformNewHigh = 0; -volatile uint32_t waveformNewLow = 0; +int32_t waveformToChange = -1; +uint32_t waveformNewHigh = 0; +uint32_t waveformNewLow = 0; static uint32_t (*timer1CB)() = NULL; +// Ensure everything is read/written to RAM +#define MEMBARRIER() { __asm__ volatile("" ::: "memory"); } + // Non-speed critical bits #pragma GCC optimize ("Os") @@ -137,7 +140,7 @@ typedef struct { } PWMState; static PWMState pwmState; -static volatile PWMState * volatile pwmUpdate = nullptr; // Set by main code, cleared by ISR +static PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR static uint32_t pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; // Called when analogWriteFreq() changed to update the PWM total period @@ -163,9 +166,11 @@ void _setPWMPeriodCC(uint32_t cc) { p.delta[p.cnt] = cc - ttl; // Final cleanup exactly cc total cycles // Update and wait for mailbox to be emptied pwmUpdate = &p; + MEMBARRIER(); forceTimerInterrupt(); while (pwmUpdate) { delay(0); + // No mem barrier. The external function call guarantees it's re-read } } pwmPeriod = cc; @@ -204,11 +209,13 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { // Update and wait for mailbox to be emptied pwmUpdate = &p; + MEMBARRIER(); forceTimerInterrupt(); while (pwmUpdate) { + MEMBARRIER(); /* Busy wait, could be in ISR */ } - + MEMBARRIER(); // Possibly shut down the timer completely if we're done if (!waveformEnabled && !pwmState.cnt && !timer1CB) { deinitTimer(); @@ -256,6 +263,7 @@ bool _setPWM(int pin, uint32_t cc) { // Set mailbox and wait for ISR to copy it over pwmUpdate = &p; + MEMBARRIER(); if (!timerRunning) { initTimer(); timer1_write(microsecondsToClockCycles(10)); @@ -289,12 +297,15 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time _stopPWM(pin); // Make sure there's no PWM live here uint32_t mask = 1<= 0) { delay(0); // Wait for waveform to update + // No mem barrier here, the call to a global function implies global state updated } } else { // if (!(waveformEnabled & mask)) { wave->timeHighCycles = timeHighCycles; @@ -306,6 +317,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time 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; + MEMBARRIER(); if (!timerRunning) { initTimer(); timer1_write(microsecondsToClockCycles(10)); @@ -314,6 +326,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time } while (waveformToEnable) { delay(0); // Wait for waveform to update + // No mem barrier here, the call to a global function implies global state updated } } @@ -362,14 +375,13 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { // If they send >=32, then the shift will result in 0 and it will also return false if (waveformEnabled & (1UL << pin)) { waveformToDisable = 1UL << pin; - // Must not interfere if Timer is due shortly - if (T1L > microsecondsToClockCycles(10)) { - timer1_write(microsecondsToClockCycles(10)); - } + forceTimerInterrupt(); while (waveformToDisable) { + MEMBARRIER(); // If it wasn't written yet, it has to be by now /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ } } + MEMBARRIER(); if (!waveformEnabled && !pwmState.cnt && !timer1CB) { deinitTimer(); } @@ -410,6 +422,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { waveformState &= ~waveformToEnable; // And clear the state of any just started waveformToEnable = 0; waveformToDisable = 0; + // No mem barrier. Globals must be written to RAM on ISR exit. // 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; // Find the last bit by subtracting off GCC's count-leading-zeros (no offset in this one) @@ -420,10 +433,12 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { pwmUpdate = nullptr; pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! pwmState.idx = pwmState.cnt; // Cause it to start at t=0 - } else if (waveformToChange >=0) { + // No need for mem barrier here. Global must be written by IRQ exit + } else if (waveformToChange >= 0) { waveform[waveformToChange].gotoTimeHighCycles = waveformNewHigh; waveform[waveformToChange].gotoTimeLowCycles = waveformNewLow; waveformToChange = -1; + // No need for memory barrier here. The global has to be written before exit the ISR. } bool done = false; From cbff7a36e87085e03d58655debd31062eab2c2b0 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 03:27:05 -0700 Subject: [PATCH 19/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 111 +++++++++++++----------- 1 file changed, 58 insertions(+), 53 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 9ce4209fba..e9ecdee856 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -64,19 +64,30 @@ typedef struct { uint32_t lastEdge; // } Waveform; -static Waveform waveform[17]; // State of all possible pins -static uint32_t waveformState = 0; // Is the pin high or low, updated in NMI so no access outside the NMI code -static uint32_t waveformEnabled = 0; // Is it actively running, updated in NMI so no access outside the NMI code +class WVFState { +public: + Waveform waveform[17]; // State of all possible pins + uint32_t waveformState = 0; // Is the pin high or low, updated in NMI so no access outside the NMI code + 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 uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin -static uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation + // Enable lock-free by only allowing updates to waveformState and waveformEnabled from IRQ service routine + uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin + uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation -int32_t waveformToChange = -1; -uint32_t waveformNewHigh = 0; -uint32_t waveformNewLow = 0; + int32_t waveformToChange = -1; + uint32_t waveformNewHigh = 0; + 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; +}; +static WVFState wvfState; -static uint32_t (*timer1CB)() = NULL; // Ensure everything is read/written to RAM #define MEMBARRIER() { __asm__ volatile("" ::: "memory"); } @@ -217,7 +228,7 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { } MEMBARRIER(); // Possibly shut down the timer completely if we're done - if (!waveformEnabled && !pwmState.cnt && !timer1CB) { + if (!wvfState.waveformEnabled && !pwmState.cnt && !wvfState.timer1CB) { deinitTimer(); } return true; @@ -288,7 +299,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time if ((pin > 16) || isFlashInterfacePin(pin)) { return false; } - Waveform *wave = &waveform[pin]; + Waveform *wave = &wvfState.waveform[pin]; wave->expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; if (runTimeCycles && !wave->expiryCycle) { wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it @@ -298,16 +309,16 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time uint32_t mask = 1<= 0) { + wvfState.waveformToChange = pin; + while (wvfState.waveformToChange >= 0) { delay(0); // Wait for waveform to update // No mem barrier here, the call to a global function implies global state updated } - } else { // if (!(waveformEnabled & mask)) { + } else { // if (!(wvfState.waveformEnabled & mask)) { wave->timeHighCycles = timeHighCycles; wave->timeLowCycles = timeLowCycles; wave->desiredHighCycles = wave->timeHighCycles; @@ -316,7 +327,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time 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; + wvfState.waveformToEnable |= mask; MEMBARRIER(); if (!timerRunning) { initTimer(); @@ -324,7 +335,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time } else { forceTimerInterrupt(); } - while (waveformToEnable) { + while (wvfState.waveformToEnable) { delay(0); // Wait for waveform to update // No mem barrier here, the call to a global function implies global state updated } @@ -336,11 +347,11 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time // Set a callback. Pass in NULL to stop it void setTimer1Callback(uint32_t (*fn)()) { - timer1CB = fn; + wvfState.timer1CB = fn; if (!timerRunning && fn) { initTimer(); timer1_write(microsecondsToClockCycles(1)); // Cause an interrupt post-haste - } else if (timerRunning && !fn && !waveformEnabled && !pwmState.cnt) { + } else if (timerRunning && !fn && !wvfState.waveformEnabled && !pwmState.cnt) { deinitTimer(); } } @@ -373,16 +384,16 @@ 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 - if (waveformEnabled & (1UL << pin)) { - waveformToDisable = 1UL << pin; + if (wvfState.waveformEnabled & (1UL << pin)) { + wvfState.waveformToDisable = 1UL << pin; forceTimerInterrupt(); - while (waveformToDisable) { + while (wvfState.waveformToDisable) { MEMBARRIER(); // If it wasn't written yet, it has to be by now /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ } } MEMBARRIER(); - if (!waveformEnabled && !pwmState.cnt && !timer1CB) { + if (!wvfState.waveformEnabled && !pwmState.cnt && !wvfState.timer1CB) { deinitTimer(); } return true; @@ -404,29 +415,23 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { #endif 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; - // Flag if the core is at 160 MHz, for use by adjust() bool turbo = (*(uint32_t*)0x3FF00014) & 1 ? true : false; uint32_t nextEventCycles = microsecondsToClockCycles(MAXIRQUS); uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); - if (waveformToEnable || waveformToDisable) { + if (wvfState.waveformToEnable || wvfState.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; + wvfState.waveformEnabled = (wvfState.waveformEnabled & ~wvfState.waveformToDisable) | wvfState.waveformToEnable; // Set the requested waveforms on/off + wvfState.waveformState &= ~wvfState.waveformToEnable; // And clear the state of any just started + wvfState.waveformToEnable = 0; + wvfState.waveformToDisable = 0; // No mem barrier. Globals must be written to RAM on ISR exit. // 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; + wvfState.startPin = __builtin_ffs(wvfState.waveformEnabled) - 1; // Find the last bit by subtracting off GCC's count-leading-zeros (no offset in this one) - endPin = 32 - __builtin_clz(waveformEnabled); + wvfState.endPin = 32 - __builtin_clz(wvfState.waveformEnabled); } else if (!pwmState.cnt && pwmUpdate) { // Start up the PWM generator by copying from the mailbox pwmState = *(PWMState*)pwmUpdate; @@ -434,15 +439,15 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! pwmState.idx = pwmState.cnt; // Cause it to start at t=0 // No need for mem barrier here. Global must be written by IRQ exit - } else if (waveformToChange >= 0) { - waveform[waveformToChange].gotoTimeHighCycles = waveformNewHigh; - waveform[waveformToChange].gotoTimeLowCycles = waveformNewLow; - waveformToChange = -1; + } else if (wvfState.waveformToChange >= 0) { + wvfState.waveform[wvfState.waveformToChange].gotoTimeHighCycles = wvfState.waveformNewHigh; + wvfState.waveform[wvfState.waveformToChange].gotoTimeLowCycles = wvfState.waveformNewLow; + wvfState.waveformToChange = -1; // No need for memory barrier here. The global has to be written before exit the ISR. } bool done = false; - if (waveformEnabled || pwmState.cnt) { + if (wvfState.waveformEnabled || pwmState.cnt) { do { nextEventCycles = microsecondsToClockCycles(MAXIRQUS); @@ -482,15 +487,15 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { nextEventCycles = min_u32(nextEventCycles, cyclesToGo); } - for (int i = startPin; i <= endPin; i++) { + for (int i = wvfState.startPin; i <= wvfState.endPin; i++) { uint32_t mask = 1<expiryCycle - now; if (expiryToGo < 0) { // Done, remove! - waveformEnabled &= ~mask; + wvfState.waveformEnabled &= ~mask; if (i == 16) { GP16O &= ~1; } else { @@ -511,8 +516,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { // Check for toggles int32_t cyclesToGo = wave->nextServiceCycle - now; if (cyclesToGo < 0) { - waveformState ^= mask; - if (waveformState & mask) { + wvfState.waveformState ^= mask; + if (wvfState.waveformState & mask) { if (i == 16) { GP16O |= 1; // GPIO16 write slow as it's RMW } else { @@ -567,10 +572,10 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { int32_t cyclesLeftTimeout = timeoutCycle - now; done = (cycleDeltaNextEvent < 0) || (cyclesLeftTimeout < 0); } while (!done); - } // if (waveformEnabled) + } // if (wvfState.waveformEnabled) - if (timer1CB) { - nextEventCycles = min_u32(nextEventCycles, timer1CB()); + if (wvfState.timer1CB) { + nextEventCycles = min_u32(nextEventCycles, wvfState.timer1CB()); } if (nextEventCycles < microsecondsToClockCycles(5)) { From 4196bf8cb79c75cabe9ec9686edf36b3524aeab5 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 03:37:20 -0700 Subject: [PATCH 20/56] Factor out common timer shutdown code --- cores/esp8266/core_esp8266_waveform.cpp | 34 ++++++++++++------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index e9ecdee856..fc50d47717 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -113,20 +113,12 @@ static void initTimer() { timerRunning = true; } -static void ICACHE_RAM_ATTR deinitTimer() { - ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); - timer1_disable(); - timer1_isr_init(); - timerRunning = false; -} - static ICACHE_RAM_ATTR void forceTimerInterrupt() { if (T1L > microsecondsToClockCycles(10)) { timer1_write(microsecondsToClockCycles(10)); } } - // PWM implementation using special purpose state machine // // Keep an ordered list of pins with the delta in cycles between each @@ -154,6 +146,18 @@ static PWMState pwmState; static PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR static uint32_t pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; + + +static ICACHE_RAM_ATTR void disableIdleTimer() { + if (timerRunning && !wvfState.waveformEnabled && !pwmState.cnt && !wvfState.timer1CB) { + ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); + timer1_disable(); + timer1_isr_init(); + timerRunning = false; + } +} + + // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(uint32_t cc) { if (cc == pwmPeriod) { @@ -226,11 +230,8 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { MEMBARRIER(); /* Busy wait, could be in ISR */ } - MEMBARRIER(); // Possibly shut down the timer completely if we're done - if (!wvfState.waveformEnabled && !pwmState.cnt && !wvfState.timer1CB) { - deinitTimer(); - } + disableIdleTimer(); return true; } @@ -351,8 +352,8 @@ void setTimer1Callback(uint32_t (*fn)()) { if (!timerRunning && fn) { initTimer(); timer1_write(microsecondsToClockCycles(1)); // Cause an interrupt post-haste - } else if (timerRunning && !fn && !wvfState.waveformEnabled && !pwmState.cnt) { - deinitTimer(); + } else { + disableIdleTimer(); } } @@ -392,10 +393,7 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { /* no-op */ // Can't delay() since stopWaveform may be called from an IRQ } } - MEMBARRIER(); - if (!wvfState.waveformEnabled && !pwmState.cnt && !wvfState.timer1CB) { - deinitTimer(); - } + disableIdleTimer(); return true; } From 14f44167563dff4208f3fdad648340a94d735dac Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 09:26:18 -0700 Subject: [PATCH 21/56] Remove unneeded extra copy on PWM start --- cores/esp8266/core_esp8266_waveform.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index fc50d47717..4aed1cde35 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -432,10 +432,9 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wvfState.endPin = 32 - __builtin_clz(wvfState.waveformEnabled); } else if (!pwmState.cnt && pwmUpdate) { // Start up the PWM generator by copying from the mailbox - pwmState = *(PWMState*)pwmUpdate; - pwmUpdate = nullptr; + pwmState.cnt = 1; + pwmState.idx = 1; // Ensure copy this cycle, cause it to start at t=0 pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! - pwmState.idx = pwmState.cnt; // Cause it to start at t=0 // No need for mem barrier here. Global must be written by IRQ exit } else if (wvfState.waveformToChange >= 0) { wvfState.waveform[wvfState.waveformToChange].gotoTimeHighCycles = wvfState.waveformNewHigh; From 2002a5ddfe78894f6c3c8f35a0c0e3b588351e9a Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 10:22:28 -0700 Subject: [PATCH 22/56] Factor out common edge work in waveform loop --- cores/esp8266/core_esp8266_waveform.cpp | 28 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 4aed1cde35..a48806fb9f 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -412,6 +412,16 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif +#define ENABLE_ADJUST // Adjust takes 36 bytes +#ifndef ENABLE_ADJUST + #undef adjust + #define adjust(x) (x) +#endif + +#define ENABLE_FEEDBACK // Feedback costs 68 bytes + +#define ENABLE_PWM // PWM takes 160 bytes + static ICACHE_RAM_ATTR void timer1Interrupt() { // Flag if the core is at 160 MHz, for use by adjust() bool turbo = (*(uint32_t*)0x3FF00014) & 1 ? true : false; @@ -448,6 +458,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { do { nextEventCycles = microsecondsToClockCycles(MAXIRQUS); +#ifdef ENABLE_PWM // PWM state machine implementation if (pwmState.cnt) { uint32_t now = GetCycleCountIRQ(); @@ -483,6 +494,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } nextEventCycles = min_u32(nextEventCycles, cyclesToGo); } +#endif for (int i = wvfState.startPin; i <= wvfState.endPin; i++) { uint32_t mask = 1<nextServiceCycle - now; if (cyclesToGo < 0) { + uint32_t nextEdgeCycles; wvfState.waveformState ^= mask; if (wvfState.waveformState & mask) { if (i == 16) { @@ -520,6 +533,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { SetGPIO(mask); } +#ifdef ENABLE_FEEDBACK if (wave->lastEdge) { auto desiredLowCycles = adjust(wave->desiredLowCycles); int32_t err = desiredLowCycles - (now - wave->lastEdge); @@ -528,9 +542,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wave->timeLowCycles += err; } } - auto timeHighCycles = adjust(wave->timeHighCycles); - wave->nextServiceCycle = now + timeHighCycles; - nextEventCycles = min_u32(nextEventCycles, timeHighCycles); +#endif + nextEdgeCycles = wave->timeHighCycles; } else { if (i == 16) { GP16O &= ~1; // GPIO16 write slow as it's RMW @@ -545,17 +558,20 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wave->desiredLowCycles = wave->gotoTimeLowCycles; wave->gotoTimeHighCycles = 0; } else { +#ifdef ENABLE_FEEDBACK auto desiredHighCycles = adjust(wave->desiredHighCycles); int32_t err = desiredHighCycles - (now - wave->lastEdge); if (abs(err) < desiredHighCycles) { // If we've lost > the entire phase, ignore this error signal err /= 4; wave->timeHighCycles += err; // Feedback 1/4 of the error } +#endif } - auto timeLowCycles = adjust(wave->timeLowCycles); - wave->nextServiceCycle = now + timeLowCycles; - nextEventCycles = min_u32(nextEventCycles, timeLowCycles); + nextEdgeCycles = wave->timeLowCycles; } + nextEdgeCycles = adjust(nextEdgeCycles); + wave->nextServiceCycle = now + nextEdgeCycles; + nextEventCycles = min_u32(nextEventCycles, nextEdgeCycles); wave->lastEdge = now; } else { uint32_t deltaCycles = wave->nextServiceCycle - now; From 5b1f288d4406acf5ae132dfbc6e4b1391141ae7d Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 11:30:00 -0700 Subject: [PATCH 23/56] Factor out waveform phase feedback loop math --- cores/esp8266/core_esp8266_waveform.cpp | 38 ++++++++++++++----------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index a48806fb9f..7d5b7d8622 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -412,15 +412,15 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif -#define ENABLE_ADJUST // Adjust takes 36 bytes +#define ENABLE_ADJUST // Adjust takes 36 bytes +#define ENABLE_FEEDBACK // Feedback costs 68 bytes +#define ENABLE_PWM // PWM takes 160 bytes + #ifndef ENABLE_ADJUST #undef adjust #define adjust(x) (x) #endif -#define ENABLE_FEEDBACK // Feedback costs 68 bytes - -#define ENABLE_PWM // PWM takes 160 bytes static ICACHE_RAM_ATTR void timer1Interrupt() { // Flag if the core is at 160 MHz, for use by adjust() @@ -440,12 +440,14 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wvfState.startPin = __builtin_ffs(wvfState.waveformEnabled) - 1; // Find the last bit by subtracting off GCC's count-leading-zeros (no offset in this one) wvfState.endPin = 32 - __builtin_clz(wvfState.waveformEnabled); +#ifdef ENABLE_PWM } else if (!pwmState.cnt && pwmUpdate) { // Start up the PWM generator by copying from the mailbox pwmState.cnt = 1; pwmState.idx = 1; // Ensure copy this cycle, cause it to start at t=0 pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! // No need for mem barrier here. Global must be written by IRQ exit +#endif } else if (wvfState.waveformToChange >= 0) { wvfState.waveform[wvfState.waveformToChange].gotoTimeHighCycles = wvfState.waveformNewHigh; wvfState.waveform[wvfState.waveformToChange].gotoTimeLowCycles = wvfState.waveformNewLow; @@ -526,6 +528,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { int32_t cyclesToGo = wave->nextServiceCycle - now; if (cyclesToGo < 0) { uint32_t nextEdgeCycles; + uint32_t desired = 0; + uint32_t *timeToUpdate; wvfState.waveformState ^= mask; if (wvfState.waveformState & mask) { if (i == 16) { @@ -535,12 +539,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } #ifdef ENABLE_FEEDBACK if (wave->lastEdge) { - auto desiredLowCycles = adjust(wave->desiredLowCycles); - int32_t err = desiredLowCycles - (now - wave->lastEdge); - if (abs(err) < desiredLowCycles) { // If we've lost > the entire phase, ignore this error signal - err /= 4; - wave->timeLowCycles += err; - } + desired = wave->desiredLowCycles; + timeToUpdate = &wave->timeLowCycles; } #endif nextEdgeCycles = wave->timeHighCycles; @@ -559,16 +559,22 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wave->gotoTimeHighCycles = 0; } else { #ifdef ENABLE_FEEDBACK - auto desiredHighCycles = adjust(wave->desiredHighCycles); - int32_t err = desiredHighCycles - (now - wave->lastEdge); - if (abs(err) < desiredHighCycles) { // If we've lost > the entire phase, ignore this error signal - err /= 4; - wave->timeHighCycles += err; // Feedback 1/4 of the error - } + desired = wave->desiredHighCycles; + timeToUpdate = &wave->timeHighCycles; #endif } nextEdgeCycles = wave->timeLowCycles; } +#ifdef ENABLE_FEEDBACK + if (desired) { + desired = adjust(desired); + int32_t err = desired - (now - wave->lastEdge); + if (abs(err) < desired) { // If we've lost > the entire phase, ignore this error signal + err /= 4; + *timeToUpdate += err; + } + } +#endif nextEdgeCycles = adjust(nextEdgeCycles); wave->nextServiceCycle = now + nextEdgeCycles; nextEventCycles = min_u32(nextEventCycles, nextEdgeCycles); From f42696ca141cc9c82a5e348eae8b5243c5f42ccb Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 29 Apr 2020 12:50:31 -0700 Subject: [PATCH 24/56] Reduce PWM size by using 32b count, indexes Byte-wide operations require extra instructions, so make index and count a full 32-bits wide. --- cores/esp8266/core_esp8266_waveform.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 7d5b7d8622..86a94c30cf 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -135,8 +135,8 @@ constexpr int maxPWMs = 8; // PWM machine state typedef struct { uint32_t mask; // Bitmask of active pins - uint8_t cnt; // How many entries - uint8_t idx; // Where the state machine is along the list + uint32_t cnt; // How many entries + uint32_t idx; // Where the state machine is along the list uint8_t pin[maxPWMs + 1]; uint32_t delta[maxPWMs + 1]; uint32_t nextServiceCycle; // Clock cycle for next step @@ -172,7 +172,7 @@ void _setPWMPeriodCC(uint32_t cc) { PWMState p; // The working copy since we can't edit the one in use p = pwmState; uint32_t ttl = 0; - for (auto i = 0; i < p.cnt; i++) { + for (uint32_t i = 0; i < p.cnt; i++) { uint64_t val64p16 = ((uint64_t)p.delta[i]) << 16; uint64_t newVal64p32 = val64p16 * ratio64p16; p.delta[i] = newVal64p32 >> 32; @@ -193,7 +193,7 @@ void _setPWMPeriodCC(uint32_t cc) { // Helper routine to remove an entry from the state machine static ICACHE_RAM_ATTR void _removePWMEntry(int pin, PWMState *p) { - int i; + uint32_t i; // Find the pin to pull out... for (i = 0; p->pin[i] != pin; i++) { /* no-op */ } @@ -498,7 +498,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } #endif - for (int i = wvfState.startPin; i <= wvfState.endPin; i++) { + for (auto i = wvfState.startPin; i <= wvfState.endPin; i++) { uint32_t mask = 1< Date: Wed, 29 Apr 2020 13:05:26 -0700 Subject: [PATCH 25/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 86a94c30cf..d38b9d81ea 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -475,7 +475,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { GPOS = pwmState.mask; // Set all active pins high // GPIO16 isn't the same as the others if (pwmState.mask & (1<<16)) { - GP16O |= 1; + GP16O = 1; } pwmState.idx = 0; } else { @@ -484,7 +484,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { GPOC = 1<timeHighCycles; } else { if (i == 16) { - GP16O &= ~1; // GPIO16 write slow as it's RMW + GP16O = 0; // GPIO16 write slow as it's RMW } else { ClearGPIO(mask); } From dfaa9cebcab413e90b18d0ecba4aa5ede4c50954 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 2 May 2020 19:45:52 -0700 Subject: [PATCH 26/56] 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. --- 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 d38b9d81ea..b9430ee3fa 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -491,8 +491,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } while (pwmState.delta[pwmState.idx] == 0); } // Preserve duty cycle over PWM period by using now+xxx instead of += delta - pwmState.nextServiceCycle = now + adjust(pwmState.delta[pwmState.idx]); - cyclesToGo = pwmState.nextServiceCycle - now; // Guaranteed to be >= 0 always + cyclesToGo = adjust(pwmState.delta[pwmState.idx]); + pwmState.nextServiceCycle = GetCycleCountIRQ() + cyclesToGo; } nextEventCycles = min_u32(nextEventCycles, cyclesToGo); } From 3909ada55f3b0785131cbf85376f77d18f84e127 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 2 May 2020 20:42:25 -0700 Subject: [PATCH 27/56] Remove redundant GetCycleCount (non-IRQ) --- 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 b9430ee3fa..e3223643ef 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -95,12 +95,6 @@ static WVFState wvfState; // Non-speed critical bits #pragma GCC optimize ("Os") -static inline ICACHE_RAM_ATTR uint32_t GetCycleCount() { - uint32_t ccount; - __asm__ __volatile__("esync; rsr %0,ccount":"=a"(ccount)); - return ccount; -} - // Interrupt on/off control static ICACHE_RAM_ATTR void timer1Interrupt(); static bool timerRunning = false; @@ -301,7 +295,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time return false; } Waveform *wave = &wvfState.waveform[pin]; - wave->expiryCycle = runTimeCycles ? GetCycleCount() + runTimeCycles : 0; + wave->expiryCycle = runTimeCycles ? ESP.getCycleCount() + runTimeCycles : 0; if (runTimeCycles && !wave->expiryCycle) { wave->expiryCycle = 1; // expiryCycle==0 means no timeout, so avoid setting it } @@ -327,7 +321,7 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time wave->lastEdge = 0; 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); + wave->nextServiceCycle = ESP.getCycleCount() + microsecondsToClockCycles(1); wvfState.waveformToEnable |= mask; MEMBARRIER(); if (!timerRunning) { From 413cd175dcc63987c81bb12c3bd728076c350f57 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 2 May 2020 21:22:23 -0700 Subject: [PATCH 28/56] Factor out common timer setup operations --- cores/esp8266/core_esp8266_waveform.cpp | 39 ++++++++++--------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index e3223643ef..1eb8ad88b3 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -100,16 +100,19 @@ static ICACHE_RAM_ATTR void timer1Interrupt(); static bool timerRunning = false; static void initTimer() { - timer1_disable(); - ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); - ETS_FRC_TIMER1_NMI_INTR_ATTACH(timer1Interrupt); - timer1_enable(TIM_DIV1, TIM_EDGE, TIM_SINGLE); - timerRunning = true; + if (!timerRunning) { + timer1_disable(); + ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); + ETS_FRC_TIMER1_NMI_INTR_ATTACH(timer1Interrupt); + timer1_enable(TIM_DIV1, TIM_EDGE, TIM_SINGLE); + timerRunning = true; + timer1_write(microsecondsToClockCycles(10)); + } } static ICACHE_RAM_ATTR void forceTimerInterrupt() { if (T1L > microsecondsToClockCycles(10)) { - timer1_write(microsecondsToClockCycles(10)); + T1L = microsecondsToClockCycles(10); } } @@ -270,13 +273,8 @@ bool _setPWM(int pin, uint32_t cc) { // Set mailbox and wait for ISR to copy it over pwmUpdate = &p; MEMBARRIER(); - if (!timerRunning) { - initTimer(); - timer1_write(microsecondsToClockCycles(10)); - } else { - forceTimerInterrupt(); - } - + initTimer(); + forceTimerInterrupt(); while (pwmUpdate) { delay(0); } @@ -324,12 +322,8 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time wave->nextServiceCycle = ESP.getCycleCount() + microsecondsToClockCycles(1); wvfState.waveformToEnable |= mask; MEMBARRIER(); - if (!timerRunning) { - initTimer(); - timer1_write(microsecondsToClockCycles(10)); - } else { - forceTimerInterrupt(); - } + initTimer(); + forceTimerInterrupt(); while (wvfState.waveformToEnable) { delay(0); // Wait for waveform to update // No mem barrier here, the call to a global function implies global state updated @@ -343,12 +337,11 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time // Set a callback. Pass in NULL to stop it void setTimer1Callback(uint32_t (*fn)()) { wvfState.timer1CB = fn; - if (!timerRunning && fn) { + if (fn) { initTimer(); - timer1_write(microsecondsToClockCycles(1)); // Cause an interrupt post-haste - } else { - disableIdleTimer(); + forceTimerInterrupt(); } + disableIdleTimer(); } From 07f5ff1c33e7e43b2247abcc028486bffe65048a Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 2 May 2020 21:44:26 -0700 Subject: [PATCH 29/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 30 ++++++++++++------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 1eb8ad88b3..882c0155f1 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -524,10 +524,19 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { SetGPIO(mask); } + if (wave->gotoTimeHighCycles) { + // Copy over next full-cycle timings + wave->timeHighCycles = wave->gotoTimeHighCycles; + wave->desiredHighCycles = wave->gotoTimeHighCycles; + wave->timeLowCycles = wave->gotoTimeLowCycles; + wave->desiredLowCycles = wave->gotoTimeLowCycles; + wave->gotoTimeHighCycles = 0; + } else { #ifdef ENABLE_FEEDBACK - if (wave->lastEdge) { - desired = wave->desiredLowCycles; - timeToUpdate = &wave->timeLowCycles; + if (wave->lastEdge) { + desired = wave->desiredLowCycles; + timeToUpdate = &wave->timeLowCycles; + } } #endif nextEdgeCycles = wave->timeHighCycles; @@ -537,19 +546,10 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { ClearGPIO(mask); } - if (wave->gotoTimeHighCycles) { - // Copy over next full-cycle timings - wave->timeHighCycles = wave->gotoTimeHighCycles; - wave->desiredHighCycles = wave->gotoTimeHighCycles; - wave->timeLowCycles = wave->gotoTimeLowCycles; - wave->desiredLowCycles = wave->gotoTimeLowCycles; - wave->gotoTimeHighCycles = 0; - } else { #ifdef ENABLE_FEEDBACK - desired = wave->desiredHighCycles; - timeToUpdate = &wave->timeHighCycles; + desired = wave->desiredHighCycles; + timeToUpdate = &wave->timeHighCycles; #endif - } nextEdgeCycles = wave->timeLowCycles; } #ifdef ENABLE_FEEDBACK @@ -557,7 +557,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { desired = adjust(desired); int32_t err = desired - (now - wave->lastEdge); if (abs(err) < desired) { // If we've lost > the entire phase, ignore this error signal - err /= 4; + err /= 2; *timeToUpdate += err; } } From 539d0d4c146382d395cb98520d2c8d5600cdb439 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 4 May 2020 12:21:01 -0700 Subject: [PATCH 30/56] Reduce code size ~145 bytes --- cores/esp8266/core_esp8266_waveform.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 882c0155f1..1ca98694e3 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -155,19 +155,21 @@ static ICACHE_RAM_ATTR void disableIdleTimer() { } +//#define ENABLE_ONLINECHANGE // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(uint32_t cc) { if (cc == pwmPeriod) { return; } if (pwmState.cnt) { + PWMState p; // The working copy since we can't edit the one in use + p = pwmState; +#ifdef ENABLE_ONLINECHANGE // Adjust any running ones to the best of our abilities by scaling them // Used FP math for speed and code size uint64_t oldCC64p0 = ((uint64_t)pwmPeriod); uint64_t newCC64p16 = ((uint64_t)cc) << 16; uint64_t ratio64p16 = (newCC64p16 / oldCC64p0); - PWMState p; // The working copy since we can't edit the one in use - p = pwmState; uint32_t ttl = 0; for (uint32_t i = 0; i < p.cnt; i++) { uint64_t val64p16 = ((uint64_t)p.delta[i]) << 16; @@ -176,6 +178,11 @@ void _setPWMPeriodCC(uint32_t cc) { ttl += p.delta[i]; } p.delta[p.cnt] = cc - ttl; // Final cleanup exactly cc total cycles +#else + // Turn off all old PWMs + p.mask = 0; + p.cnt = 0; +#endif // Update and wait for mailbox to be emptied pwmUpdate = &p; MEMBARRIER(); @@ -260,8 +267,10 @@ bool _setPWM(int pin, uint32_t cc) { ttl += p.delta[i]; } // Shift everything out by one to make space for new edge - memmove(&p.pin[i + 1], &p.pin[i], (1 + p.cnt - i) * sizeof(p.pin[0])); - memmove(&p.delta[i + 1], &p.delta[i], (1 + p.cnt - i) * sizeof(p.delta[0])); + for (int32_t j = p.cnt; j >= (int)i; j--) { + p.pin[j + 1] = p.pin[j]; + p.delta[j + 1] = p.delta[j]; + } int off = cc - ttl; // The delta from the last edge to the one we're inserting p.pin[i] = pin; p.delta[i] = off; // Add the delta to this new pin From df51b210aa2e7c678881746912b03abf61cbcebc Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 4 May 2020 13:35:06 -0700 Subject: [PATCH 31/56] 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). --- cores/esp8266/core_esp8266_waveform.cpp | 43 +++++++++++++++++++++---- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 1ca98694e3..3d6b1aeac6 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -195,8 +195,27 @@ void _setPWMPeriodCC(uint32_t cc) { pwmPeriod = cc; } +// Helper routine to clean up any tagged off pins +static void _cleaupPWM(PWMState *p) { + uint32_t leftover = 0; + uint32_t in, out; + for (in = 0, out = 0; in < p->cnt; in++) { + if (p->mask & (1<pin[in])) { + p->pin[out] = p->pin[in]; + p->delta[out] = p->delta[in] + leftover; + leftover = 0; + out++; + } else { + leftover += p->delta[in]; + } + } + p->cnt = out; + p->pin[out] = 0xff; + p->delta[out] = p->delta[in] + leftover; +} + // Helper routine to remove an entry from the state machine -static ICACHE_RAM_ATTR void _removePWMEntry(int pin, PWMState *p) { +static void _removePWMEntry(int pin, PWMState *p) { uint32_t i; // Find the pin to pull out... @@ -224,9 +243,16 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { PWMState p; // The working copy since we can't edit the one in use p = pwmState; - _removePWMEntry(pin, &p); - // Update and wait for mailbox to be emptied + // In _stopPWM we just clear the mask but keep everything else + // untouched to save IRAM. The main startPWM will handle cleanup. + p.mask &= ~(1< Date: Mon, 4 May 2020 18:35:19 -0700 Subject: [PATCH 32/56] Fix typo in PWM pin 1->0 transition Actually check the pin mask is active before setting the PWM pin low. D'oh. --- cores/esp8266/core_esp8266_waveform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 3d6b1aeac6..0a2ae2114e 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -504,7 +504,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { do { // Drop the pin at this edge - if (1< Date: Tue, 5 May 2020 08:13:46 -0700 Subject: [PATCH 33/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 33 +++++-------------------- 1 file changed, 6 insertions(+), 27 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 0a2ae2114e..25c0278c8d 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -195,18 +195,20 @@ void _setPWMPeriodCC(uint32_t cc) { pwmPeriod = cc; } -// Helper routine to clean up any tagged off pins -static void _cleaupPWM(PWMState *p) { +// Helper routine to remove an entry from the state machine +// and clean up any marked-off entries +static void _cleanAndRemovePWM(PWMState *p, int pin) { uint32_t leftover = 0; uint32_t in, out; for (in = 0, out = 0; in < p->cnt; in++) { - if (p->mask & (1<pin[in])) { + if ((p->pin[in] != pin) && (p->mask & (1<pin[in]))) { p->pin[out] = p->pin[in]; p->delta[out] = p->delta[in] + leftover; leftover = 0; out++; } else { leftover += p->delta[in]; + p->mask &= ~(1<pin[in]); } } p->cnt = out; @@ -214,26 +216,6 @@ static void _cleaupPWM(PWMState *p) { p->delta[out] = p->delta[in] + leftover; } -// Helper routine to remove an entry from the state machine -static void _removePWMEntry(int pin, PWMState *p) { - uint32_t i; - - // Find the pin to pull out... - for (i = 0; p->pin[i] != pin; i++) { /* no-op */ } - auto delta = p->delta[i]; - - // Add the removed previous pin delta to preserve absolute position - p->delta[i+1] += delta; - - // Move everything back one - for (i++; i <= p->cnt; i++) { - p->pin[i-1] = p->pin[i]; - p->delta[i-1] = p->delta[i]; - } - // Remove the pin from the active list - p->mask &= ~(1<cnt--; -} // Called by analogWrite(0/100%) to disable PWM on a specific pin ICACHE_RAM_ATTR bool _stopPWM(int pin) { @@ -270,11 +252,8 @@ bool _setPWM(int pin, uint32_t cc) { stopWaveform(pin); PWMState p; // Working copy p = pwmState; - _cleaupPWM(&p); // Get rid of any entries for this pin - if ((1<= maxPWMs) { return false; // No space left From 9424090af2e6e6fca64bd9c99a2630f4a0b4b3d0 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 08:31:45 -0700 Subject: [PATCH 34/56] Remove unused analogMap, toneMap Save ~100 bytes of IROM by removing the tone/analog pin tracking from the interface functions. They were completely unused. --- cores/esp8266/Tone.cpp | 9 +-------- cores/esp8266/core_esp8266_wiring_pwm.cpp | 4 ---- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/cores/esp8266/Tone.cpp b/cores/esp8266/Tone.cpp index d4df0d779f..b4dc93c642 100644 --- a/cores/esp8266/Tone.cpp +++ b/cores/esp8266/Tone.cpp @@ -25,10 +25,6 @@ #include "core_esp8266_waveform.h" #include "user_interface.h" -// Which pins have a tone running on them? -static uint32_t _toneMap = 0; - - static void _startTone(uint8_t _pin, uint32_t high, uint32_t low, uint32_t duration) { if (_pin > 16) { return; @@ -47,9 +43,7 @@ static void _startTone(uint8_t _pin, uint32_t high, uint32_t low, uint32_t durat duration = microsecondsToClockCycles(duration * 1000UL); duration += high + low - 1; duration -= duration % (high + low); - if (startWaveformClockCycles(_pin, high, low, duration)) { - _toneMap |= 1 << _pin; - } + startWaveformClockCycles(_pin, high, low, duration); } @@ -91,6 +85,5 @@ void noTone(uint8_t _pin) { return; } stopWaveform(_pin); - _toneMap &= ~(1 << _pin); digitalWrite(_pin, 0); } diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index fd297310d1..56bb269cab 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -26,7 +26,6 @@ extern "C" { -static uint32_t analogMap = 0; static int32_t analogScale = PWMRANGE; static uint16_t analogFreq = 1000; @@ -36,7 +35,6 @@ extern void __analogWriteRange(uint32_t range) { } } - extern void __analogWriteFreq(uint32_t freq) { if (freq < 100) { analogFreq = 100; @@ -62,7 +60,6 @@ extern void __analogWrite(uint8_t pin, int val) { val = analogScale; } - analogMap &= ~(1 << pin); uint32_t high = (analogPeriod * val) / analogScale; uint32_t low = analogPeriod - high; pinMode(pin, OUTPUT); @@ -74,7 +71,6 @@ extern void __analogWrite(uint8_t pin, int val) { digitalWrite(pin, LOW); } else { _setPWM(pin, high); - analogMap |= (1 << pin); } } From c8b53ef7c285b882a6916f372daf6fe2cc76fc15 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 09:36:29 -0700 Subject: [PATCH 35/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 56 ++++++++++++------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 25c0278c8d..dd34d029a9 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -55,13 +55,11 @@ 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 timeHighCycles; // Currently running waveform period + uint32_t timeHighCycles; // Actual running waveform period (adjusted using desiredCycles) uint32_t timeLowCycles; // - uint32_t desiredHighCycles; // Currently running waveform period - uint32_t desiredLowCycles; // - uint32_t gotoTimeHighCycles; // Copied over on the next period to preserve phase - uint32_t gotoTimeLowCycles; // - uint32_t lastEdge; // + uint32_t desiredHighCycles; // Ideal waveform period to drive the error signal + uint32_t desiredLowCycles; // + uint32_t lastEdge; // Cycle when this generator last changed } Waveform; class WVFState { @@ -74,7 +72,7 @@ class WVFState { uint32_t waveformToEnable = 0; // Message to the NMI handler to start a waveform on a inactive pin uint32_t waveformToDisable = 0; // Message to the NMI handler to disable a pin from waveform generation - int32_t waveformToChange = -1; + uint32_t waveformToChange = 0; // Mask of pin to change. One bit set in main app, cleared when effected in the NMI uint32_t waveformNewHigh = 0; uint32_t waveformNewLow = 0; @@ -83,8 +81,8 @@ class WVFState { // 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; + uint16_t startPin = 0; + uint16_t endPin = 0; }; static WVFState wvfState; @@ -318,22 +316,22 @@ int startWaveformClockCycles(uint8_t pin, uint32_t timeHighCycles, uint32_t time uint32_t mask = 1<= 0) { + // Make sure no waveform changes are waiting to be applied + while (wvfState.waveformToChange) { delay(0); // Wait for waveform to update // No mem barrier here, the call to a global function implies global state updated } + wvfState.waveformNewHigh = timeHighCycles; + wvfState.waveformNewLow = timeLowCycles; + MEMBARRIER(); + wvfState.waveformToChange = mask; + // The waveform will be updated some time in the future on the next period for the signal } else { // if (!(wvfState.waveformEnabled & mask)) { wave->timeHighCycles = timeHighCycles; wave->timeLowCycles = timeLowCycles; wave->desiredHighCycles = wave->timeHighCycles; wave->desiredLowCycles = wave->timeLowCycles; wave->lastEdge = 0; - wave->gotoTimeHighCycles = wave->timeHighCycles; - wave->gotoTimeLowCycles = wave->timeLowCycles; // Actually set the pin high or low in the IRQ service to guarantee times wave->nextServiceCycle = ESP.getCycleCount() + microsecondsToClockCycles(1); wvfState.waveformToEnable |= mask; MEMBARRIER(); @@ -387,8 +385,13 @@ 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 - if (wvfState.waveformEnabled & (1UL << pin)) { - wvfState.waveformToDisable = 1UL << pin; + uint32_t mask = 1<= 0) { - wvfState.waveform[wvfState.waveformToChange].gotoTimeHighCycles = wvfState.waveformNewHigh; - wvfState.waveform[wvfState.waveformToChange].gotoTimeLowCycles = wvfState.waveformNewLow; - wvfState.waveformToChange = -1; - // No need for memory barrier here. The global has to be written before exit the ISR. } bool done = false; @@ -541,13 +539,13 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } else { SetGPIO(mask); } - if (wave->gotoTimeHighCycles) { + if (wvfState.waveformToChange & mask) { // Copy over next full-cycle timings - wave->timeHighCycles = wave->gotoTimeHighCycles; - wave->desiredHighCycles = wave->gotoTimeHighCycles; - wave->timeLowCycles = wave->gotoTimeLowCycles; - wave->desiredLowCycles = wave->gotoTimeLowCycles; - wave->gotoTimeHighCycles = 0; + wave->timeHighCycles = wvfState.waveformNewHigh; + wave->desiredHighCycles = wvfState.waveformNewHigh; + wave->timeLowCycles = wvfState.waveformNewLow; + wave->desiredLowCycles = wvfState.waveformNewLow; + wvfState.waveformToChange = 0; } else { #ifdef ENABLE_FEEDBACK if (wave->lastEdge) { From e6b7aa1d201c60e66bbf10cd880cab50de5cdbe9 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 10:52:24 -0700 Subject: [PATCH 36/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 10 +++++----- cores/esp8266/core_esp8266_wiring_pwm.cpp | 16 +++++++--------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index dd34d029a9..aa7cec799a 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -139,7 +139,7 @@ typedef struct { static PWMState pwmState; static PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR -static uint32_t pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; +uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; @@ -156,7 +156,7 @@ static ICACHE_RAM_ATTR void disableIdleTimer() { //#define ENABLE_ONLINECHANGE // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(uint32_t cc) { - if (cc == pwmPeriod) { + if (cc == _pwmPeriod) { return; } if (pwmState.cnt) { @@ -165,7 +165,7 @@ void _setPWMPeriodCC(uint32_t cc) { #ifdef ENABLE_ONLINECHANGE // Adjust any running ones to the best of our abilities by scaling them // Used FP math for speed and code size - uint64_t oldCC64p0 = ((uint64_t)pwmPeriod); + uint64_t oldCC64p0 = ((uint64_t)_pwmPeriod); uint64_t newCC64p16 = ((uint64_t)cc) << 16; uint64_t ratio64p16 = (newCC64p16 / oldCC64p0); uint32_t ttl = 0; @@ -190,7 +190,7 @@ void _setPWMPeriodCC(uint32_t cc) { // No mem barrier. The external function call guarantees it's re-read } } - pwmPeriod = cc; + _pwmPeriod = cc; } // Helper routine to remove an entry from the state machine @@ -260,7 +260,7 @@ bool _setPWM(int pin, uint32_t cc) { p.pin[0] = pin; p.delta[0] = cc; p.pin[1] = 0xff; - p.delta[1] = pwmPeriod - cc; + p.delta[1] = _pwmPeriod - cc; p.cnt = 1; p.mask = 1< 0) { @@ -37,13 +37,13 @@ extern void __analogWriteRange(uint32_t range) { extern void __analogWriteFreq(uint32_t freq) { if (freq < 100) { - analogFreq = 100; + freq = 100; } else if (freq > 60000) { - analogFreq = 60000; + freq = 60000; } else { - analogFreq = freq; + freq = freq; } - uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / analogFreq; + uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / freq; _setPWMPeriodCC(analogPeriod); } @@ -52,16 +52,14 @@ extern void __analogWrite(uint8_t pin, int val) { return; } - uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / analogFreq; - _setPWMPeriodCC(analogPeriod); if (val < 0) { val = 0; } else if (val > analogScale) { val = analogScale; } - uint32_t high = (analogPeriod * val) / analogScale; - uint32_t low = analogPeriod - high; + uint32_t high = (_pwmPeriod * val) / analogScale; + uint32_t low = _pwmPeriod - high; pinMode(pin, OUTPUT); if (low == 0) { _stopPWM(pin); From 28645ff764ff69731645dc8ade76b880ad8e38ff Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 11:14:29 -0700 Subject: [PATCH 37/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 112 +++++++----------------- 1 file changed, 33 insertions(+), 79 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index aa7cec799a..7a41731cd7 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -47,10 +47,6 @@ extern "C" { // Maximum delay between IRQs #define MAXIRQUS (10000) -// Set/clear GPIO 0-15 by bitmask -#define SetGPIO(a) do { GPOS = a; } while (0) -#define ClearGPIO(a) do { GPOC = a; } while (0) - // Waveform generator can create tones, PWM, and servos typedef struct { uint32_t nextServiceCycle; // ESP cycle timer when a transition required @@ -128,21 +124,22 @@ static ICACHE_RAM_ATTR void forceTimerInterrupt() { constexpr int maxPWMs = 8; // PWM machine state -typedef struct { +typedef struct PWMState { uint32_t mask; // Bitmask of active pins uint32_t cnt; // How many entries uint32_t idx; // Where the state machine is along the list uint8_t pin[maxPWMs + 1]; uint32_t delta[maxPWMs + 1]; uint32_t nextServiceCycle; // Clock cycle for next step + struct PWMState *pwmUpdate; // Set by main code, cleared by ISR } PWMState; static PWMState pwmState; -static PWMState *pwmUpdate = nullptr; // Set by main code, cleared by ISR uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; - +// If there are no more scheduled activities, shut down Timer 1. +// Otherwise, do nothing. static ICACHE_RAM_ATTR void disableIdleTimer() { if (timerRunning && !wvfState.waveformEnabled && !pwmState.cnt && !wvfState.timer1CB) { ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL); @@ -152,8 +149,21 @@ static ICACHE_RAM_ATTR void disableIdleTimer() { } } +// Notify the NMI that a new PWM state is available through the mailbox. +// Wait for mailbox to be emptied (either busy or delay() as needed) +static ICACHE_RAM_ATTR void _notifyPWM(PWMState *p, bool idle) { + p->pwmUpdate = nullptr; + pwmState.pwmUpdate = p; + MEMBARRIER(); + forceTimerInterrupt(); + while (pwmState.pwmUpdate) { + if (idle) { + delay(0); + } + MEMBARRIER(); + } +} -//#define ENABLE_ONLINECHANGE // Called when analogWriteFreq() changed to update the PWM total period void _setPWMPeriodCC(uint32_t cc) { if (cc == _pwmPeriod) { @@ -162,33 +172,11 @@ void _setPWMPeriodCC(uint32_t cc) { if (pwmState.cnt) { PWMState p; // The working copy since we can't edit the one in use p = pwmState; -#ifdef ENABLE_ONLINECHANGE - // Adjust any running ones to the best of our abilities by scaling them - // Used FP math for speed and code size - uint64_t oldCC64p0 = ((uint64_t)_pwmPeriod); - uint64_t newCC64p16 = ((uint64_t)cc) << 16; - uint64_t ratio64p16 = (newCC64p16 / oldCC64p0); - uint32_t ttl = 0; - for (uint32_t i = 0; i < p.cnt; i++) { - uint64_t val64p16 = ((uint64_t)p.delta[i]) << 16; - uint64_t newVal64p32 = val64p16 * ratio64p16; - p.delta[i] = newVal64p32 >> 32; - ttl += p.delta[i]; - } - p.delta[p.cnt] = cc - ttl; // Final cleanup exactly cc total cycles -#else // Turn off all old PWMs p.mask = 0; p.cnt = 0; -#endif // Update and wait for mailbox to be emptied - pwmUpdate = &p; - MEMBARRIER(); - forceTimerInterrupt(); - while (pwmUpdate) { - delay(0); - // No mem barrier. The external function call guarantees it's re-read - } + _notifyPWM(&p, true); } _pwmPeriod = cc; } @@ -215,7 +203,7 @@ static void _cleanAndRemovePWM(PWMState *p, int pin) { } -// Called by analogWrite(0/100%) to disable PWM on a specific pin +// Disable PWM on a specific pin (i.e. when a digitalWrite or analogWrite(0%/100%)) ICACHE_RAM_ATTR bool _stopPWM(int pin) { if (!((1<> (turbo ? 0 : 1)) #endif -#define ENABLE_ADJUST // Adjust takes 36 bytes -#define ENABLE_FEEDBACK // Feedback costs 68 bytes -#define ENABLE_PWM // PWM takes 160 bytes - -#ifndef ENABLE_ADJUST - #undef adjust - #define adjust(x) (x) -#endif - static ICACHE_RAM_ATTR void timer1Interrupt() { // Flag if the core is at 160 MHz, for use by adjust() @@ -445,14 +412,12 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wvfState.startPin = __builtin_ffs(wvfState.waveformEnabled) - 1; // Find the last bit by subtracting off GCC's count-leading-zeros (no offset in this one) wvfState.endPin = 32 - __builtin_clz(wvfState.waveformEnabled); -#ifdef ENABLE_PWM - } else if (!pwmState.cnt && pwmUpdate) { + } else if (!pwmState.cnt && pwmState.pwmUpdate) { // Start up the PWM generator by copying from the mailbox pwmState.cnt = 1; pwmState.idx = 1; // Ensure copy this cycle, cause it to start at t=0 pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop! // No need for mem barrier here. Global must be written by IRQ exit -#endif } bool done = false; @@ -460,17 +425,15 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { do { nextEventCycles = microsecondsToClockCycles(MAXIRQUS); -#ifdef ENABLE_PWM // PWM state machine implementation if (pwmState.cnt) { uint32_t now = GetCycleCountIRQ(); int32_t cyclesToGo = pwmState.nextServiceCycle - now; if (cyclesToGo < 0) { if (pwmState.idx == pwmState.cnt) { // Start of pulses, possibly copy new - if (pwmUpdate) { + if (pwmState.pwmUpdate) { // Do the memory copy from temp to global and clear mailbox - pwmState = *(PWMState*)pwmUpdate; - pwmUpdate = nullptr; + pwmState = *(PWMState*)pwmState.pwmUpdate; } GPOS = pwmState.mask; // Set all active pins high // GPIO16 isn't the same as the others @@ -498,7 +461,6 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { } nextEventCycles = min_u32(nextEventCycles, cyclesToGo); } -#endif for (auto i = wvfState.startPin; i <= wvfState.endPin; i++) { uint32_t mask = 1<expiryCycle - now; if (expiryToGo < 0) { // Done, remove! - wvfState.waveformEnabled &= ~mask; if (i == 16) { GP16O = 0; - } else { - ClearGPIO(mask); - } + } + GPOC = mask; + wvfState.waveformEnabled &= ~mask; continue; } } @@ -536,9 +497,9 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { if (wvfState.waveformState & mask) { if (i == 16) { GP16O = 1; // GPIO16 write slow as it's RMW - } else { - SetGPIO(mask); } + GPOS = mask; + if (wvfState.waveformToChange & mask) { // Copy over next full-cycle timings wave->timeHighCycles = wvfState.waveformNewHigh; @@ -547,27 +508,21 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wave->desiredLowCycles = wvfState.waveformNewLow; wvfState.waveformToChange = 0; } else { -#ifdef ENABLE_FEEDBACK if (wave->lastEdge) { desired = wave->desiredLowCycles; timeToUpdate = &wave->timeLowCycles; } } -#endif nextEdgeCycles = wave->timeHighCycles; } else { if (i == 16) { GP16O = 0; // GPIO16 write slow as it's RMW - } else { - ClearGPIO(mask); } -#ifdef ENABLE_FEEDBACK + GPOC = mask; desired = wave->desiredHighCycles; timeToUpdate = &wave->timeHighCycles; -#endif nextEdgeCycles = wave->timeLowCycles; } -#ifdef ENABLE_FEEDBACK if (desired) { desired = adjust(desired); int32_t err = desired - (now - wave->lastEdge); @@ -576,7 +531,6 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { *timeToUpdate += err; } } -#endif nextEdgeCycles = adjust(nextEdgeCycles); wave->nextServiceCycle = now + nextEdgeCycles; nextEventCycles = min_u32(nextEventCycles, nextEdgeCycles); From 174d19eecf018b472256356e83b6dd33cdfefb94 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 12:14:59 -0700 Subject: [PATCH 38/56] Clean up old comments --- 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 7a41731cd7..415202256e 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -496,7 +496,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { wvfState.waveformState ^= mask; if (wvfState.waveformState & mask) { if (i == 16) { - GP16O = 1; // GPIO16 write slow as it's RMW + GP16O = 1; } GPOS = mask; @@ -516,7 +516,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { nextEdgeCycles = wave->timeHighCycles; } else { if (i == 16) { - GP16O = 0; // GPIO16 write slow as it's RMW + GP16O = 0; } GPOC = mask; desired = wave->desiredHighCycles; From a910eaebd047597bc24806de145dfcc322504c6c Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 13:09:32 -0700 Subject: [PATCH 39/56] Fix indent, remove some unneeded if-else branches --- cores/esp8266/core_esp8266_waveform.cpp | 75 ++++++++++++------------- 1 file changed, 36 insertions(+), 39 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 415202256e..006b84773e 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -198,7 +198,7 @@ static void _cleanAndRemovePWM(PWMState *p, int pin) { } } p->cnt = out; - p->pin[out] = 0xff; + // Final pin is never used: p->pin[out] = 0xff; p->delta[out] = p->delta[in] + leftover; } @@ -237,16 +237,16 @@ bool _setPWM(int pin, uint32_t cc) { // And add it to the list, in order if (p.cnt >= maxPWMs) { return false; // No space left - } else if (p.cnt == 0) { + } + + if (p.cnt == 0) { // Starting up from scratch, special case 1st element and PWM period p.pin[0] = pin; p.delta[0] = cc; - p.pin[1] = 0xff; + // Final pin is never used: p.pin[1] = 0xff; p.delta[1] = _pwmPeriod - cc; - p.cnt = 1; - p.mask = 1<timeHighCycles = timeHighCycles; + wave->desiredHighCycles = timeHighCycles; wave->timeLowCycles = timeLowCycles; - wave->desiredHighCycles = wave->timeHighCycles; - wave->desiredLowCycles = wave->timeLowCycles; + wave->desiredLowCycles = timeLowCycles; wave->lastEdge = 0; wave->nextServiceCycle = ESP.getCycleCount() + microsecondsToClockCycles(1); wvfState.waveformToEnable |= mask; @@ -427,33 +427,30 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { // PWM state machine implementation if (pwmState.cnt) { - uint32_t now = GetCycleCountIRQ(); - int32_t cyclesToGo = pwmState.nextServiceCycle - now; + int32_t cyclesToGo = pwmState.nextServiceCycle - GetCycleCountIRQ(); if (cyclesToGo < 0) { if (pwmState.idx == pwmState.cnt) { // Start of pulses, possibly copy new - if (pwmState.pwmUpdate) { - // Do the memory copy from temp to global and clear mailbox - pwmState = *(PWMState*)pwmState.pwmUpdate; - } - GPOS = pwmState.mask; // Set all active pins high - // GPIO16 isn't the same as the others - if (pwmState.mask & (1<<16)) { - GP16O = 1; - } - pwmState.idx = 0; + if (pwmState.pwmUpdate) { + // Do the memory copy from temp to global and clear mailbox + pwmState = *(PWMState*)pwmState.pwmUpdate; + } + GPOS = pwmState.mask; // Set all active pins high + if (pwmState.mask & (1<<16)) { + GP16O = 1; + } + pwmState.idx = 0; } else { - do { - // Drop the pin at this edge - if (pwmState.mask & (1<desiredHighCycles = wvfState.waveformNewHigh; wave->timeLowCycles = wvfState.waveformNewLow; wave->desiredLowCycles = wvfState.waveformNewLow; + wave->lastEdge = 0; wvfState.waveformToChange = 0; - } else { - if (wave->lastEdge) { - desired = wave->desiredLowCycles; - timeToUpdate = &wave->timeLowCycles; - } + } + if (wave->lastEdge) { + desired = wave->desiredLowCycles; + timeToUpdate = &wave->timeLowCycles; } nextEdgeCycles = wave->timeHighCycles; } else { From 179b9d654cfdba3978fff38afa848b40f9f6859a Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 5 May 2020 13:53:56 -0700 Subject: [PATCH 40/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 006b84773e..02424b756a 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -93,7 +93,7 @@ static WVFState wvfState; static ICACHE_RAM_ATTR void timer1Interrupt(); static bool timerRunning = false; -static void initTimer() { +static __attribute__((noinline)) void initTimer() { if (!timerRunning) { timer1_disable(); ETS_FRC_TIMER1_INTR_ATTACH(NULL, NULL); @@ -176,7 +176,9 @@ void _setPWMPeriodCC(uint32_t cc) { p.mask = 0; p.cnt = 0; // Update and wait for mailbox to be emptied + initTimer(); _notifyPWM(&p, true); + disableIdleTimer(); } _pwmPeriod = cc; } @@ -266,6 +268,7 @@ bool _setPWM(int pin, uint32_t cc) { p.mask |= 1< Date: Wed, 6 May 2020 11:08:07 -0700 Subject: [PATCH 41/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 02424b756a..83ec67aa00 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -560,7 +560,6 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { // Do it here instead of global function to save time and because we know it's edge-IRQ T1L = nextEventCycles >> (turbo ? 1 : 0); - TEIE |= TEIE1; // Edge int enable } }; From 7fe9a2d7d8f3ecc1e82781d923d977edb87b55df Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 7 May 2020 09:21:49 -0700 Subject: [PATCH 42/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 65 +++++++++++++++++------ cores/esp8266/core_esp8266_waveform.h | 4 +- cores/esp8266/core_esp8266_wiring_pwm.cpp | 16 +----- 3 files changed, 53 insertions(+), 32 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 83ec67aa00..032f46735b 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -164,23 +164,31 @@ static ICACHE_RAM_ATTR void _notifyPWM(PWMState *p, bool idle) { } } +static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range); + // Called when analogWriteFreq() changed to update the PWM total period -void _setPWMPeriodCC(uint32_t cc) { +void _setPWMFreq(uint32_t freq) { + // Convert frequency into clock cycles + uint32_t cc = microsecondsToClockCycles(1000000UL) / freq; + if (cc == _pwmPeriod) { - return; + return; // No change } + + _pwmPeriod = cc; + if (pwmState.cnt) { PWMState p; // The working copy since we can't edit the one in use - p = pwmState; - // Turn off all old PWMs - p.mask = 0; p.cnt = 0; + for (uint32_t i = 0; i < pwmState.cnt; i++) { + auto pin = pwmState.pin[i]; + _addPWMtoList(p, pin, wvfState.waveform[pin].desiredHighCycles, wvfState.waveform[pin].desiredLowCycles); + } // Update and wait for mailbox to be emptied initTimer(); _notifyPWM(&p, true); disableIdleTimer(); } - _pwmPeriod = cc; } // Helper routine to remove an entry from the state machine @@ -229,16 +237,25 @@ ICACHE_RAM_ATTR bool _stopPWM(int pin) { return true; } -// Called by analogWrite(1...99%) to set the PWM duty in clock cycles -bool _setPWM(int pin, uint32_t cc) { - stopWaveform(pin); - PWMState p; // Working copy - p = pwmState; - // Get rid of any entries for this pin - _cleanAndRemovePWM(&p, pin); - // And add it to the list, in order - if (p.cnt >= maxPWMs) { - return false; // No space left +static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range) { + // Stash the val and range so we can re-evaluate the fraction + // should the user change PWM frequency. This allows us to + // give as great a precision as possible. We know by construction + // that the waveform for this pin will be inactive so we can borrow + // memory from that structure. + wvfState.waveform[pin].desiredHighCycles = val; // Numerator == high + wvfState.waveform[pin].desiredLowCycles = range; // Denominator == low + + uint32_t cc = (_pwmPeriod * val) / range; + + if (cc == 0) { + _stopPWM(pin); + digitalWrite(pin, HIGH); + return; + } else if (cc >= _pwmPeriod) { + _stopPWM(pin); + digitalWrite(pin, LOW); + return; } if (p.cnt == 0) { @@ -266,10 +283,26 @@ bool _setPWM(int pin, uint32_t cc) { } p.cnt++; p.mask |= 1<= maxPWMs) { + return false; // No space left + } + + _addPWMtoList(p, pin, val, range); // Set mailbox and wait for ISR to copy it over initTimer(); _notifyPWM(&p, true); + disableIdleTimer(); return true; } diff --git a/cores/esp8266/core_esp8266_waveform.h b/cores/esp8266/core_esp8266_waveform.h index fd727fedf3..d8ed42ba10 100644 --- a/cores/esp8266/core_esp8266_waveform.h +++ b/cores/esp8266/core_esp8266_waveform.h @@ -72,9 +72,9 @@ void setTimer1Callback(uint32_t (*fn)()); // Internal-only calls, not for applications -extern void _setPWMPeriodCC(uint32_t cc); +extern void _setPWMFreq(uint32_t freq); extern bool _stopPWM(int pin); -extern bool _setPWM(int pin, uint32_t cc); +extern bool _setPWM(int pin, uint32_t val, uint32_t range); #ifdef __cplusplus } diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index 0f7612924c..cd21c8a6a5 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -27,7 +27,6 @@ extern "C" { static int32_t analogScale = PWMRANGE; -extern uint32_t _pwmPeriod; extern void __analogWriteRange(uint32_t range) { if (range > 0) { @@ -43,8 +42,7 @@ extern void __analogWriteFreq(uint32_t freq) { } else { freq = freq; } - uint32_t analogPeriod = microsecondsToClockCycles(1000000UL) / freq; - _setPWMPeriodCC(analogPeriod); + _setPWMFreq(freq); } extern void __analogWrite(uint8_t pin, int val) { @@ -58,18 +56,8 @@ extern void __analogWrite(uint8_t pin, int val) { val = analogScale; } - uint32_t high = (_pwmPeriod * val) / analogScale; - uint32_t low = _pwmPeriod - high; pinMode(pin, OUTPUT); - if (low == 0) { - _stopPWM(pin); - digitalWrite(pin, HIGH); - } else if (high == 0) { - _stopPWM(pin); - digitalWrite(pin, LOW); - } else { - _setPWM(pin, high); - } + _setPWM(pin, val, analogScale); } extern void analogWrite(uint8_t pin, int val) __attribute__((weak, alias("__analogWrite"))); From e7cb533683634997dd65b4107d3ce6efd955edb1 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 7 May 2020 18:18:52 -0700 Subject: [PATCH 43/56] Adjust for fixed overhead on PWM period Pulls the actual PWM period closer to the requested one with a simple, 0-overhead static adjustment. --- cores/esp8266/core_esp8266_waveform.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 032f46735b..a0a753ef77 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -135,7 +135,7 @@ typedef struct PWMState { } PWMState; static PWMState pwmState; -uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; +static uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; // If there are no more scheduled activities, shut down Timer 1. @@ -171,6 +171,13 @@ void _setPWMFreq(uint32_t freq) { // Convert frequency into clock cycles uint32_t cc = microsecondsToClockCycles(1000000UL) / freq; + // Simple static adjustment to bring period closer to requested due to overhead +#if F_CPU == 80000000 + cc -= microsecondsToClockCycles(2); +#else + cc -= microsecondsToClockCycles(1); +#endif + if (cc == _pwmPeriod) { return; // No change } From 083560d651bea01175fac6313dcde9c7a7e69983 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Fri, 8 May 2020 08:05:46 -0700 Subject: [PATCH 44/56] Fix value reversal when analogWrite out of range Silly mistake, swapped high and low values when checking analogWrite for over/under values. Fixed --- 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 a0a753ef77..e491410929 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -257,11 +257,11 @@ static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range) { if (cc == 0) { _stopPWM(pin); - digitalWrite(pin, HIGH); + digitalWrite(pin, LOW); return; } else if (cc >= _pwmPeriod) { _stopPWM(pin); - digitalWrite(pin, LOW); + digitalWrite(pin, HIGH); return; } From 5be49610cf55c799518ec15e93e3cc175abe2e4a Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 10 May 2020 11:15:42 -0700 Subject: [PATCH 45/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 39 ++++++++++++------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index e491410929..e0e1d0bc13 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -376,26 +376,6 @@ void setTimer1Callback(uint32_t (*fn)()) { disableIdleTimer(); } - -// Speed critical bits -#pragma GCC optimize ("O2") - -// Normally would not want two copies like this, but due to different -// optimization levels the inline attribute gets lost if we try the -// other version. -static inline ICACHE_RAM_ATTR uint32_t GetCycleCountIRQ() { - uint32_t ccount; - __asm__ __volatile__("rsr %0,ccount":"=a"(ccount)); - return ccount; -} - -static inline ICACHE_RAM_ATTR uint32_t min_u32(uint32_t a, uint32_t b) { - if (a < b) { - return a; - } - return b; -} - // Stops a waveform on a pin int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { // Can't possibly need to stop anything if there is no timer active @@ -421,6 +401,25 @@ int ICACHE_RAM_ATTR stopWaveform(uint8_t pin) { return true; } +// Speed critical bits +#pragma GCC optimize ("O2") + +// Normally would not want two copies like this, but due to different +// optimization levels the inline attribute gets lost if we try the +// other version. +static inline ICACHE_RAM_ATTR uint32_t GetCycleCountIRQ() { + uint32_t ccount; + __asm__ __volatile__("rsr %0,ccount":"=a"(ccount)); + return ccount; +} + +static inline ICACHE_RAM_ATTR uint32_t min_u32(uint32_t a, uint32_t b) { + if (a < b) { + return a; + } + return b; +} + // The SDK and hardware take some time to actually get to our NMI code, so // decrement the next IRQ's timer value by a bit so we can actually catch the // real CPU cycle counter we want for the waveforms. From 051008ace44fbb6b30aa2ca4804af36e10767d0d Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 12 May 2020 18:42:14 -0700 Subject: [PATCH 46/56] Avoid side effects in addPWMtoList --- cores/esp8266/core_esp8266_waveform.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index e0e1d0bc13..36b8eac04e 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -255,14 +255,11 @@ static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range) { uint32_t cc = (_pwmPeriod * val) / range; + // Clip to sane values in the case we go from OK to not-OK when adjusting frequencies if (cc == 0) { - _stopPWM(pin); - digitalWrite(pin, LOW); - return; + cc = 1; } else if (cc >= _pwmPeriod) { - _stopPWM(pin); - digitalWrite(pin, HIGH); - return; + cc = _pwmPeriod - 1; } if (p.cnt == 0) { @@ -304,6 +301,13 @@ bool _setPWM(int pin, uint32_t val, uint32_t range) { return false; // No space left } + // Sanity check for all-on/off + uint32_t cc = (_pwmPeriod * val) / range; + if ((cc == 0) || (cc >= _pwmPeriod)) { + digitalWrite(pin, cc ? HIGH : LOW); + return true; + } + _addPWMtoList(p, pin, val, range); // Set mailbox and wait for ISR to copy it over From 6692418b3e135f35dc6638e9056e6be1a80c7853 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 18 May 2020 16:03:19 -0700 Subject: [PATCH 47/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 36b8eac04e..b116e0a17b 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -135,7 +135,8 @@ typedef struct PWMState { } PWMState; static PWMState pwmState; -static uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / 1000; +static uint32_t _pwmFreq = 1000; +static uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / _pwmFreq; // If there are no more scheduled activities, shut down Timer 1. @@ -168,14 +169,17 @@ static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range); // Called when analogWriteFreq() changed to update the PWM total period void _setPWMFreq(uint32_t freq) { + _pwmFreq = freq; + // Convert frequency into clock cycles uint32_t cc = microsecondsToClockCycles(1000000UL) / freq; // Simple static adjustment to bring period closer to requested due to overhead + // Empirically determined as a constant PWM delay and a function of the number of PWMs #if F_CPU == 80000000 - cc -= microsecondsToClockCycles(2); + cc -= ((microsecondsToClockCycles(pwmState.cnt) * 13) >> 4) + 110; #else - cc -= microsecondsToClockCycles(1); + cc -= ((microsecondsToClockCycles(pwmState.cnt) * 10) >> 4) + 75; #endif if (cc == _pwmPeriod) { @@ -314,6 +318,10 @@ bool _setPWM(int pin, uint32_t val, uint32_t range) { initTimer(); _notifyPWM(&p, true); disableIdleTimer(); + + // Potentially recalculate the PWM period if we've added another pin + _setPWMFreq(_pwmFreq); + return true; } From 524f047b8f5d86e0c999eb35913ebb24ddeb376e Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 6 Jun 2020 10:32:38 -0700 Subject: [PATCH 48/56] 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 +/-. --- cores/esp8266/core_esp8266_waveform.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index b116e0a17b..7379926c59 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -190,6 +190,7 @@ void _setPWMFreq(uint32_t freq) { if (pwmState.cnt) { PWMState p; // The working copy since we can't edit the one in use + p.mask = 0; p.cnt = 0; for (uint32_t i = 0; i < pwmState.cnt; i++) { auto pin = pwmState.pin[i]; From 361d4a2664093926f6a9b35c1c7bf14144c1f074 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 6 Jun 2020 16:01:03 -0700 Subject: [PATCH 49/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 89 +++++++++++++------------ 1 file changed, 46 insertions(+), 43 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 7379926c59..9077fb8f24 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -426,11 +426,12 @@ static inline ICACHE_RAM_ATTR uint32_t GetCycleCountIRQ() { return ccount; } -static inline ICACHE_RAM_ATTR uint32_t min_u32(uint32_t a, uint32_t b) { - if (a < b) { - return a; - } - return b; +// Find the earliest cycle as compared to right now +static inline ICACHE_RAM_ATTR uint32_t earliest(uint32_t a, uint32_t b) { + uint32_t now = GetCycleCountIRQ(); + int32_t da = a - now; + int32_t db = b - now; + return (da < db) ? a : b; } // The SDK and hardware take some time to actually get to our NMI code, so @@ -441,10 +442,10 @@ static inline ICACHE_RAM_ATTR uint32_t min_u32(uint32_t a, uint32_t b) { // so the ESP cycle counter is actually running at a variable speed. // adjust(x) takes care of adjusting a delta clock cycle amount accordingly. #if F_CPU == 80000000 - #define DELTAIRQ (microsecondsToClockCycles(3)) + #define DELTAIRQ (microsecondsToClockCycles(2)) #define adjust(x) ((x) << (turbo ? 1 : 0)) #else - #define DELTAIRQ (microsecondsToClockCycles(2)) + #define DELTAIRQ (microsecondsToClockCycles(1)) #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif @@ -453,7 +454,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { // Flag if the core is at 160 MHz, for use by adjust() bool turbo = (*(uint32_t*)0x3FF00014) & 1 ? true : false; - uint32_t nextEventCycles = microsecondsToClockCycles(MAXIRQUS); + uint32_t nextEventCycle = GetCycleCountIRQ() + microsecondsToClockCycles(MAXIRQUS); uint32_t timeoutCycle = GetCycleCountIRQ() + microsecondsToClockCycles(14); if (wvfState.waveformToEnable || wvfState.waveformToDisable) { @@ -478,40 +479,43 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { bool done = false; if (wvfState.waveformEnabled || pwmState.cnt) { do { - nextEventCycles = microsecondsToClockCycles(MAXIRQUS); + nextEventCycle = GetCycleCountIRQ() + microsecondsToClockCycles(MAXIRQUS); // PWM state machine implementation if (pwmState.cnt) { - int32_t cyclesToGo = pwmState.nextServiceCycle - GetCycleCountIRQ(); - if (cyclesToGo < 0) { - if (pwmState.idx == pwmState.cnt) { // Start of pulses, possibly copy new - if (pwmState.pwmUpdate) { - // Do the memory copy from temp to global and clear mailbox - pwmState = *(PWMState*)pwmState.pwmUpdate; - } - GPOS = pwmState.mask; // Set all active pins high - if (pwmState.mask & (1<<16)) { - GP16O = 1; - } - pwmState.idx = 0; - } else { - do { - // Drop the pin at this edge - if (pwmState.mask & (1<nextServiceCycle = now + nextEdgeCycles; - nextEventCycles = min_u32(nextEventCycles, nextEdgeCycles); wave->lastEdge = now; - } else { - uint32_t deltaCycles = wave->nextServiceCycle - now; - nextEventCycles = min_u32(nextEventCycles, deltaCycles); } + nextEventCycle = earliest(nextEventCycle, wave->nextServiceCycle); } // Exit the loop if we've hit the fixed runtime limit or the next event is known to be after that timeout would occur uint32_t now = GetCycleCountIRQ(); - int32_t cycleDeltaNextEvent = timeoutCycle - (now + nextEventCycles); + int32_t cycleDeltaNextEvent = nextEventCycle - now; int32_t cyclesLeftTimeout = timeoutCycle - now; - done = (cycleDeltaNextEvent < 0) || (cyclesLeftTimeout < 0); + done = (cycleDeltaNextEvent > microsecondsToClockCycles(4)) || (cyclesLeftTimeout < 0); } while (!done); } // if (wvfState.waveformEnabled) if (wvfState.timer1CB) { - nextEventCycles = min_u32(nextEventCycles, wvfState.timer1CB()); + nextEventCycle = earliest(nextEventCycle, GetCycleCountIRQ() + wvfState.timer1CB()); } + int32_t nextEventCycles = nextEventCycle - GetCycleCountIRQ(); + if (nextEventCycles < microsecondsToClockCycles(5)) { nextEventCycles = microsecondsToClockCycles(5); } From 9e487061709991487a3a9624e8e0ad3622642671 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 7 Jun 2020 12:03:02 -0700 Subject: [PATCH 50/56] 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. --- cores/esp8266/core_esp8266_waveform.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 9077fb8f24..5bd66fd074 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -449,6 +449,8 @@ static inline ICACHE_RAM_ATTR uint32_t earliest(uint32_t a, uint32_t b) { #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif +// When the time to the next edge is greater than this, RTI and set another IRQ to minimize CPU usage +#define MINIRQTIME microsecondsToClockCycles(4) static ICACHE_RAM_ATTR void timer1Interrupt() { // Flag if the core is at 160 MHz, for use by adjust() @@ -598,7 +600,7 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { uint32_t now = GetCycleCountIRQ(); int32_t cycleDeltaNextEvent = nextEventCycle - now; int32_t cyclesLeftTimeout = timeoutCycle - now; - done = (cycleDeltaNextEvent > microsecondsToClockCycles(4)) || (cyclesLeftTimeout < 0); + done = (cycleDeltaNextEvent > MINIRQTIME) || (cyclesLeftTimeout < 0); } while (!done); } // if (wvfState.waveformEnabled) @@ -608,8 +610,8 @@ static ICACHE_RAM_ATTR void timer1Interrupt() { int32_t nextEventCycles = nextEventCycle - GetCycleCountIRQ(); - if (nextEventCycles < microsecondsToClockCycles(5)) { - nextEventCycles = microsecondsToClockCycles(5); + if (nextEventCycles < MINIRQTIME) { + nextEventCycles = MINIRQTIME; } nextEventCycles -= DELTAIRQ; From 565f21f6826dafb906e01f8fadd5e3f7b8c59288 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 7 Jun 2020 14:38:52 -0700 Subject: [PATCH 51/56] 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 --- 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 5bd66fd074..800ab74b66 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -442,10 +442,10 @@ static inline ICACHE_RAM_ATTR uint32_t earliest(uint32_t a, uint32_t b) { // so the ESP cycle counter is actually running at a variable speed. // adjust(x) takes care of adjusting a delta clock cycle amount accordingly. #if F_CPU == 80000000 - #define DELTAIRQ (microsecondsToClockCycles(2)) + #define DELTAIRQ (microsecondsToClockCycles(9)/4) #define adjust(x) ((x) << (turbo ? 1 : 0)) #else - #define DELTAIRQ (microsecondsToClockCycles(1)) + #define DELTAIRQ (microsecondsToClockCycles(9)/8) #define adjust(x) ((x) >> (turbo ? 0 : 1)) #endif From 272dc9d2650364a09b3f23e30b260ccbf71cabdf Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sun, 7 Jun 2020 17:21:15 -0700 Subject: [PATCH 52/56] Undo the 160->80 frequency adjust Since the adjustment for when a 160mhz compile is running at 80mhz is giving bad behavior, simply remove it and revert to old behavior. --- cores/esp8266/core_esp8266_waveform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_waveform.cpp b/cores/esp8266/core_esp8266_waveform.cpp index 800ab74b66..d2bc42c57b 100644 --- a/cores/esp8266/core_esp8266_waveform.cpp +++ b/cores/esp8266/core_esp8266_waveform.cpp @@ -446,7 +446,7 @@ static inline ICACHE_RAM_ATTR uint32_t earliest(uint32_t a, uint32_t b) { #define adjust(x) ((x) << (turbo ? 1 : 0)) #else #define DELTAIRQ (microsecondsToClockCycles(9)/8) - #define adjust(x) ((x) >> (turbo ? 0 : 1)) + #define adjust(x) ((x) >> 0) #endif // When the time to the next edge is greater than this, RTI and set another IRQ to minimize CPU usage From e5ba2176d936bda0ce6b71421d3d84baede58d52 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 29 Aug 2020 16:26:19 -0700 Subject: [PATCH 53/56] Update core_esp8266_wiring_pwm.cpp --- cores/esp8266/core_esp8266_wiring_pwm.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index b584f3f84b..06a93f58ef 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -28,7 +28,6 @@ extern "C" { static uint32_t analogMap = 0; static int32_t analogScale = 255; // Match upstream default, breaking change from 2.x.x -static uint16_t analogFreq = 1000; extern void __analogWriteRange(uint32_t range) { if ((range >= 15) && (range <= 65535)) { @@ -76,4 +75,4 @@ extern void analogWriteFreq(uint32_t freq) __attribute__((weak, alias("__analogW extern void analogWriteRange(uint32_t range) __attribute__((weak, alias("__analogWriteRange"))); extern void analogWriteResolution(int res) __attribute__((weak, alias("__analogWriteResolution"))); -}; \ No newline at end of file +}; From f91175420a1a61401975748bc7e4f553803b477e Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Sat, 29 Aug 2020 16:46:23 -0700 Subject: [PATCH 54/56] Update core_esp8266_wiring_pwm.cpp --- cores/esp8266/core_esp8266_wiring_pwm.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cores/esp8266/core_esp8266_wiring_pwm.cpp b/cores/esp8266/core_esp8266_wiring_pwm.cpp index 06a93f58ef..95f4acdaf0 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.cpp +++ b/cores/esp8266/core_esp8266_wiring_pwm.cpp @@ -26,7 +26,6 @@ extern "C" { -static uint32_t analogMap = 0; static int32_t analogScale = 255; // Match upstream default, breaking change from 2.x.x extern void __analogWriteRange(uint32_t range) { From 4cc3d8a15c104d86315dce4f4a673cf012305aa0 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 19 Nov 2020 18:27:32 -0800 Subject: [PATCH 55/56] Fix Servo shutdown changes which caused trouble with Servo::detach() --- libraries/Servo/src/Servo.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libraries/Servo/src/Servo.cpp b/libraries/Servo/src/Servo.cpp index 09d87c0f47..531d8fc875 100644 --- a/libraries/Servo/src/Servo.cpp +++ b/libraries/Servo/src/Servo.cpp @@ -95,7 +95,11 @@ void Servo::detach() { if (_attached) { _servoMap &= ~(1 << _pin); +#ifdef WAVEFORM_LOCKED_PHASE startWaveform(_pin, 0, REFRESH_INTERVAL, 1); +#else + startWaveform(_pin, 1, REFRESH_INTERVAL, REFRESH_INTERVAL); +#endif delay(REFRESH_INTERVAL / 1000); // long enough to complete active period under all circumstances. stopWaveform(_pin); _attached = false; From a35390907cdb0385aaacd7dd3e662aef75cdc8a8 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 19 Nov 2020 19:55:57 -0800 Subject: [PATCH 56/56] Servo shutdown tweak in PWM path --- libraries/Servo/src/Servo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/Servo/src/Servo.cpp b/libraries/Servo/src/Servo.cpp index 531d8fc875..d24ea33799 100644 --- a/libraries/Servo/src/Servo.cpp +++ b/libraries/Servo/src/Servo.cpp @@ -98,7 +98,7 @@ void Servo::detach() #ifdef WAVEFORM_LOCKED_PHASE startWaveform(_pin, 0, REFRESH_INTERVAL, 1); #else - startWaveform(_pin, 1, REFRESH_INTERVAL, REFRESH_INTERVAL); + // TODO - timeHigh == 0 is illegal in _PWM code branch. Do nothing for now. #endif delay(REFRESH_INTERVAL / 1000); // long enough to complete active period under all circumstances. stopWaveform(_pin);