Moving variables from Global to functions

Hi guys, apologies if this is in the wrong place,

I'm starting my first Arduino project and as part of the prep work I'm trying to find the best way to handle RGB information and colour transitions for an LED. My script so far is:

#define RED 11
#define GREEN 10
#define BLUE 9

int Colours[21][3] = {
{150,100,70}, // White 0
{200,0,0}, // Red 1
{201,9,0}, // Maroon 2
{189,18,0}, // Ember 3
{177,27,0}, // Orange 4
{190,24,6}, // Salmon 5
{180,81,1}, // Straw 6
{105,81,0}, // Honeycombe 7
{100,100,6}, // Aqua 8
{21,172,9}, // Grass 9
{0,200,0}, // Green 10
{0,105,21}, // Swamp 11
{36,150,60}, // Teal 12
{50,139,150}, // Crystal 13
{30,60,186}, // Cyan 14
{0,0,200}, // Blue 15
{90,3,186}, // Purple 16
{120,0,126}, // Lilac 17
{163,1,24}, // Pink 18
{99,15,69}, // Soft 19
{186,36,54} // Pastel 20
};

int Row = 0;
int R;
int G;
int B;

int Static() {
if(Row == 20) { Row = 0; }
else { Row += 1; }
}

void setup() {
pinMode(RED, OUTPUT);
pinMode(GREEN, OUTPUT);
pinMode(BLUE, OUTPUT);
}

void loop() {
Static();
R = Colours[Row][0];
G = Colours[Row][1];
B = Colours[Row][2];
analogWrite(RED,R);
analogWrite(GREEN,G);
analogWrite(BLUE,B);
delay(750);
}

This allows the LED to scroll through the colours in the multi-dimensional array. The thing is, it only works if I have the Row variable as global and I would prefer it to be nested within the function (without making explicit references to the RGB values within the function) so that different LEDs can behave according to different functions at the same time. Similarly, I'd like the delay to be nested within the function rather than in the loop. How might I go about this?

If you want a variable that is not global to survive (ie remember its previous value) across calls to a function (like loop() ) then you need to declare that variable static

Read about scope as well

PS: , do yourself a favour and please read How to get the best out of this forum and modify your first post accordingly (including code tags).

1 Like

An admirable goal, but what's going to kill that plan is using delay() for the essential timing of the effects.

Spend some time here:

Even if it doesn't help you directly now, it will not be a waste of your time.

a7

I'd move the colours to a new file, something like Colours.h and include it in the main sketch like:

struct RGB_Colours  {
  const char * name;
  byte R;
  byte G;
  byte B;
  RGB_Colours(const char * name, byte R, byte G, byte B)
    : R(R), G(G), B(B) { }
};

namespace Colour {

 const byte size = 21;

  RGB_Colours list[size] = {
    { "WHITE", 150, 100, 70 },
    { "RED", 200, 0, 0 },
    { "MAROON", 201, 9, 0 },
    { "EMBER", 189, 18, 0 },
    { "ORANGE", 177, 27, 0 },
    { "SALMON", 190, 24, 6 },
    { "STRAW", 180, 81, 1 },
    { "HONEYCOMBE", 105, 81, 0 },
    { "AQUA", 100, 100, 6 },
    { "GRASS", 21, 172, 9 },
    { "GREEN", 0, 200, 0 },
    { "SWAP", 0, 105, 21 },
    { "TEAL", 36, 150, 60 },
    { "CRYSTAL", 50, 139, 150 },
    { "CYAN", 30, 60, 186 },
    { "BLUE", 0, 0, 200 },
    { "PURPLE", 90, 3, 186 },
    { "LILAC", 120, 0, 126 },
    { "PINK", 163, 1, 24 },
    { "SOFT", 99, 15, 69 },
    { "PASTEL", 186, 36, 54 }
  };
  
} // End of Colour

Then have the main sketch something like this:

#define RED_PIN 11
#define GREEN_PIN 10
#define BLUE_PIN 9

#include "Colours.h"

void displayAll() {
  for (byte i = 0; i < Colour::size; i++) {
    analogWrite(RED_PIN, Colour::list[i].R);
    analogWrite(GREEN_PIN, Colour::list[i].G);
    analogWrite(BLUE_PIN, Colour::list[i].B);
    delay(750);
  }
}

void setup() {
  pinMode(RED_PIN, OUTPUT);
  pinMode(GREEN_PIN, OUTPUT);
  pinMode(BLUE_PIN, OUTPUT);
}

void loop() {
  displayAll();
}

Of course, if you don't want it blocking then the for loop and the delay will need changing. But if you're only displaying one 'light show' at a time, it should be fine. Also, you don't really need the colour names, but I would like them at least for debugging!

Could always create a class and go OO

Thanks for the quick solution and links to the forum help page! :slight_smile:

I think for the long term project this is probably the way to go. It's weird, now you've said it it seems obvious but its almost like because I've got hardware now all my knowledge of C programming has gone out the window!

Try bringing the code for lighting the LEDs out of the loop() and inside it's own function, something like

uint8_t set_colour (const uint8_t row)
{
    // analogWrite() x 3, as in loop() above
   
   if (row == 20) return 0;
   return (1 + row);

   /* That 20 magic number is better refactored as
    * a defined constant, by the way.
    */
}

You can then change the row as a timed event:

uint32_t g_timestamp;
uint8_t  g_row = 0;
const uint32_t REFRESH_TIME = 750;

void loop ()
{
    if (millis() - g_timestamp >= REFRESH_TIME)
    {
        g_timestamp = millis();
        g_row = set_colour (g_row);
    }
}

When this works, you can extend the code to work with multiple LEDs. To avoid the proliferation of global variables, create a strcut for saving data about LEDs, for instance

struct Led
{
    const uint8_t pin_red;
    const uint8_t pin_green;
    const uint8_t pin_blue;
    uint8_t row;
    const uint32_t refresh_time    
    uint32_t timestamp;
};