Waveform: int/uint8_t inconsistency and implied stop PWM

cores/esp8266/Tone.cpp

@@ -30,11 +30,6 @@ 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,

cores/esp8266/core_esp8266_waveform.h

@@ -110,11 +110,10 @@ int stopWaveform(uint8_t pin);
 // Make sure the CB function has the IRAM_ATTR decorator.
 void setTimer1Callback(uint32_t (*fn)());
 
-
 // Internal-only calls, not for applications
-extern void _setPWMFreq(uint32_t freq);
-extern bool _stopPWM(uint8_t pin);
-extern bool _setPWM(int pin, uint32_t val, uint32_t range);
+void _setPWMFreq(uint32_t freq);
+bool _stopPWM(uint8_t pin);
+bool _setPWM(uint8_t pin, uint32_t val, uint32_t range);
 
 #ifdef __cplusplus
 }

cores/esp8266/core_esp8266_waveform_phase.cpp

@@ -55,8 +55,8 @@ extern "C" void enablePhaseLockedWaveform (void)
 
 // No-op calls to override the PWM implementation
 extern "C" void _setPWMFreq_weak(uint32_t freq) { (void) freq; }
-extern "C" IRAM_ATTR bool _stopPWM_weak(int pin) { (void) pin; return false; }
-extern "C" bool _setPWM_weak(int pin, uint32_t val, uint32_t range) { (void) pin; (void) val; (void) range; return false; }
+extern "C" IRAM_ATTR bool _stopPWM_weak(uint8_t pin) { (void) pin; return false; }
+extern "C" bool _setPWM_weak(uint8_t pin, uint32_t val, uint32_t range) { (void) pin; (void) val; (void) range; return false; }
 
 
 // Timer is 80MHz fixed. 160MHz CPU frequency need scaling.
@@ -78,7 +78,7 @@ constexpr int32_t DELTAIRQCCYS = ISCPUFREQ160MHZ ?
 enum class WaveformMode : uint8_t {INFINITE = 0, EXPIRES = 1, UPDATEEXPIRY = 2, INIT = 3};
 
 // Waveform generator can create tones, PWM, and servos
-typedef struct {
+struct Waveform {
   uint32_t nextPeriodCcy; // ESP clock cycle when a period begins. If WaveformMode::INIT, temporarily holds positive phase offset ccy count
   uint32_t endDutyCcy;    // ESP clock cycle when going from duty to off
   int32_t dutyCcys;       // Set next off cycle at low->high to maintain phase
@@ -88,7 +88,7 @@ typedef struct {
   WaveformMode mode;
   int8_t alignPhase;      // < 0 no phase alignment, otherwise starts waveform in relative phase offset to given pin
   bool autoPwm;           // perform PWM duty to idle cycle ratio correction under high load at the expense of precise timings
-} Waveform;
+};
 
 namespace {
 
@@ -122,10 +122,11 @@ static void initTimer() {
   ETS_FRC_TIMER1_NMI_INTR_ATTACH(timer1Interrupt);
   timer1_enable(TIM_DIV1, TIM_EDGE, TIM_SINGLE);
   waveform.timer1Running = true;
+  waveform.nextEventCcy = ESP.getCycleCount() + IRQLATENCYCCYS;
   timer1_write(IRQLATENCYCCYS); // Cause an interrupt post-haste
 }
 
-static void IRAM_ATTR deinitTimer() {
+static IRAM_ATTR void deinitTimer() {
   ETS_FRC_TIMER1_NMI_INTR_ATTACH(NULL);
   timer1_disable();
   timer1_isr_init();
@@ -194,10 +195,7 @@ int startWaveformClockCycles_weak(uint8_t pin, uint32_t highCcys, uint32_t lowCc
     if (!waveform.timer1Running) {
       initTimer();
     }
-    else if (T1V > IRQLATENCYCCYS) {
-      // Must not interfere if Timer is due shortly
-      timer1_write(IRQLATENCYCCYS);
-    }
+    // The ISR pulls updates on the next waveform interval
   }
   else {
     wave.mode = WaveformMode::INFINITE; // turn off possible expiry to make update atomic from NMI
@@ -232,6 +230,7 @@ IRAM_ATTR int stopWaveform_weak(uint8_t pin) {
     std::atomic_thread_fence(std::memory_order_release);
     // Must not interfere if Timer is due shortly
     if (T1V > IRQLATENCYCCYS) {
+      waveform.nextEventCcy = ESP.getCycleCount() + IRQLATENCYCCYS;
       timer1_write(IRQLATENCYCCYS);
     }
     while (waveform.toDisableBits) {
@@ -267,7 +266,7 @@ static IRAM_ATTR void timer1Interrupt() {
   const bool isCPU2X = CPU2X & 1;
   if ((waveform.toSetBits && !(waveform.enabled & waveform.toSetBits)) || waveform.toDisableBits) {
     // Handle enable/disable requests from main app.
-    waveform.enabled = (waveform.enabled & ~waveform.toDisableBits) | waveform.toSetBits; // Set the requested waveforms on/off
+    waveform.enabled = (waveform.enabled | waveform.toSetBits) & ~waveform.toDisableBits; // Set the requested waveforms on/off
     // 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)
     waveform.toDisableBits = 0;
   }
@@ -308,7 +307,7 @@ static IRAM_ATTR void timer1Interrupt() {
   uint32_t now = ESP.getCycleCount();
   uint32_t isrNextEventCcy = now;
   while (busyPins) {
-    if (static_cast<int32_t>(isrNextEventCcy - now) > IRQLATENCYCCYS) {
+    if (static_cast<int32_t>(isrNextEventCcy - now) > IRQLATENCYCCYS + DELTAIRQCCYS) {
       waveform.nextEventCcy = isrNextEventCcy;
       break;
     }

cores/esp8266/core_esp8266_waveform_pwm.cpp

@@ -128,21 +128,20 @@ static IRAM_ATTR void forceTimerInterrupt() {
 constexpr int maxPWMs = 8;
 
 // PWM machine state
-typedef struct PWMState {
+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 uint32_t _pwmFreq = 1000;
 static uint32_t _pwmPeriod = microsecondsToClockCycles(1000000UL) / _pwmFreq;
 
-
 // If there are no more scheduled activities, shut down Timer 1.
 // Otherwise, do nothing.
 static IRAM_ATTR void disableIdleTimer() {
@@ -158,9 +157,12 @@ static IRAM_ATTR void disableIdleTimer() {
 // Wait for mailbox to be emptied (either busy or delay() as needed)
 static IRAM_ATTR void _notifyPWM(PWMState *p, bool idle) {
   p->pwmUpdate = nullptr;
+  MEMBARRIER();
   pwmState.pwmUpdate = p;
   MEMBARRIER();
-  forceTimerInterrupt();
+  if (idle) {
+    forceTimerInterrupt();
+  }
   while (pwmState.pwmUpdate) {
     if (idle) {
       esp_yield();
@@ -169,8 +171,7 @@ static IRAM_ATTR void _notifyPWM(PWMState *p, bool idle) {
   }
 }
 
-static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range);
-
+static void _addPWMtoList(PWMState &p, uint8_t pin, uint32_t val, uint32_t range);
 
 // Called when analogWriteFreq() changed to update the PWM total period
 extern void _setPWMFreq_weak(uint32_t freq) __attribute__((weak)); 
@@ -216,7 +217,7 @@ void _setPWMFreq(uint32_t freq) {
 
 // Helper routine to remove an entry from the state machine
 // and clean up any marked-off entries
-static void _cleanAndRemovePWM(PWMState *p, int pin) {
+static void _cleanAndRemovePWM(PWMState *p, uint8_t pin) {
   uint32_t leftover = 0;
   uint32_t in, out;
   for (in = 0, out = 0; in < p->cnt; in++) {
@@ -243,8 +244,7 @@ IRAM_ATTR bool _stopPWM_weak(uint8_t pin) {
     return false; // Pin not actually active
   }
 
-  PWMState p;  // The working copy since we can't edit the one in use
-  p = pwmState;
+  PWMState p = pwmState; // The working copy since we can't edit the one in use
 
   // In _stopPWM we just clear the mask but keep everything else
   // untouched to save IRAM.  The main startPWM will handle cleanup.
@@ -265,7 +265,7 @@ IRAM_ATTR bool _stopPWM(uint8_t pin) {
   return _stopPWM_bound(pin);
 }
 
-static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range) {
+static void _addPWMtoList(PWMState& p, uint8_t 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
@@ -279,17 +279,19 @@ static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range) {
   // Clip to sane values in the case we go from OK to not-OK when adjusting frequencies
   if (cc == 0) {
     cc = 1;
-  } else if (cc >= _pwmPeriod) {
+  }
+  else if (cc >= _pwmPeriod) {
     cc = _pwmPeriod - 1;
   }
 
   if (p.cnt == 0) {
     // Starting up from scratch, special case 1st element and PWM period
     p.pin[0] = pin;
     p.delta[0] = cc;
-   // Final pin is never used: p.pin[1] = 0xff;
+    // Final pin is never used: p.pin[1] = 0xff;
     p.delta[1] = _pwmPeriod - cc;
-  } else {
+  }
+  else {
     uint32_t ttl = 0;
     uint32_t i;
     // Skip along until we're at the spot to insert
@@ -311,9 +313,12 @@ static void _addPWMtoList(PWMState &p, int pin, uint32_t val, uint32_t range) {
 }
 
 // Called by analogWrite(1...99%) to set the PWM duty in clock cycles
-extern bool _setPWM_weak(int pin, uint32_t val, uint32_t range) __attribute__((weak));
-bool _setPWM_weak(int pin, uint32_t val, uint32_t range) {
-  stopWaveform(pin);
+extern bool _setPWM_weak(uint8_t pin, uint32_t val, uint32_t range) __attribute__((weak));
+bool _setPWM_weak(uint8_t pin, uint32_t val, uint32_t range) {
+  const uint32_t mask = 1<<pin;
+  if (wvfState.waveformEnabled & mask) {
+    stopWaveform(pin);
+  }
   PWMState p;  // Working copy
   p = pwmState;
   // Get rid of any entries for this pin
@@ -342,8 +347,8 @@ bool _setPWM_weak(int pin, uint32_t val, uint32_t range) {
 
   return true;
 }
-static bool _setPWM_bound(int pin, uint32_t val, uint32_t range) __attribute__((weakref("_setPWM_weak")));
-bool _setPWM(int pin, uint32_t val, uint32_t range) {
+static bool _setPWM_bound(uint8_t pin, uint32_t val, uint32_t range) __attribute__((weakref("_setPWM_weak")));
+bool _setPWM(uint8_t pin, uint32_t val, uint32_t range) {
   return _setPWM_bound(pin, val, range);
 }
 
@@ -368,7 +373,7 @@ int startWaveformClockCycles_weak(uint8_t pin, uint32_t timeHighCycles, uint32_t
 
   _stopPWM(pin); // Make sure there's no PWM live here
 
-  uint32_t mask = 1<<pin;
+  const uint32_t mask = 1<<pin;
   MEMBARRIER();
   if (wvfState.waveformEnabled & mask) {
     // Make sure no waveform changes are waiting to be applied
@@ -436,9 +441,12 @@ IRAM_ATTR int stopWaveform_weak(uint8_t pin) {
   if (!timerRunning) {
     return false;
   }
+
+  _stopPWM(pin); // Make sure there's no PWM live here
+
   // If user sends in a pin >16 but <32, this will always point to a 0 bit
   // If they send >=32, then the shift will result in 0 and it will also return false
-  uint32_t mask = 1<<pin;
+  const uint32_t mask = 1<<pin;
   if (wvfState.waveformEnabled & mask) {
     wvfState.waveformToDisable = mask;
     // Cancel any pending updates for this waveform, too.
@@ -530,43 +538,43 @@ static IRAM_ATTR void timer1Interrupt() {
 
       // PWM state machine implementation
       if (pwmState.cnt) {
-        int32_t cyclesToGo;
+        int32_t pwmCyclesToGo;
         do {
-            cyclesToGo = pwmState.nextServiceCycle - GetCycleCountIRQ();
-            if (cyclesToGo < 0) {
+            pwmCyclesToGo = pwmState.nextServiceCycle - GetCycleCountIRQ();
+            if (pwmCyclesToGo <= 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;
+                    pwmState = *pwmState.pwmUpdate;
                   }
                   GPOS = pwmState.mask; // Set all active pins high
                   if (pwmState.mask & (1<<16)) {
                     GP16O = 1;
                   }
                   pwmState.idx = 0;
-                } else {
+                }
+                else {
                   do {
                     // Drop the pin at this edge
-                    if (pwmState.mask & (1<<pwmState.pin[pwmState.idx])) {
-                      GPOC = 1<<pwmState.pin[pwmState.idx];
-                      if (pwmState.pin[pwmState.idx] == 16) {
-                        GP16O = 0;
-                      }
+                    GPOC = 1<<pwmState.pin[pwmState.idx];
+                    if (pwmState.pin[pwmState.idx] == 16) {
+                      GP16O = 0;
                     }
                     pwmState.idx++;
                     // Any other pins at this same PWM value will have delta==0, drop them too.
                   } while (pwmState.delta[pwmState.idx] == 0);
                 }
                 // Preserve duty cycle over PWM period by using now+xxx instead of += delta
-                cyclesToGo = adjust(pwmState.delta[pwmState.idx]);
-                pwmState.nextServiceCycle = GetCycleCountIRQ() + cyclesToGo;
+                pwmCyclesToGo = adjust(pwmState.delta[pwmState.idx]);
+                pwmState.nextServiceCycle = GetCycleCountIRQ() + pwmCyclesToGo;
             }
             nextEventCycle = earliest(nextEventCycle, pwmState.nextServiceCycle);
-        } while (pwmState.cnt && (cyclesToGo < 100));
+            // PWM can starve the generic waveform generator if pwmCyclesToGo remains below 100
+        } while (pwmState.cnt && pwmCyclesToGo < 100);
       }
 
       for (auto i = wvfState.startPin; i <= wvfState.endPin; i++) {
-        uint32_t mask = 1<<i;
+        const uint32_t mask = 1<<i;
 
         // If it's not on, ignore!
         if (!(wvfState.waveformEnabled & mask)) {

cores/esp8266/core_esp8266_wiring_digital.cpp

@@ -82,8 +82,7 @@ extern void __pinMode(uint8_t pin, uint8_t mode) {
 }
 
 extern void IRAM_ATTR __digitalWrite(uint8_t pin, uint8_t val) {
-  stopWaveform(pin); // Disable any Tone or startWaveform on this pin
-  _stopPWM(pin);     // and any analogWrites (PWM)
+  stopWaveform(pin); // Disable any Tone, startWaveform, or analogWrite on this pin
   if(pin < 16){
     if(val) GPOS = (1 << pin);
     else GPOC = (1 << pin);

cores/esp8266/core_esp8266_wiring_pwm.cpp

@@ -44,7 +44,14 @@ extern void __analogWriteFreq(uint32_t freq) {
 }
 
 extern void __analogWrite(uint8_t pin, int val) {
-  analogWriteMode(pin, val, false);
+  if (pin > 16) {
+    return;
+  }
+  bool openDrain = false;
+  if (analogMap & 1UL << pin) {
+    openDrain = GPC(pin) & (1 << GPCD);
+  }
+  analogWriteMode(pin, val, openDrain);
 }
 
 extern void __analogWriteMode(uint8_t pin, int val, bool openDrain) {
@@ -59,27 +66,37 @@ extern void __analogWriteMode(uint8_t pin, int val, bool openDrain) {
   }
 
   if (analogMap & 1UL << pin) {
-  // Per the Arduino docs at https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite/
-  // val: the duty cycle: between 0 (always off) and 255 (always on).
-  // So if val = 0 we have digitalWrite(LOW), if we have val==range we have digitalWrite(HIGH)
-
     analogMap &= ~(1 << pin);
+    const bool isOpenDrain = GPC(pin) & (1 << GPCD);
+    if (isOpenDrain != openDrain) {
+      GPC(pin) ^= (1 << GPCD);
+    }
   }
   else {
-    if(openDrain) {
-      pinMode(pin, OUTPUT_OPEN_DRAIN);
-    } else {
-      pinMode(pin, OUTPUT);
-    }
+    pinMode(pin, openDrain ? OUTPUT_OPEN_DRAIN : OUTPUT);
   }
   uint32_t high = (analogPeriod * val) / analogScale;
   uint32_t low = analogPeriod - high;
   // 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)
   int phaseReference = __builtin_ffs(analogMap) - 1;
+  // Per the Arduino docs at https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite/
+  // val: the duty cycle: between 0 (always off) and 255 (always on).
+  // So if val = 0 we have digitalWrite(LOW), if we have val==range we have digitalWrite(HIGH)
   if (_setPWM(pin, val, analogScale)) {
-    analogMap |= (1 << pin);
-  } else if (startWaveformClockCycles(pin, high, low, 0, phaseReference, 0, true)) {
-    analogMap |= (1 << pin);
+    if (val > 0 && val < analogScale) {
+      analogMap |= (1 << pin);
+    }
+  } else {
+    const bool detach = (val == 0 || val == analogScale);
+    // To go steady LOW or HIGH, let the waveform run into next duty cycle, if any. Then stop.
+    if (startWaveformClockCycles(pin, high, low, static_cast<uint32_t>(detach), phaseReference, 0, true)) {
+      if (detach) {
+        delay((1000 + analogFreq) / analogFreq);
+        stopWaveform(pin);
+      } else {
+        analogMap |= (1 << pin);
+      }
+    }
   }
 }
 

libraries/Servo/src/Servo.cpp

@@ -116,8 +116,7 @@ void Servo::writeMicroseconds(int value)
     _servoMap &= ~(1 << _pin);
     // 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)
     int phaseReference = __builtin_ffs(_servoMap) - 1;
-    if (startWaveform(_pin, _valueUs, REFRESH_INTERVAL - _valueUs, 0, phaseReference))
-    {
+    if (startWaveform(_pin, _valueUs, REFRESH_INTERVAL - _valueUs, 0, phaseReference)) {
       _servoMap |= (1 << _pin);
     }
   }