search

SpenceKonde / ATTinyCore

Arduino core for ATtiny 1634, 828, x313, x4, x41, x5, x61, x7 and x8
Other
1.59k stars 308 forks source link

tone() doesn't give the expected output if repeatedly called with the same argument in a tight loop #111

Closed SpenceKonde closed 6 years ago

SpenceKonde commented 7 years ago

Technically, one can argue that this is bad coding to begin with, to spam calls to tone() as fast as the processor can do it - but people do it all the time, so we should handle it.

Full story is here http://forum.arduino.cc/index.php?topic=447949

sleemanj commented 7 years ago

FWIW, I rewrite tone for the x5 as variant code...

https://github.com/sleemanj/ATTinyCore/blob/master/avr/variants/tinyX5/pins_arduino.h#L72 https://github.com/sleemanj/ATTinyCore/blob/master/avr/variants/tinyX5/Tone.cpp

I also implemented tone() for Tiny13, but the implementation is a bit more complex to fit within the flash and ram of that chip by doing more compile-time calculation

https://github.com/sleemanj/ATTinyCore/blob/master/avr/variants/tiny13/pins_arduino.h#L75 https://github.com/sleemanj/ATTinyCore/blob/master/avr/variants/tiny13/Tone.h https://github.com/sleemanj/ATTinyCore/blob/master/avr/variants/tiny13/Tone.cpp

SpenceKonde commented 7 years ago

That implementation doesn't support generating output directly with output compare though - which I think is pretty cool, and was where a fair amount of my effort went. You can generate an absurdly high frequency square wave if you clock timer1 off the PLL

sleemanj commented 7 years ago

Yeah, if that's what you need then yep, I'm only targeting the lowest common denominator - like I say in my comments "let's face it, blurting out Mario Bros is about the most people use tone() for"

But with that said, I would encourage putting tone() in variant code specially crafted for the, err, variant, if you are going to be digging in it anyway and ripping out the #if mess that doesn't apply to that variant.

Working out exactly which bit of code applies to your specific situation when it's a sea of #if statements based around the existence or otherwise of certain registers... well you know, not fun.

SpenceKonde commented 7 years ago

Not sure if yours is a manifestation of the same issue or not, as described in the link at the top of this issue, the problem reported here was that someone put the tone() call in a busy-wait loop and got essentially nothing out.

What microcontroller are you using? There are several almost completely different codepaths for tone() depending on the microcontroller you're using? Can you post code that reproduces the issue? If it's a tiny85, which pin are you using? Are you using pin4 as recommended?

Do you not see this behavior with tone() on an atmega328p-based board? I have not played with tone() at all (I don't even own a peizo buzzer) so I don't really know what the baseline expectation is there. This is something that's going to take some digging to sort out I think, but it sounds like you're encountering a much more serious issue than what initiated this issue (I was not planning to prioritize this - continual calls to tone() within a super-tight while loop is a corner case, not worthy of development effort that could be applied to the unified wire library (#53) or the enhanced fuse configuration options (#105), or the bootloader hex file export issue (#73). But it sounds like there may be a much larger and more important issue with tone here)

I would greatly appreciate your answers to these questions so that I can make the best use of any scraps of time I'm able to find to take a look at this.

SpenceKonde commented 7 years ago

@DaleMitchell - any update? I may have a chance to look at this over the weekend, and it would greatly improve my chances of getting this fixed if you could answer some of the questions I posed above. Thanks!