Go Down

Topic: Array value change unexpectedly over time (Read 601 times) previous topic - next topic

grantusmaximus

This has had me stumped for a few days now! I've a lot of programming experience but new to C++ and Arduino..

I've an array (_RGBpins) whose values randomly/unexpectedly change over time.

Below is a cut down sketch which replicated the problem with serial debugging.

Code: [Select]

#include "rgbled.h"

RGBLed rgbled(6,7,8);

//The setup function is called once at startup of the sketch
void setup()
{
  Serial.begin(9600);
  Serial.print("started ");
}

// The loop function is called in an endless loop
void loop()
{
  byte white[] = {255,255,255};
  byte black[] = {0,0,0};
  rgbled.tempChange(white);
  delay(3000);
  rgbled.tempChange(black);
 
}


rgbled.cpp
Code: [Select]

#include "rgbled.h"
#include "Arduino.h"

// led pins
byte _RGBpins[3];

// constructor function
RGBLed::RGBLed(byte redPin, byte greenPin, byte bluePin) {
  _RGBpins[0] = redPin;
  _RGBpins[1] = greenPin;
  _RGBpins[2] = bluePin;

  for (byte i = 0; i < 3; i++) {
    pinMode(_RGBpins[i], OUTPUT);
  }
}

void RGBLed::tempChange(byte rgb[]) {
  Serial.print(" pins:");
  for (byte i = 0; i < 3; i++) {
      Serial.print(_RGBpins[i]); // values in _RGBpins change randomly!?!
      Serial.print(",");
  }
}


rgbled.h
Code: [Select]

#ifndef RGBLED_H_
#define RGBLED_H_
#include "Arduino.h"

class RGBLed {
public:
                RGBLed(byte redPin, byte greenPin, byte bluePin);
void tempChange(byte rgb[]);
private:
                byte _RGBpins[];

};

#endif


thanks in advance..

Nick Gammon

Quote
Code: [Select]
private:
                byte _RGBpins[];

...

  _RGBpins[0] = redPin;
  _RGBpins[1] = greenPin;
  _RGBpins[2] = bluePin;



You are putting 3 bytes of data into a zero-sized array. Very bad. Try:

Code: [Select]

private:
                byte _RGBpins[3];

Nick Gammon

Also, due to the "static initialization order fiasco" you should not be calling library functions from a constructor. Remove the following:

Code: [Select]

for (byte i = 0; i < 3; i++) {
    pinMode(_RGBpins[i], OUTPUT);
  }


... from the constructor. Make a RGBLed::begin() function, and call that in setup(), like this:

Code: [Select]

void RGBLed::begin ()
{
  for (byte i = 0; i < 3; i++) {
    pinMode(_RGBpins[i], OUTPUT);
  }
}

grantusmaximus

Thanks heaps Nick!

Haven't had much experience with header files before, didn't realise the size needed to be set there too.

Also I'd noticed some constructor problems earlier, will go with your begin() suggestions thanks!

:)

Go Up