Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tone causes crash with frequency = 0 #2491

Closed
ghost opened this issue Sep 7, 2016 · 3 comments
Closed

Tone causes crash with frequency = 0 #2491

ghost opened this issue Sep 7, 2016 · 3 comments

Comments

@ghost
Copy link

ghost commented Sep 7, 2016

It's not uncommon to use tone with frequency = 0 to indicate 'silence/no note'.
Example: https://github.com/arduino/Arduino/blob/master/build/shared/examples/02.Digital/toneMelody/toneMelody.ino
This crashes right now, I expect because of a division in zero in: https://github.com/esp8266/Arduino/blob/master/cores/esp8266/Tone.cpp

timer1_write((clockCyclesPerMicrosecond() * 500000) / frequency);
Admittedly the behavior on Arduino for frequency = 0 doesn't seem to be documented, and I expect on some other cores it would also crash or do weird things..

@Razpudding
Copy link

I second this issue. I worked around it by checking if frequency is '0' and if so setting it to '1'. If someone has an actual solution I'd love to hear it.

@pfeerick
Copy link
Contributor

pfeerick commented Dec 20, 2016

Over on the Oak we've also just experienced this issue as well, as it is essentially based on the ESP8266 core. In the issue I opened over there (digistump/OakCore#75) to document that it's a known problem,

Upon having a quick look through the original Arduino implementation, it appears it should suffer from the same issue, as it would also divide by zero. However, it appears that "the AVR doesn't have a divide instruction, so can't itself know that a divide-by-zero has occurred", and basically fakes divides with shifts, thus doesn't actually realise something is very, very wrong!?

I proposed the following solution to work around the issue, but only at the user sketch level... it doesn't really fix the problem as it results from the implementation, but it at least prevents the sketch from crashing...

Basically, Instead of

tone(3, melody[thisNote], noteDuration);

to use

if (melody[thisNote] == 0)
{
  noTone(3);
  delay(noteDuration);
}
else
{
  tone(3, melody[thisNote], noteDuration);
}

pfeerick added a commit to pfeerick/ESP8266-Arduino that referenced this issue Dec 20, 2016
As per the issue at esp8266#2491, there is a divide by error issue resulting from the specification of 0 as the frequency. This does not appear to affect the AVR implementation, but it crashes on ESP8266s. I have merely removed the division if the frequency is zero, which appears to be giving the expected results (no tone), without any code crashes. 

To test, simply load the toneMelody sketch included with the Arduino IDE (Examples -> 02. Digital -> toneMelody) and change the piezo to something else if you need to. On the Witty module used to test this, I could also tell by the wifi led blinking every time the code crashed as the ESP8266 immediately rebooted.
igrr pushed a commit that referenced this issue Jan 17, 2017
* Prevent divide by zero error causing code to crash

As per the issue at #2491, there is a divide by error issue resulting from the specification of 0 as the frequency. This does not appear to affect the AVR implementation, but it crashes on ESP8266s. I have merely removed the division if the frequency is zero, which appears to be giving the expected results (no tone), without any code crashes. 

To test, simply load the toneMelody sketch included with the Arduino IDE (Examples -> 02. Digital -> toneMelody) and change the piezo to something else if you need to. On the Witty module used to test this, I could also tell by the wifi led blinking every time the code crashed as the ESP8266 immediately rebooted.

* Use noTone when frequency is zero

When a frequency of zero is given to tone(), instead call noTone() and exit. Placed after some of the initialisation stuff to ensure the pin is mapped as a output, etc. Tested as functional against a Node MCU 1.0 board and the toneMelody example sketch, using GPIO5 (pin D1).

* Errant tab in formatting

* Rest of tabs that crept in from web editor

Defaulted to tabs and 8 indent :sigh:
@devyte
Copy link
Collaborator

devyte commented Oct 13, 2017

Code is already merged, closing.

@devyte devyte closed this as completed Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants