search

adafruit / Adafruit_CircuitPython_MagTag

Helper library for the Adafruit MagTag
MIT License
28 stars 24 forks source link

allow tone frequency of 0 #68

Closed RufusVS closed 2 years ago

RufusVS commented 3 years ago

I left an issue in the issues section, but decided I could simply make a pull request myself. (I'm pretty new to the process). This fix allows a specification of a zero frequency to the play_tone function. Forum: https://forums.adafruit.com/viewtopic.php?f=60&t=182821 Issue: https://github.com/adafruit/Adafruit_CircuitPython_MagTag/issues/67

TheKitty commented 3 years ago

Why is a tone value of zero desirable? If the user believes a tone might be zero, they could handle.

RufusVS commented 3 years ago

Why is a tone value of zero desirable? If the user believes a tone might be zero, they could handle.

I probably should have quoted my issue or forum post which explained it much better. Forum: https://forums.adafruit.com/viewtopic.php?f=60&t=182821 Issue: https://github.com/adafruit/Adafruit_CircuitPython_MagTag/issues/67

TL;DR Music often requires rests (periods of silence). It makes more sense to have it use the same duration intervals that are used for the other notes than handling it as a special case and needing the import of a timer module. Specifying zero allows for timed periods of silence (rests). (It also would make it compatible with most other implementations, Macropad, Arduino, etc.)

makermelissa commented 2 years ago

Have you tested that this works on hardware? If I recall correctly, I think it was causing errors and that's why we limited it to frequency above 0, but it's possible things have changed since then. The play_tone() function is really just a wrapper for the simpleio tone function: https://github.com/adafruit/Adafruit_CircuitPython_SimpleIO/blob/main/simpleio.py#L37 and I'm not sure how it would handle a 0 hertz frequency.