Prevent divide by zero error causing tone() to crash

[pfeerick]

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.

[codecov-io]

Current coverage is 27.80% (diff: 100%)

Merging #2780 into master will not change coverage

@@             master      #2780   diff @@
==========================================
  Files            20         20          
  Lines          3625       3625          
  Methods         335        335          
  Messages          0          0          
  Branches        656        656          
==========================================
  Hits           1008       1008          
  Misses         2441       2441          
  Partials        176        176          

Powered by Codecov. Last update 0291a6e...d5ccfcf

[igrr]

Wouldn't this cause a 1Hz signal to be generated instead?
I would suggest to move the check for zero frequency to the top of the function, and do a delay(duration) followed by a return in that case.

[pfeerick]

Possibly - I didn't check on the 'scope to see what the output from that pin was with the changes - just that the piezo stopped making (audible?) noise, and the code not crashing. I don't think using a delay() is a viable option though, as tone() is supposed to be a non-blocking function - it merely sets the timer and the ISR does the work of maintaining the PWM signal and disabling it if necessary.

So where does that leave things? Setting a flag in tone if the frequency is 0 and using it on the ISR to leave the pin low until the timer runs out?

[igrr]

Ah, you are right. I've missed the non-blocking nature of tone.

I don't know what is the AVR Arduino behavior for tone with zero frequency — does it generate positive pulse with length equal to duration, or it just leaves the output low? Can anyone check this? If it is the former, then indeed we need to use a timer (and check for possible timer overflows...), if it is the latter, then just bail out of tone without doing anything if frequency is zero.

[pfeerick]

On Arduino (AVR anyway) tone with zero frequency runs with no audible sound, for the specified period. I'll check in a few days what the actual output is on a 'scope. Is used in the toneMelody example, as it allows for a pause without having to do anything fancy, as the melody is stored in an array, and it is just playing each element in sequence, with a second array specifying note lengths.

Is there anything wrong with simply setting the pin low like the ISR does when the timer runs out (at L127) in tone() when a frequency of zero is passed, and adding a check for a raised flag in the ISR (at L119)that simply stops the pin from toggling if the flag is raised? That to me seems to allow the ISR to manage things as it should, and give the intended results. Although since the code is written with multiple tone pins as a possibility, it will probably have to be an boolean? array like tone_zero[] or something to match the rest of the code.

[pfeerick]

Actually, on second thoughts... and after some checks against a scope with an Arduino Nano + reading some more documentation on tone, it can't go below 31 Hz anyway, and simply outputs nothing if you try. So probably just a check for a 0 frequency and an redirect to noTone instead as that will disable the timer again, and set the pin low / off.

[pfeerick]

It would appear the ESP8266 is better at tone() than the Arduino in some instances... the arduino implementation states it is limited to a minimum of 31-41 hertz (depending on board), whereas the ESP8266 can go down as far as at least 5hz. The latest commit appears to produce the expected results, and a frequency of 0 does indeed result in no crashes and no output.