Fix Piezo.prototype.note() #1324

islemaster · 2017-04-13T17:47:28Z

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.

Question: How is the documentation at http://johnny-five.io/api/piezo/#api generated? The documentation for tone there was very helpful in understanding the intent of these methods, but it's also confusing because the first argument is misnamed in the signature tone(frequency, duration). I couldn't find a way to update that from this repo. EDIT: Found the docs on the repository wiki and updated accordingly.

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.

islemaster · 2017-04-13T17:48:58Z

test/piezo.js

@@ -781,10 +807,11 @@ exports["Piezo - I2C Backpack"] = {
  tone: function(test) {
    test.expect(3);

-    this.piezo.tone(262, 1000);
+    // C4 = 262Hz = 1908μs duty cycle (half-period)
+    this.piezo.tone(1908, 1000);


I updated this test because it could be misleading and imply that tone was supposed to be called with a frequency (since 262Hz is a familiar frequency C4, used elsewhere in these tests).

islemaster · 2017-04-13T18:56:04Z

😖 Seeing a flaky animation test failure on Travis again, and one failure I haven't seen before:

Testing rgb.collection.js.Fatal error: Cannot read property 'setUp' of undefined�
npm ERR! Test failed.  See above for more details.

I don't repro locally, would appreciate thoughts on what went wrong here.

rwaldron · 2017-04-17T16:15:41Z

@lyzadanger review?

lyzadanger

Thank you for your work on this! I added a bunch of very fine-grained nit comments, but they are all optional from my point of view. I have not tested this on hardware; let me know if that level of review is needed—this is a code/API-level/test review only.

lyzadanger · 2017-04-17T19:02:23Z

lib/piezo.js

@@ -401,16 +401,37 @@ Piezo.ToSong = function(stringSong, beats) {
  return song;
 };

+/**
+ * Play a note for a duration.
+ * @param {string} note - see Piezo.Notes.  Case-insensitive.


Maybe add a case-sensitivity test (i.e. invoke this method with a different case in the tests; tests—at least the ones you added—look like they're all lower-case)?

lyzadanger · 2017-04-17T19:04:48Z

lib/piezo.js

-
-  return this.tone(tone, duration);
+  var frequency = Piezo.Parsers.hzFromInput(note);
+  return this.frequency(frequency, duration);


Stylistically, this could all be done in one statement, e.g.:

return this.frequency(Piezo.Parsers.hzFromInput(note), duration);

as you do in frequency, eliminating the need for the var declaration and being consistent, but that's totally up to you.

lyzadanger · 2017-04-17T19:05:38Z

lib/piezo.js

+ * @param {number} tone - Given as a computed duty-cycle,
+ *   in microseconds. Larger values produce lower tones.
+ *   See https://en.wikipedia.org/wiki/Duty_cycle
+ * @param {number} duration - in milliseconds.


Good commenting. Thank you!

lyzadanger · 2017-04-17T19:06:47Z

test/piezo.js


-    // note delegates to tone;
+    // note delegates to frequncy, which delegates to tone;


frequency is misspelled but this is in an incredibly nitpicky comment. Only change it if you have other reason to edit the test file :)

lyzadanger · 2017-04-17T19:08:58Z

lib/piezo.js

+/**
+ * Play a note for a duration.
+ * @param {string} note - see Piezo.Notes.  Case-insensitive.
+ *   If an octave is not given, the configured default octave


Perhaps extend this comment to explain what octave means in this context (e.g. in the string value "a4", "a" is the note and 4 is the octave)?

lyzadanger · 2017-04-17T19:10:07Z

test/piezo.js

    var toneSpy = this.sandbox.spy(this.piezo, "tone");
+    var frequencySpy = this.sandbox.spy(this.piezo, "frequency");

    // accepts octave.


Similarly to comments for the note attribute of the note method...maybe drop in definition of difference between note and octave here?

islemaster · 2017-04-17T19:38:33Z

Thank you for the review @lyzadanger! I'll push an update addressing your comments by this time tomorrow. I can test these changes locally with an Adafruit Circuit Playground board and Circuit Playground Firmata but I don't have access to any other hardware for testing. If you feel additional testing is required I'll need help with that.

- 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 · 2017-04-17T22:56:32Z

Review feedback addressed.

lyzadanger · 2017-04-18T13:09:55Z

@islemaster Lookin' good! I'd love to see the fixup commits squashed when this merges but that's probably in @rwaldron 's purview... :)

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 commented Apr 13, 2017

View reviewed changes

Fix lint error

50a3cec

lyzadanger approved these changes Apr 17, 2017

View reviewed changes

Address review feedback

050489e

- 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 mentioned this pull request Apr 18, 2017

Fix Piezo.prototype.note() code-dot-org/johnny-five#3

Merged

islemaster closed this in code-dot-org/johnny-five#3 Apr 18, 2017

islemaster mentioned this pull request Apr 18, 2017

Fix Piezo.prototype.note() #1328

Merged

islemaster deleted the piezo-note-calls-frequency branch April 21, 2017 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Piezo.prototype.note() #1324

Fix Piezo.prototype.note() #1324

islemaster commented Apr 13, 2017 •

edited

Loading

islemaster Apr 13, 2017

islemaster commented Apr 13, 2017

rwaldron commented Apr 17, 2017

lyzadanger left a comment

lyzadanger Apr 17, 2017

lyzadanger Apr 17, 2017

lyzadanger Apr 17, 2017

lyzadanger Apr 17, 2017

lyzadanger Apr 17, 2017

lyzadanger Apr 17, 2017

islemaster commented Apr 17, 2017

islemaster commented Apr 17, 2017

lyzadanger commented Apr 18, 2017


		// note delegates to tone;
		// note delegates to frequncy, which delegates to tone;

Fix Piezo.prototype.note() #1324

Fix Piezo.prototype.note() #1324

Conversation

islemaster commented Apr 13, 2017 • edited Loading

islemaster Apr 13, 2017

Choose a reason for hiding this comment

islemaster commented Apr 13, 2017

rwaldron commented Apr 17, 2017

lyzadanger left a comment

Choose a reason for hiding this comment

lyzadanger Apr 17, 2017

Choose a reason for hiding this comment

lyzadanger Apr 17, 2017

Choose a reason for hiding this comment

lyzadanger Apr 17, 2017

Choose a reason for hiding this comment

lyzadanger Apr 17, 2017

Choose a reason for hiding this comment

lyzadanger Apr 17, 2017

Choose a reason for hiding this comment

lyzadanger Apr 17, 2017

Choose a reason for hiding this comment

islemaster commented Apr 17, 2017

islemaster commented Apr 17, 2017

lyzadanger commented Apr 18, 2017

islemaster commented Apr 13, 2017 •

edited

Loading