Skip to content

play_tone function should not raise Value error on frequency of 0 #67

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

Closed
RufusVS opened this issue Sep 4, 2021 · 5 comments
Closed
Assignees

Comments

@RufusVS
Copy link
Contributor

RufusVS commented Sep 4, 2021

Currently in the library, if a zero frequency is specified to play_tone, a ValueError is thrown. In most other similar implementations, a value of 0 simply represents silence, or rest, for the duration specified. There is no reason for this implementation to be different. Also, this change is not likely to affect any user's existing code.

Current Code:

def play_tone(frequency, duration):
        """..."""
        if frequency <= 0:
            raise ValueError("The frequency has to be greater than 0.")

Suggested change:

def play_tone(frequency, duration):
        """..."""
        if frequency < 0:
            raise ValueError("Negative frequencies are not allowed.")
@tannewt
Copy link
Member

tannewt commented Sep 7, 2021

@kattni What do you think the right way of doing rests is?

@tannewt
Copy link
Member

tannewt commented Sep 7, 2021

play_tone is blocking so it could just be a time.sleep(duration) instead.

@RufusVS
Copy link
Contributor Author

RufusVS commented Sep 8, 2021

Blocking vs. non-blocking is a wrinkle, but here's my original post on the subject:
https://forums.adafruit.com/posting.php?mode=quote&f=60&p=888324

The gist being:

        for freq,dur in melody:
                peripherals.play_tone(freq,dur)

Is more intuitive and less clunky than:

        import time
        for freq,dur in melody:
            if freq:
                peripherals.play_tone(freq,dur)
            else:
                time.sleep(dur)

(Also requiring the need to import time, which might not have been necessary)
The simple version seems to me more compatible with MacroPad and other (and similar to Arduino code, as well).

@tannewt
Copy link
Member

tannewt commented Sep 8, 2021

I agree that in this case it is simpler. However, I'm not sure that it's better in the case where frequency is the result of a computation that shouldn't return zero but does due to an error. Input validation can make it easier to find bugs. I'm not sure allowing zero frequency is better for all cases.

@makermelissa
Copy link
Collaborator

Fixed via #68.

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