From be7a732b9d92f4a52c1df699429b81b638661a9c Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 2 Jul 2018 11:02:49 -0600 Subject: [PATCH] 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); --- cores/esp8266/Arduino.h | 1 + cores/esp8266/Tone.cpp | 7 +++++++ cores/esp8266/core_esp8266_waveform.c | 17 ++++++++++++----- cores/esp8266/core_esp8266_wiring_pwm.c | 3 ++- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/cores/esp8266/Arduino.h b/cores/esp8266/Arduino.h index b274f1712..c1e185414 100644 --- a/cores/esp8266/Arduino.h +++ b/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); diff --git a/cores/esp8266/Tone.cpp b/cores/esp8266/Tone.cpp index a3345bb61..35a5a4151 100644 --- a/cores/esp8266/Tone.cpp +++ b/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; diff --git a/cores/esp8266/core_esp8266_waveform.c b/cores/esp8266/core_esp8266_waveform.c index 0454c768b..306fd3d45 100644 --- a/cores/esp8266/core_esp8266_waveform.c +++ b/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,6 +195,10 @@ 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; } @@ -200,6 +206,8 @@ int startWaveform(uint8_t pin, uint32_t timeHighUS, uint32_t timeLowUS, uint32_t 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<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; } diff --git a/cores/esp8266/core_esp8266_wiring_pwm.c b/cores/esp8266/core_esp8266_wiring_pwm.c index 153f30ee3..22ff2663b 100644 --- a/cores/esp8266/core_esp8266_wiring_pwm.c +++ b/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);