Add two animations: Rainbow and Theater #159
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rainbow is a slow transition through the rainbow Theater is a 'crawl' paired with the rainbow fade enums are allocated, but these are not hooked up anywhere yet.
binnur
requested changes
Feb 27, 2020
| @@ -95,6 +95,8 @@ enum | |||
| kClimb, | |||
| kVision, | |||
| kEject, | |||
| kRainbow, | |||
| kTheater, | |||
There was a problem hiding this comment.
kRainbow and kTheater are animations and not specific robot states. Given that, why not just change the existing animations (ex. auto_mode, see switch statement) to use these instead of creating new commands? Do you have specific robot state use cases for these animations, if so the enums should reflect that state.
| @@ -443,3 +443,60 @@ void flash(unsigned long timeInterval, uint32_t color, uint8_t brightness) | |||
| setAllPixelsOff(); | |||
| showPixels(); | |||
| } | |||
|
|
|||
| void rainbowCycle(int SpeedDelay) { | |||
There was a problem hiding this comment.
As a note, delay() function in Arduino is an unsigned long. Regardless, you don't want an accidental signed argument pass to this function, it will result in an unpredictable delay.
| byte *c; | ||
| uint16_t i, j; | ||
|
|
||
| for(j=0; j<256*5; j++) { // 5 cycles of all colors on wheel |
There was a problem hiding this comment.
Please reformat the code to match the existing coding style, i.e. placement of {}
|
As to comment 1, it is not clear to me what animations we will finally want
for the current robot states. I wanted to include these new animations so
that we could use them, if that was desired. I included them in the set of
enums - perhaps it would be better to comment them out?
I will fix the issues in comments 2 and 3.
…-randy
On Wed, Feb 26, 2020 at 8:19 PM Binnur Al-Kazily ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In Arduino-Bling/RobotBling/RobotBling.ino
<#159 (comment)>
:
> @@ -95,6 +95,8 @@ enum
kClimb,
kVision,
kEject,
+ kRainbow,
+ kTheater,
kRainbow and kTheater are animations and not specific robot states. Given
that, why not just change the existing animations (ex. auto_mode, see
switch statement) to use these instead of creating new commands? Do you
have specific robot state use cases for these animations, if so the enums
should reflect that state.
------------------------------
In Arduino-Bling/RobotBling/animations.ino
<#159 (comment)>
:
> @@ -443,3 +443,60 @@ void flash(unsigned long timeInterval, uint32_t color, uint8_t brightness)
setAllPixelsOff();
showPixels();
}
+
+void rainbowCycle(int SpeedDelay) {
As a note, delay() function in Arduino is an unsigned long. Regardless,
you don't want an accidental signed argument pass to this function, it will
result in an unpredictable delay.
------------------------------
In Arduino-Bling/RobotBling/animations.ino
<#159 (comment)>
:
> @@ -443,3 +443,60 @@ void flash(unsigned long timeInterval, uint32_t color, uint8_t brightness)
setAllPixelsOff();
showPixels();
}
+
+void rainbowCycle(int SpeedDelay) {
+ byte *c;
+ uint16_t i, j;
+
+ for(j=0; j<256*5; j++) { // 5 cycles of all colors on wheel
Please reformat the code to match the existing coding style, i.e.
placement of {}
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159?email_source=notifications&email_token=AADAW6Y2YKDFEOF7JCM5B4LRE45OVA5CNFSM4K4RRVX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXDZ3GI#pullrequestreview-365403545>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAW627G663DYLDMB2G4MTRE45OVANCNFSM4K4RRVXQ>
.
|
|
|
Then I will take the code out of the Java enum and the Arduino, and just
leave the new animations in animations.ino, so that they could be used as a
replacement for an existing implementation. So there is no confusion.
…-randy
On Wed, Feb 26, 2020 at 8:59 PM Binnur Al-Kazily ***@***.***> wrote:
it is not clear to me what animations we will finally want for the current
robot states
I don't understand this comment -- All the robot states and animations are
implemented and integrated. Arduino loop() switch statement lists out the
currently planned robot states & animations. Other than vision, these robot
states are already integrated into the command actions via joystick button
controls. Note that the existing animations can easily be replaced with
others w/o a problem. With that said two important things to note: 1) as
far as I know, we don't have LED installed on the robot; 2) this PR will
get merged after the competition weekend to avoid unnecessary code churn.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159?email_source=notifications&email_token=AADAW6YMPLTALUBRCNQY2OLRE5CDLA5CNFSM4K4RRVX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENC56WY#issuecomment-591781723>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAW65HXMTN7AI5KYPIVXDRE5CDLANCNFSM4K4RRVXQ>
.
|
Not clear where these animations might be used. Remove my integrations, but preserve the changes to animations.ino
|
Per request - changed PR. |
omentic
requested changes
Feb 28, 2020
| for(j=0; j<256*5; j++) | ||
| { // 5 cycles of all colors on wheel | ||
| for(i=0; i< NUM_LEDS; i++) | ||
| { |
There was a problem hiding this comment.
@randomgrace here and in other place indentation is all messed up - looks like the problem is a mixture of tabs and spaces. (even though i like tabs better) i'd suggest using spaces.
| for (int j=0; j < 256; j++) | ||
| { // cycle all 256 colors in the wheel | ||
| for (int q=0; q < 3; q++) | ||
| { |
There was a problem hiding this comment.
seems like some tab characters are eight-width characters while space indents are two...
|
|
||
| if(WheelPos < 85) | ||
| { | ||
| c[0]=WheelPos * 3; |
| void theaterChaseRainbow(unsigned long SpeedDelay) { | ||
| byte *c; | ||
|
|
||
| for (int j=0; j < 256; j++) |
There was a problem hiding this comment.
also throughout this file (including outside of your pr) i see different uses of non-indentation spaces, ex. int j = 0 or int j=0.
|
My bad - I thought that I had massaged my .vimrc file to correctly do
indents and expandtabs, but obviously not.
…-randy
On Thu, Feb 27, 2020, 8:03 PM j-james ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In Arduino-Bling/RobotBling/animations.ino
<#159 (comment)>
:
> @@ -443,3 +443,73 @@ void flash(unsigned long timeInterval, uint32_t color, uint8_t brightness)
setAllPixelsOff();
showPixels();
}
+
+void rainbowCycle(unsigned long SpeedDelay)
+{
+ byte *c;
+ uint16_t i, j;
+
+ for(j=0; j<256*5; j++)
+ { // 5 cycles of all colors on wheel
+ for(i=0; i< NUM_LEDS; i++)
+ {
@randomgrace <https://github.com/randomgrace> here and in other place
indentation is all messed up - looks like the problem is a mixture of tabs
and spaces. (even though i like tabs better) i'd suggest using spaces.
------------------------------
In Arduino-Bling/RobotBling/animations.ino
<#159 (comment)>
:
> + WheelPos -= 170;
+ c[0]=0;
+ c[1]=WheelPos * 3;
+ c[2]=255 - WheelPos * 3;
+ }
+
+ return c;
+}
+
+void theaterChaseRainbow(unsigned long SpeedDelay) {
+ byte *c;
+
+ for (int j=0; j < 256; j++)
+ { // cycle all 256 colors in the wheel
+ for (int q=0; q < 3; q++)
+ {
seems like some tab characters are eight-width characters while space
indents are two...
------------------------------
In Arduino-Bling/RobotBling/animations.ino
<#159 (comment)>
:
> + c=Wheel(((i * 256 / NUM_LEDS) + j) & 255);
+ pixels.setPixelColor(i, *c, *(c+1), *(c+2));
+ }
+ showPixels();
+ delay(SpeedDelay);
+ }
+}
+
+// used by rainbowCycle and theaterChaseRainbow
+byte * Wheel(byte WheelPos)
+{
+ static byte c[3];
+
+ if(WheelPos < 85)
+ {
+ c[0]=WheelPos * 3;
are these one-space indents?
------------------------------
In Arduino-Bling/RobotBling/animations.ino
<#159 (comment)>
:
> + }
+ else
+ {
+ WheelPos -= 170;
+ c[0]=0;
+ c[1]=WheelPos * 3;
+ c[2]=255 - WheelPos * 3;
+ }
+
+ return c;
+}
+
+void theaterChaseRainbow(unsigned long SpeedDelay) {
+ byte *c;
+
+ for (int j=0; j < 256; j++)
also throughout this file (including outside of your pr) i see different
uses of non-indentation spaces, ex. int j = 0 or int j=0.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#159?email_source=notifications&email_token=AADAW63ADYSX6GZBZLI4P4DRFCEKLA5CNFSM4K4RRVX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJPWVI#pullrequestreview-366148437>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAW64KCLPN3P2W4TGS43TRFCEKLANCNFSM4K4RRVXQ>
.
|
|
I think that the indents are fixed.
…-randy
On Thu, Feb 27, 2020 at 10:58 PM Randy Groves ***@***.***> wrote:
My bad - I thought that I had massaged my .vimrc file to correctly do
indents and expandtabs, but obviously not.
-randy
On Thu, Feb 27, 2020, 8:03 PM j-james ***@***.***> wrote:
> ***@***.**** requested changes on this pull request.
> ------------------------------
>
> In Arduino-Bling/RobotBling/animations.ino
> <#159 (comment)>
> :
>
> > @@ -443,3 +443,73 @@ void flash(unsigned long timeInterval, uint32_t color, uint8_t brightness)
> setAllPixelsOff();
> showPixels();
> }
> +
> +void rainbowCycle(unsigned long SpeedDelay)
> +{
> + byte *c;
> + uint16_t i, j;
> +
> + for(j=0; j<256*5; j++)
> + { // 5 cycles of all colors on wheel
> + for(i=0; i< NUM_LEDS; i++)
> + {
>
> @randomgrace <https://github.com/randomgrace> here and in other place
> indentation is all messed up - looks like the problem is a mixture of tabs
> and spaces. (even though i like tabs better) i'd suggest using spaces.
> ------------------------------
>
> In Arduino-Bling/RobotBling/animations.ino
> <#159 (comment)>
> :
>
> > + WheelPos -= 170;
> + c[0]=0;
> + c[1]=WheelPos * 3;
> + c[2]=255 - WheelPos * 3;
> + }
> +
> + return c;
> +}
> +
> +void theaterChaseRainbow(unsigned long SpeedDelay) {
> + byte *c;
> +
> + for (int j=0; j < 256; j++)
> + { // cycle all 256 colors in the wheel
> + for (int q=0; q < 3; q++)
> + {
>
> seems like some tab characters are eight-width characters while space
> indents are two...
> ------------------------------
>
> In Arduino-Bling/RobotBling/animations.ino
> <#159 (comment)>
> :
>
> > + c=Wheel(((i * 256 / NUM_LEDS) + j) & 255);
> + pixels.setPixelColor(i, *c, *(c+1), *(c+2));
> + }
> + showPixels();
> + delay(SpeedDelay);
> + }
> +}
> +
> +// used by rainbowCycle and theaterChaseRainbow
> +byte * Wheel(byte WheelPos)
> +{
> + static byte c[3];
> +
> + if(WheelPos < 85)
> + {
> + c[0]=WheelPos * 3;
>
> are these one-space indents?
> ------------------------------
>
> In Arduino-Bling/RobotBling/animations.ino
> <#159 (comment)>
> :
>
> > + }
> + else
> + {
> + WheelPos -= 170;
> + c[0]=0;
> + c[1]=WheelPos * 3;
> + c[2]=255 - WheelPos * 3;
> + }
> +
> + return c;
> +}
> +
> +void theaterChaseRainbow(unsigned long SpeedDelay) {
> + byte *c;
> +
> + for (int j=0; j < 256; j++)
>
> also throughout this file (including outside of your pr) i see different
> uses of non-indentation spaces, ex. int j = 0 or int j=0.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#159?email_source=notifications&email_token=AADAW63ADYSX6GZBZLI4P4DRFCEKLA5CNFSM4K4RRVX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJPWVI#pullrequestreview-366148437>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADAW64KCLPN3P2W4TGS43TRFCEKLANCNFSM4K4RRVXQ>
> .
>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rainbow is a slow transition through the rainbow
Theater is a 'crawl' paired with the rainbow fade
enums are allocated, but these are not hooked up anywhere yet.