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

allow tone frequency of 0 #68

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Conversation

RufusVS
Copy link
Contributor

@RufusVS RufusVS commented Sep 6, 2021

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
Copy link

TheKitty commented Sep 6, 2021

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

@RufusVS
Copy link
Contributor Author

RufusVS commented Sep 6, 2021

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
Copy link
Collaborator

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.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Tested this successfully on MagTag 7.1.0 beta 1 with code like this:

import time
from adafruit_magtag.magtag import MagTag

magtag = MagTag()

song = [(400, 0.35), (0, 0.5), (467, 0.35), (800, 0.25)]

for note in song:
    magtag.peripherals.play_tone(note[0], note[1])
    time.sleep(note[1])

I confirmed there does not seem to be any issue with passing 0 to simpleio and it does result in silent playback.

@FoamyGuy FoamyGuy merged commit a3e8d28 into adafruit:main Dec 11, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Dec 11, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM6DS to 4.2.2 from 4.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM6DS#46 from tekktrik/feature/add-typing
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_MagTag to 2.1.5 from 2.1.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_MagTag#68 from RufusVS/tone_frequency_0_fix
  > Merge pull request adafruit/Adafruit_CircuitPython_MagTag#76 from FoamyGuy/power_before_neopixel_init

Updating https://github.com/adafruit/Adafruit_CircuitPython_PYOA to 2.5.0 from 2.4.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_PYOA#25 from lesamouraipourpre/remove-old-refresh
  > update rtd py version
@RufusVS RufusVS deleted the tone_frequency_0_fix branch February 12, 2022 20:46
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

Successfully merging this pull request may close these issues.

4 participants