Compatibility and IRQ fixed for waveform/tone/pwm (#4872)

* Compatibility and IRQ fixed for waveform/tone/pwm

Fix a compiler ambiguity introduced with a floating point frequency option
for tone().  Thanks to @Rob58329 for discovering this and proposing the
fix.

Match original analogWrite behavior by going from 0...1023 (PWMRANGE) and
not 0...1024, and also explicitly set the analogWrite pin to an OUTPUT.
Thanks to @JAndrassy for finding this.

Fixes #4380 discovered by @cranphin where interrupts were disabled on a
stopWaveform().  Remove that completely and bracket the update of non-atomic
fields in the structure with disable/enable IRQs for safety.

* Fix tone(int,int,int) infinite loop

Explicitly cast the frequency, when passed in as an int, to an
unsigned int.  Verified with snippet:
  tone(D1, (int)1000, 500);
  tone(D1, (unsigned int)1000, 500);
  tone(D1, 1000.0, 500);
  tone(D1, (int)1000);
  tone(D1, (unsigned int)1000);
  tone(D1, 1000.0);

Author: earlephilhower

cores/esp8266/Arduino.h

@@ -279,6 +279,7 @@ unsigned long pulseIn(uint8_t pin, uint8_t state, unsigned long timeout = 100000
 unsigned long pulseInLong(uint8_t pin, uint8_t state, unsigned long timeout = 1000000L);
 
 void tone(uint8_t _pin, unsigned int frequency, unsigned long duration = 0);
+void tone(uint8_t _pin, int frequency, unsigned long duration = 0);
 void tone(uint8_t _pin, double frequency, unsigned long duration = 0);
 void noTone(uint8_t _pin);
 

cores/esp8266/Tone.cpp

@@ -70,6 +70,13 @@ void tone(uint8_t _pin, double frequency, unsigned long duration) {
 }
 
 
+// Fix ambiguous tone() binding when adding in a duration
+void tone(uint8_t _pin, int frequency, unsigned long duration) {
+  // Call the unsigned int version of the function explicitly
+  tone(_pin, (unsigned int)frequency, duration);
+}
+
+
 void noTone(uint8_t _pin) {
   if (_pin > 16) {
     return;

cores/esp8266/core_esp8266_waveform.c

@@ -43,10 +43,6 @@
 // Need speed, not size, here
 #pragma GCC optimize ("O3")
 
-// Map the IRQ stuff to standard terminology
-#define cli() ets_intr_lock()
-#define sei() ets_intr_unlock()
-
 // Maximum delay between IRQs
 #define MAXIRQUS (10000)
 
@@ -180,6 +176,12 @@ int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t
   if (!wave) {
     return false;
   }
+
+  // To safely update the packed bitfields we need to stop interrupts while setting them as we could
+  // get an IRQ in the middle of a multi-instruction mask-and-set required to change them which would
+  // then cause an IRQ update of these values (.enabled only, for now) to be lost.
+  ets_intr_lock();
+
   wave->nextTimeHighCycles = MicrosecondsToCycles(timeHighUS) - 70;  // Take out some time for IRQ codepath
   wave->nextTimeLowCycles = MicrosecondsToCycles(timeLowUS) - 70;  // Take out some time for IRQ codepath
   wave->timeLeftCycles = MicrosecondsToCycles(runTimeUS);
@@ -193,13 +195,19 @@ int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t
     }
     ReloadTimer(MicrosecondsToCycles(1)); // Cause an interrupt post-haste
   }
+
+  // Re-enable interrupts here since we're done with the update
+  ets_intr_unlock();
+
   return true;
 }
 
 // Stops a waveform on a pin
 int stopWaveform(uint8_t pin) {
   for (size_t i = 0; i < countof(waveform); i++) {
     if (((pin == 16) && waveform[i].gpio16Mask) || ((pin != 16) && (waveform[i].gpioMask == 1<<pin))) {
+      // Note that there is no interrupt unsafety here.  The IRQ can only ever change .enabled from 1->0
+      // We're also doing that, so even if an IRQ occurred it would still stay as 0.
       waveform[i].enabled = 0;
       int cnt = timer1CB?1:0;
       for (size_t i = 0; i < countof(waveform); i++) {
@@ -211,7 +219,6 @@ int stopWaveform(uint8_t pin) {
       return true;
     }
   }
-  cli();
   return false;
 }
 

cores/esp8266/core_esp8266_wiring_pwm.c

@@ -26,7 +26,7 @@
 
 
 static uint32_t analogMap = 0;
-static int32_t analogScale = 255;
+static int32_t analogScale = PWMRANGE;
 static uint16_t analogFreq = 1000;
 
 extern void __analogWriteRange(uint32_t range) {
@@ -59,6 +59,7 @@ extern void __analogWrite(uint8_t pin, int val) {
   analogMap &= ~(1 << pin);
   uint32_t high = (analogPeriod * val) / analogScale;
   uint32_t low = analogPeriod - high;
+  pinMode(pin, OUTPUT);
   if (low == 0) {
     stopWaveform(pin);
     digitalWrite(pin, HIGH);