Arduino fade in/out 5 leds circuit acting wierd

Question

I have a simple arduino project with 5 leds, the project should start with led 1 fade through 50 millisec to reach full brightness, then led 2 and so on until led 5 then turn them all off and loop.

here is an image for the circuit (pardon the messy look my first time):

"all leds cathod are connected" and here is the code :

 int led[5]={3,5,6,9,10};
// led pins array (pwm)
void setup() {
  // put your setup code here, to run once:
  for (int i = 0; i < 5; i++){
    pinMode(led[i], OUTPUT);
    }
    // set each pin as an output
}

void loop() {
  for (int i = 0; i < 5; i++){
    int pin = led[i];
    // storing the value of the current pin in a variable
    int j = 0;
    while(j <= 50){
      j++;
      delay(1);
      analogWrite(pin,j*2);
    }
    /* the while loop is used to make the fade effect
    by increasing the duty cycle by 2% each millisec 
    over 50 millisec */
  }
  delay(200);
  
  for (int z = 5; z > -1; z--){
    int pin = led[z];
    analogWrite(pin,0);
    }
    // turning off all leds
    delay(50);
}

the problem is as follows : the first round of the loop the circuit acts as expected led 1 on then 2 etc then they all turn off and loops again, on loop 16 led 1 turns off and the loop continues only with leds from 2-5 and on loop 31 led 4 and 5 turns off and only led 2 and 3 keep on going.

Edit 1 : i just found something interesting on arduino's official page (pins supporting pwm 3, 5, 6, 9, 10, 11 ) pwm frequency 490 Hz (pins 5 and 6: 980 Hz) so could the frequency result in this error?

Edit 2 : the issue was resolved with a really wierd change in the code the very first line of code " int led[5]={3,5,6,9,10};" i changed the size of the array from 5 to 6 like this " int led[6]={3,5,6,9,10};" and the issue was resolved, I discovered this while i was trying to use 4 leds and i forgot to change the size, if anyone could explain how did this change helped solve the problem ?

Answer 1

Your problem may be in your count down code. Specifically this loop:

for (int z = 5; z > -1; z--){

Z starts at 5, and you then try and access element 5 of a 5-element array. That sounds reasonable until you remember arrays are 0-based, so there is no element 5, only elements 0-4.

So you're reading data that you shouldn't be reading. Just reading shouldn't have any effect though, so :shrug:...

Anyway, there's some "best practice" things you should do to improve your code:

1. Use 8-bit values to store your pins (uint8_t)
2. Make your pins array const so it's never modifyable
3. Replace your while loop with a simpler to understand and follow for loop
4. Keep your loops as simple count-up and use math to work out which array entry you want.
5. Keep variables to the smallest storage size needed for the values you're working with.

For example (untested):

const uint8_t led[5] = { 3,5,6,9,10 };
// led pins array (pwm)

void setup() {
  for (uint8_t i = 0; i < 5; i++) {
    pinMode(led[i], OUTPUT);
  }
}

void loop() {
  for (uint8_t i = 0; i < 5; i++){
    int pin = led[i];
    for (uint8_t j = 0; j < 50; j++) {
      delay(1);
      analogWrite(pin, j * 2);
    }
  }

  delay(200);
  
  for (int z = 0; z < 5; z++) {
    int pin = led[4 - z];
    analogWrite(pin, 0);
  }

  delay(50);
}

Also as noted in the comments you must have one resistor per LED and never connect all the cathodes together like that. The brightness of each LED is determined by the current flowing through it. That current is the current through the resistor, which is determined by the forward volgage of the LEDs. The more LEDs you have on the more that current is split between the LEDs. And different LEDs will have different forward voltages, and the lowest one will "win" affecting the current that the resistor allows. It's all a horrible mess and must be avoided at all costs. The only times you can have just one resistor for multiple LEDs are a) if you have a series chain of LEDs making them in effect just one LED with a higher forward voltage, or b) can absolutely guarantee that one and only one LED will ever be turned on at a time.

Answer 2

Here is the @Majenko code output

The code same as the one posted in the other answer. You can find the simulation link here --> https://wokwi.com/arduino/projects/318609194610590292

I also hooked up a logic analyser and here is how it looks :) You can look how the Duty cyle is becoming larger afer every cycle.