Pages: [1]   Go Down
Author Topic: Variable Seems to Change on It's Own  (Read 1180 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Newbie
*
Karma: 0
Posts: 5
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I have 4 adafruit 25 pixel strands connected to an Arduino Uno. Previously I had a button with a pull-up resistor connected to pin2 as an interrupt, but as I originally thought that might be related to my issue I've removed everything for now (including the button and related hardware connected to the Arduino).

So I've completely stripped down my code to make it as simple as possible and I'm still getting this very strange issue. Basically, the program correctly sets the 100 pixels one at a time to an off-white color. For the first 100 iterations of loop(), mode is 0 and the animation runs fine. Once it hits the end of the strand, mode changes to 30 and I can't seem to figure out why.

The output being as you would expect:
Code:
0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/0/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/30/... etc

The idea being that each press of the button should increment mode and cause the strand to switch to the next pattern. It should be able to continue animating indefinitely while performing other tasks on each frame. For now I just have removed all but one pattern, but the purpose of the switch is to allow many patterns.

Code:
#include "SPI.h"
#include "Adafruit_WS2801.h"

int dataPin  = 3;    // Yellow wire on Adafruit Pixels
int clockPin = 5;    // Green wire on Adafruit Pixels

Adafruit_WS2801 strip = Adafruit_WS2801(100, dataPin, clockPin);

int const pixels = 100;
int a[pixels]; //brightness
int r[pixels];
int g[pixels];
int b[pixels];

int mode = 0;
int currentLED = 0;

 
void setup() { 
  strip.begin();
  Serial.begin(9600);
 
  clear();
 
  strip.show();
}


void loop() {   
  Serial.print(mode);
  Serial.print("/");
 
  // drawing
  switch (mode) {
    case 0:
      colorWipe(255, 220, 255, 30);
      break;
  }
 
  // update the entire strand
  for (int i; i < pixels; i++) {
    strip.setPixelColor(i, Color(r[i] * (a[i] / 255) , g[i] * (a[i] / 255) , b[i] * (a[i] / 255 )));
  }
 
  // show the changes
  strip.show();
}

void clear() {
  for (int i = 0; i < pixels; i++) {
    r[i] = 0;
    b[i] = 0;
    g[i] = 0;
    a[i] = 0;
  }
}

// fill the dots one after the other with said color
void colorWipe(byte myA, byte myR, byte myG, byte myB) {
  a[currentLED] = myA;
  r[currentLED] = myR;
  g[currentLED] = myG;
  b[currentLED] = myB;
 
  currentLED++;
  if (currentLED > pixels) {
    currentLED = 0;
  }
}

/* Helper functions */

// Create a 24 bit color value from R,G,B
uint32_t Color(byte r, byte g, byte b)
{
  uint32_t c;
  c = r;
  c <<= 8;
  c |= g;
  c <<= 8;
  c |= b;
  return c;
}

I'm completely at a loss what else I can try. There is no longer any code that modifies the value of mode so I don't even see where to start troubleshooting.

Thanks for any suggestions
Logged

Offline Offline
Faraday Member
**
Karma: 62
Posts: 3080
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

suggestions:   where you got    for ( int i ; i<pixels ; i++ )
is probably unwise.   i might have an indeterminate initial value,    you should probably have   int i=0 ; i<pixels

your "mode" variable is declared after the arrays.   This makes it vulnerable to being overwritten if you write
past the end of the array  accidentally or on purpose.   Try declaring "mode" before the arrays.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 5
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Ah I didn't catch the incorrectly declared i.

It looks like the order of declarations fixed my problem. Very interesting, I never would have thought but it does make sense. It looks like the actual error is in the colorWipe function where it should be resetting currentLED to 0 when it is equal to 100 and not 101;

Thanks for the help, would have taken me a while to figure that out.
Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 642
Posts: 50365
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Very interesting, I never would have thought but it does make sense.
Unlike global variables, local variables are not initialized, unless YOU cause that to happen.

It's better to get in the habit of initializing every variable - local or global.

It's also better to minimize the use of global variables. pixels and currentLed should be passed to colorWipe().
Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 642
Posts: 50365
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

One final note. a, r, g, and b never hold values that are larger than a byte, do they? You could save 20% of SRAM by making the type of those arrays byte, instead of int.
Logged

United Kingdom
Offline Offline
Tesla Member
***
Karma: 227
Posts: 6637
Hofstadter's Law: It always takes longer than you expect, even when you take into account Hofstadter's Law.
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
your "mode" variable is declared after the arrays.   This makes it vulnerable to being overwritten if you write
past the end of the array  accidentally or on purpose.   Try declaring "mode" before the arrays.

Rather than cover up the bug, why not fix it? Here is the code concerned:

Code:
 currentLED++;
  if (currentLED > pixels) {
    currentLED = 0;
  }

See anything wrong? I do!

[EDIT: I hadn't fully read reply#2 when I wrote this - looks like you may have fixed this already.]
« Last Edit: January 13, 2013, 03:58:46 am by dc42 » Logged

Formal verification of safety-critical software, software development, and electronic design and prototyping. See http://www.eschertech.com. Please do not ask for unpaid help via PM, use the forum.

Offline Offline
Newbie
*
Karma: 0
Posts: 5
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Unlike global variables, local variables are not initialized, unless YOU cause that to happen.

It's better to get in the habit of initializing every variable - local or global.

It's also better to minimize the use of global variables. pixels and currentLed should be passed to colorWipe().

I wasn't sure how else to declare and initialize the arrays, should I just declare the arrays inside setup() where I initialize them? Or is there a way to set every entry in an array to the same variable? I looked around a little bit for how to do that but the for loop seemed the best way.

I'm not sure I fully understand local vs global, would I declare these variables in setup() to make them local? I know variables declared inside a function will be local, but those two will be needed by most of my patterns.

One final note. a, r, g, and b never hold values that are larger than a byte, do they? You could save 20% of SRAM by making the type of those arrays byte, instead of int.

Thanks for the tip, though I was aware of this one. Originally I had arrays storing entire 24 bit colors and when I'm tinkering I tend to leave things as they were in case I wanted to go back, but I think I'm pretty happy with this (unless you have any suggestions on how I would go about "buffering" the entire strand). The only thing I was going to change was to use HSB instead of RGB + Brightness, which would save even more space.

Rather than cover up the bug, why not fix it? Here is the code concerned:

You mean, is there something else wrong other than what I mentioned in my reply? I made the change I mentioned and it isn't writing to entry 100 anymore, but if there is something else wrong there I don't see it.
Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 642
Posts: 50365
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
should I just declare the arrays inside setup() where I initialize them?
Where do you initialize a, r, g, and b in setup? I don't see that happening.

Quote
I'm not sure I fully understand local vs global, would I declare these variables in setup() to make them local?
Yes, but, then they would be local to setup() where they are not used, rather than local to loop, where they are used.

Leaving them as global is probably best, until you understand scope better.

Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 5
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Where do you initialize a, r, g, and b in setup? I don't see that happening.

Maybe I'm unclear on what initialize means. I thought clear()—which sets every value in the array to 0—would be considered initializing it. Wouldn't I just have to use a loop similar for loop to initialize the arrays?

Yes, but, then they would be local to setup() where they are not used, rather than local to loop, where they are used.

This makes sense, but if I declare them in loop then won't they will be declared again every frame? Should I be declaring them at the beginning of loop() and then have a separate looping function inside of loop() (so that loop() essentially only runs once)? I'm getting a little off topic here, I should probably spend some more time tinkering before asking so many questions.
Logged

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 642
Posts: 50365
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Maybe I'm unclear on what initialize means. I thought clear()—which sets every value in the array to 0—would be considered initializing it. Wouldn't I just have to use a loop similar for loop to initialize the arrays?
clear() may be called from setup(), but the initialization is done IN clear(), not IN setup().

Quote
but if I declare them in loop then won't they will be declared again every frame?
Unless they are declared static.

But, its easier to just leave them global.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 5
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

clear() may be called from setup(), but the initialization is done IN clear(), not IN setup().

But, its easier to just leave them global.

Oh, right. I think I just got confused—my original point was just that they are being initialized, but I realize that was right after you said global variables are automatically initialized anyway.

Thanks again for the help. I'm still wondering what dc42 meant, but I think he just missed my reply.
Logged

Offline Offline
Edison Member
*
Karma: 116
Posts: 2205
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Variable Seems to Change on It's Own

That magic.
Logged

Pages: [1]   Go Up
Jump to: