Program stops working after a minute

Question

I have made this small program to lighten up in sequence 8 LEDs, it starts fine, but it stops working after a minute or so and the first 2 LEDs stay permanently ON, its like the chip stoped, here is the code :

// Initialize an array with all 8 leds
// and give them the corresponding
// digital number.
const byte led[8] = {0,1,2,3,4,5,6,7};


// the setup routine runs once when you press reset:
void setup() {

  // initialize all 8 digital I/O pins as outputs.
  for (byte i = 0; i<9; i++) {
    pinMode(led[i], OUTPUT);
  }
  pinMode(13, OUTPUT);
}

void left (void);
void right (void);

// the loop routine runs over and over again forever:
void loop() {

  right();
  left();

}


void left (void) {
  for (byte i = 0; i<9; i++) {
    digitalWrite(led[i], HIGH);
    delay(50);
    digitalWrite(led[i], LOW);
  }
}

void right (void) {
  for (byte i = 8; i>0; i--) {
    digitalWrite(led[i], HIGH);
    delay(50);
    digitalWrite(led[i], LOW);
  }
}

I am using a very old arduino duemilanove.

Answer 1

You had several problems with your loop conditions. Arrays are 0 indexed. If we declare const byte led[8], led[0] is the first element and led[7] is the 8th and last element.

Also, in void right(), you declared i as a byte which is an 8-bit, unsigned number from 0 to 255. While you wrote the conditional to be i>0, it needs to be i>-1 or i>=0. So, if we change the coditional and keep i as a byte the loop would decrement i like 8,7,6,5,4,3,2,1,0,255,254... forever. So instead, i should be declared as a char which is an 8-bit, signed number from -128 to 127.

// Initialize an array with all 8 leds
// and give them the corresponding
// digital number.
const byte led[8] = {0,1,2,3,4,5,6,7};

// the setup routine runs once when you press reset:
void setup() {
  // initialize all 8 digital I/O pins as outputs.
  for (byte i = 0; i<8; i++) {
    pinMode(led[i], OUTPUT);
  }
  pinMode(13, OUTPUT);
}

void left (void);
void right (void);

// the loop routine runs over and over again forever:
void loop() {
  right();
  left();
}

void left (void) {
  for (byte i = 0; i<8; i++) {
    digitalWrite(led[i], HIGH);
    delay(50);
    digitalWrite(led[i], LOW);
  }
}

void right (void) {
  for (char i = 7; i>=0; i--) {
    digitalWrite(led[i], HIGH);
    delay(50);
    digitalWrite(led[i], LOW);
  }
}

Answer 2

Your loop conditions aren't quite right, which could be resulting in undefined behaviour. I highly doubt it would cause it to stop working spontaneously, but it's possible in theory.

In each loop that counts upwards, you basically need to stop before the counter reaches 8. For example:

for (byte i = 0; i<8; i++) ...

Note that it's i<8 instead of i<9.

The reason is that the array is only 8 elements long, so the indices run from 0 to 7 (inclusive). Currently, your loops are going all the way up to and including 8, which means it's trying to read past the end of your array.

You'll need to fix that in setup() and left().

The loop that goes downwards in right() is a little more difficult. At the moment, it runs through indices 8 to 1, meaning it has the same problem of reading past the array. However, it also never actually reaches index 0.

It's harder to fix because byte is an unsigned type, so you can't use i>=0 as your condition (as it will never terminate). Personally, I would rewrite the loop to count upwards, but then reverse the index order inside the loop, so that 0 becomes 7, and so on. For example:

for (byte i = 0; i<8; i++) {
    digitalWrite(led[7-i], HIGH);
    delay(50);
    digitalWrite(led[7-i], LOW);
}

Like I said, I doubt this will actually solve your problem. It's worth trying though.

Answer 3

Thank you guys, its fixed now, i just dont understand why i cant use byte if i do, i>=0, this way i never goes to 255 anyway and if does, it doesn matter, unless im missing something. Here goes the final working code :

/*
  Blink
  Turns on all 8 leds in sequence back and forth.

 */


// Initialize an array with all 8 leds
// and give them the corresponding
// digital number.
const char led[8] = {0,1,2,3,4,5,6,7};
#define speed 40

// the setup routine runs once when you press reset:
void setup() {

  // initialize all 8 digital I/O pins as outputs.
  for (int i = 0; i<8; i++) {
    pinMode(led[i], OUTPUT);
  }
  pinMode(13, OUTPUT);
}

void left (void);
void right (void);

// the loop routine runs over and over again forever:
void loop() {

  // Turn off the buzzer on pin 13
  digitalWrite(13, LOW);

  while(1) {
  right();
  left();
  }

}


void left (void) {
  for (int i = 0; i<8; i++) {
    digitalWrite(led[i], HIGH);
    delay(speed);
    digitalWrite(led[i], LOW);
  }
}

void right (void) {
  for (int i = 7; i>=0; i--) {
    digitalWrite(led[i], HIGH);
    delay(speed);
    digitalWrite(led[i], LOW);
  }
}