Add RGB.pulse method #10

islemaster · 2018-01-10T21:36:56Z

Similar to the LED.pulse() method that runs an animation for a colorless LED, but implemented for a color LED. I had to make more changes than I expected here, I'll annotate them inline.

Tested against Adafruit Circuit Playground Developer Edition in Code.org's App Lab+Maker Toolkit environment, against my local development machine.

islemaster · 2018-01-10T21:37:47Z

lib/led/rgb.js

@@ -182,7 +182,8 @@ function RGB(opts) {
    blue: 255,
    intensity: 100,
    isAnode: opts.isAnode || false,
-    interval: null
+    interval: null,
+    isRunning: false


isRunning is used to track whether an Animation (requiring more detail that setInterval) is in progress. Copying the pattern used in led.js here.

islemaster · 2018-01-10T21:41:24Z

lib/led/rgb.js

+    red: 255,
+    green: 255,
+    blue: 255
+  };


Before, state.prev was set during calls to off() so that you could later call on() and restore the LED to its previous color. It was cleared when it was consumed by on() and a default value of white (255,255,255) was encoded in that method.

Since nothing else depended on state.prev, I've added it to the initial component state with its default white value.

I now update it when color() is called, to handle the following case:

rgb.color('red'); rgb.pulse(300);

I'm also not clearing it when on() is called - there's no particular reason to.

It may look like this is redundant with the state.values object, above, but upon further examination the comment on state.values does not accurately describe the way it or the state.red, state.green, state.blue properties are used - they seem to always represent the current interpolated RGB values, not the ones most recently set.

islemaster · 2018-01-10T21:45:27Z

lib/led/rgb.js

+RGB.prototype.pulse = function(duration, callback) {
+  var state = priv.get(this);
+
+  var currentColor = state.prev;


pulse() uses the last color set using color(), saved to state.prev instead of checking the actual current color of the LEDs. This is to avoid the problem where subsequent calls to pulse() would lead to progressively smaller intensity ranges. Consider this case:

color('red'); pulse(300); // Sets up an animation for intensity 0-100 // 150ms passes pulse(300); // Current intensity is 50, so new animation created from the interpolated color appears to be intensity 0-50. // 150ms passes pulse(300); // Current intensity is 50 but appears to be 25, so new animation appears to be intensity 0-25. // ...and so on, the animation gets dimmer with each iteration.

islemaster · 2018-01-10T21:46:21Z

lib/led/rgb.js

@@ -573,7 +621,7 @@ RGB.prototype[Animation.normalize] = function(keyFrames) {
 */

 RGB.prototype[Animation.render] = function(frames) {
-  return this.color(frames[0]);
+  return this.update(frames[0]);


The animation render method here was calling color(), which does a lot of normalization and validation before itself calling update. I've changed this to behave more like the led class, which trusts its animations and calls update directly.

islemaster · 2018-01-10T21:49:44Z

test/rgb.collection.js


    var rgbs = new RGB.Collection([
      [1, 2, 3],
      [4, 5, 6],
    ]);

-    this.color = this.sandbox.stub(RGB.prototype, "color");
+    rgbs.each(function(rgb) {
+      this.sandbox.stub(rgb, "write");


I had to update these tests that stub color to test animations, since the Animation.render method no longer calls color. I can't stub update because it's a read-only property, so I'm stubbing write which is another layer down, and a better representation of the values actually being sent to the device.

davidsbailey · 2018-01-11T02:26:12Z

sorry I missed this today, @islemaster . I will take a look in the morning.

davidsbailey · 2018-01-11T19:52:48Z

👀 looking...

davidsbailey

Apologies for the slow response, much of this is new to me. I have a few questions inline.

davidsbailey · 2018-01-11T21:47:03Z

test/rgb.collection.js

@@ -291,21 +291,36 @@ exports["RGB.Collection"] = {


  "Animation.render": function(test) {
-    test.expect(1);
+    test.expect(4);


conceptually, what are you expecting to equal 4 here? Are there any docs or other guidance for understanding these test cases?

Johnny-Five tests are written with nodeunit. API docs are inline in that README, here. expect(count) sets the number of assertions the test expects to run. This count is checked when done() is called at the end of the test, and the test failed if there were missing/extra assertions.

davidsbailey · 2018-01-11T21:49:13Z

test/rgb.js

-    this.rgb[Animation.render]([0]);
-    test.equal(this.color.callCount, 1);
+    test.expect(2);
+    // rgb.write() is already wrapped


where is rgb.write() wrapped?

Line 55, in the setUp method for this block of tests, it's stubbed using sinon:

johnny-five/test/rgb.js

Line 55 in f1b1a55

this.write = this.sandbox.spy(this.rgb, "write");

davidsbailey · 2018-01-11T21:59:25Z

lib/led/rgb.js

+    ],
+    metronomic: true,
+    loop: true,
+    easing: "inOutSine",


Where are the docs for these options? I don't see much at https://github.com/rwaldron/johnny-five/blob/002b5191b99a990bc6e6f34ada62b99d5a01ac09/lib/animation.js#L139-L145

See http://johnny-five.io/api/animation/#segment-properties

davidsbailey · 2018-01-11T22:08:20Z

docs/accelerometer-LIS3DH.md

@@ -77,7 +77,7 @@ board.on("ready", function() {
 ## License
 Copyright (c) 2012, 2013, 2014 Rick Waldron <waldron.rick@gmail.com>
 Licensed under the MIT license.
-Copyright (c) 2017 The Johnny-Five Contributors
+Copyright (c) 2018 The Johnny-Five Contributors


I didn't see any changes in this commit besides the copyright year change. Did I miss something?

No: The CI setup for this repo regenerates these docs, and complains if it finds differences between what it generated and the committed copies. Due to the year change, it's requiring me to commit updates for all the documentation files to pass CI.

davidsbailey

LGTM, though still interested in the answers to my questions.

davidsbailey · 2018-01-11T22:25:39Z

Awesome, thanks for the answers, Brad! Hopefully this helps grease the rails for the next review.

islemaster commented Jan 10, 2018

View reviewed changes

islemaster added 2 commits January 10, 2018 13:52

Add RGB.pulse method

f1b1a55

Updated examples

2e8a074

islemaster force-pushed the rgb-pulse branch from d2005bf to 2e8a074 Compare January 10, 2018 21:53

islemaster requested a review from davidsbailey January 10, 2018 21:54

davidsbailey reviewed Jan 11, 2018

View reviewed changes

davidsbailey approved these changes Jan 11, 2018

View reviewed changes

islemaster merged commit e4d4db2 into master Jan 11, 2018

islemaster deleted the rgb-pulse branch January 11, 2018 22:52

MeyAyre mentioned this pull request Jan 11, 2018

Created block for colorLeds[i].pulse code-dot-org/code-dot-org#19737

Merged

fisher-alice mentioned this pull request Oct 13, 2022

Add RGB pulse function #15

Merged

This was referenced Nov 30, 2022

Fix rgb pulse function #24

Merged

Add pulse method to RGB class rwaldron/johnny-five#1803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RGB.pulse method #10

Add RGB.pulse method #10

islemaster commented Jan 10, 2018

islemaster Jan 10, 2018

islemaster Jan 10, 2018 •

edited

Loading

islemaster Jan 10, 2018

islemaster Jan 10, 2018

islemaster Jan 10, 2018

davidsbailey commented Jan 11, 2018

davidsbailey commented Jan 11, 2018

davidsbailey left a comment

davidsbailey Jan 11, 2018

islemaster Jan 11, 2018

davidsbailey Jan 11, 2018

islemaster Jan 11, 2018

davidsbailey Jan 11, 2018

islemaster Jan 11, 2018

davidsbailey Jan 11, 2018

islemaster Jan 11, 2018

davidsbailey left a comment

davidsbailey commented Jan 11, 2018

Add RGB.pulse method #10

Add RGB.pulse method #10

Conversation

islemaster commented Jan 10, 2018

Choose a reason for hiding this comment

islemaster Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey commented Jan 11, 2018

davidsbailey commented Jan 11, 2018

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

davidsbailey commented Jan 11, 2018

islemaster Jan 10, 2018 •

edited

Loading