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.

Sign up for GitHub

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

Jump to bottom

Fix Piezo.prototype.note (bad frequencies) #1323

Closed
lyzadanger opened this issue Apr 13, 2017 · 3 comments
Closed

Fix Piezo.prototype.note (bad frequencies) #1323

lyzadanger opened this issue Apr 13, 2017 · 3 comments
Assignees
@lyzadanger
Labels
Class:Piezo

Comments

@lyzadanger
Copy link
Collaborator

Per code-dot-org/code-dot-org#14332

Override johnny-five's Piezo.prototype.note because it's playing the wrong frequencies, at least in Maker Toolkit. I think this is a bug in their component and I'm working on getting it fixed upstream. In the meantime it's fairly easy to patch in a fix in our wrapper.
@lyzadanger lyzadanger self-assigned this Apr 13, 2017
@lyzadanger
Copy link
Collaborator Author

I plan to work on this next week (week of Apr. 17). Someone else can grab this if that's not soon enough!

@islemaster islemaster mentioned this issue Apr 13, 2017
Closed
islemaster added a commit to code-dot-org/johnny-five that referenced this issue Apr 18, 2017
@islemaster
Fix Piezo.prototype.note()
4651e95 
This commit applies upstream fix rwaldron#1324 for issue rwaldron#1323 to code-dot-org/johnny-five early, even though we're behind.  This should merge trivially when we catch up.

The `note()` method was passing a frequency (Hz) to the `tone()` method, which is expecting to receive a duty cycle (μs), leading to the wrong note being played.  The solution is for `note()` to call `frequency()` instead, which will do the appropriate conversion.

Since it took me a while to understand the relationships between `tone`, `frequency` and `note` I've added some documentation to those methods to clear things up for the next person.  I've also made some unit tests more aggressive about checking that values are converted appropriately as they're passed around, and commented note-frequency-duty equivalencies to help the test read nicer.

Includes squashed fixups:

- Fix lint error.

- Address review feedback:
  - Test that `note()` is case-insensitive
  - Better description of default octave
  - One-line Piezo.prototype.note
  - s/frequncy/frequency
  - Note tests demonstrate the difference between note letter and octave number better
@islemaster islemaster mentioned this issue Apr 18, 2017
Merged
@islemaster
Copy link
Contributor

Fixed in #1328, released in v0.10.10.

@dtex
Copy link
Collaborator

dtex commented Jan 1, 2018

Looks like this issue should have been closed a while back. Re-open if I'm missing something. Thanks everyone.

@dtex dtex closed this as completed Jan 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lyzadanger lyzadanger

Labels
Class:Piezo
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lyzadanger @dtex @islemaster