Add pulse method to RGB class #1803
Conversation
| /* istanbul ignore else */ | ||
| if (this.isOn) { | ||
| state.prev = RGB.colors.reduce((current, color) => (current[color] = state[color], current), {}); | ||
|
|
There was a problem hiding this comment.
Within the off() method, there is no need to reassign state.prev if the RGB is on since state.prev is assigned when the on() method is called. This also ensures that if off() is called when the RGB is pulsing that the RGB values are NOT assigned lesser and lesser values until reaching (0,0,0) which is the case when off() is repeatedly called after pulse() is called if the line above remains.
| } | ||
|
|
||
|
|
||
| } | ||
|
|
||
| RGB.colors = ["red", "green", "blue"]; |
There was a problem hiding this comment.
The assignment of RGB.colors was moved before Animation.keys is assigned to it. This small PR resolves this issue and adds a unit test for this fix.
lib/led/rgb.js
Outdated
| return this.color(frames[0]); | ||
| const state = priv.get(this); | ||
| state.value = frames[0]; | ||
| return this.update(); |
There was a problem hiding this comment.
The Animation.render method here was calling color(), which does a lot of normalization and validation before itself calling update on line 270 above.
Now the render method behaves more like render in the led class which 'trusts' its animations and calls update directly .
This also bypasses the reassignment of state.prev to a 'tween' value when Animation.render is called for pulse function which resulted in the RGB values decreasing so that the RGB light would gradually decrease.
There was a problem hiding this comment.
Thank you, @fisher-alice; This is an excellent addition to the RGB class.
Everything looks good, and I think the only thing that is missing is documentation. The wiki needs the RGB.pulse() method, and it warrants an example file too.
Can you add those things?
…value if on() called after pulse()
| this.update(colors); | ||
| } else if (state.isRunning) { |
There was a problem hiding this comment.
Added this else-if branch for case when on() is called while RGB is pulsing so that RGB displays full RGB values and not 'tween' values of pulse method.
Hello @dtex , thanks for taking time to look at my PR! Yes, I can definitely add documentation to the wiki and create an example file. FYI - I also added a commit since your comment. |
…method, revise comments
|
Oh, you're in Houston too?! Cool! I haven't been getting out much lately (for like almost three years). Have you been to any local meetups or groups on IoT or robtics stuff? I see Houston Robotics Club has been re-animated but I haven't checked it out yet. There are a few PR's here that I think are ready. I'll ping Rick and see if I can get them merged. |
Currently, the Led class contains both pulse and blink methods, but the Rgb class only contains the blink method.
This PR adds a
pulsemethod to the Rgb class. Note that the pulse method was recently re-added to the forked branchcode-dot-org/johnny-fiveafter being updated to the latest release of upstream. Thepulsemethod was first added to the Rgb class by @islemaster in this PR.I added the state value
isRunningwhich was already a defined property in order to keep track of whether an Animation is in progress, i.e., ifpulseis being run. This is similar to howisRunningis used inlib/led.js.The update method is now writeable to be able to reassign it in the unit test for Animation.render in
test/rgb.js.More details of implementation are included in embedded comments.
Testing
I modified unit tests in both
rgb.collection.jsandrgb.jsas well as added new unit tests for the RGB pulse method.Documentation
At the request of @dtex, I added the example file
docs/led-rgb-pulse.mdand here is documentation about the Rgb pulse method that can be added to the johnny-five wiki.