Revising and editing a code for a RGB LED control

So I am new to Arduino and C++ coding as you can tell by the basic project I am starting off with, so I decided to build a control system for an RGB LED similar to that of Christmas lights except only using one LED. I have a set up with 2 buttons and 1 controls the color of the LED, while the other controls the “mode” of the LED which are on,blinking, and fading in and out. So after the code was done and working I was looking to make some improvements however I have no clue how to make these improvements, and am hoping someone can help me out with these.

Looking to change:
-Use some shortcuts in code to make it shorter as it is quite long.
-Find a way so the mode/color changed immediately when I press the button so I don’t need to hold it and wait for the cycle (blink/fade) to finish.
-Add a mode where the LED fades in a color and then fades into a random color, then fades out.
-Also taking suggestions for any more.

So here is the code I have (it’s quite long):

//////////////PIN VARIABLES///////////
const int LEDB = 11;
const int LEDG = 10;
const int LEDR = 9;
const int CBUTTON = 7;
const int MBUTTON = 4;
///////////COLOR VARIABLES////////////
int R = 0;
int G = 0;
int B = 0;
int Y = 0;
int P = 0;
int Cy = 0;
int W = 0;
///////////////BUTTON VARIABLES////////////
int Cval = 0;
int C = 0;
int M = 1;
int Mval = 0;
////////////////////////////////



//////////LED CONTROL////////
 ////////////RED///////////
 void RED()
 {
  if(C==1)
  {
    if(M==1)
    {
      analogWrite(LEDR, 255);
      analogWrite(LEDG, 0);
      analogWrite(LEDB, 0);
    }
    if(M==2)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
      analogWrite(LEDB, 0);
      analogWrite(LEDG, 0);
      analogWrite(LEDR, 255);
      delay(1000);
      analogWrite(LEDR, 0);
      delay(1000);
    }
    }
    if(M==3)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
      for(R=0;R<255;R++)
      {
        analogWrite(LEDR, R);
        delay(10);
      }
      for(R=255;R>0;R--)
      {
        analogWrite(LEDR, R);
        delay(10);
      }
      }
    }
  }
 }
////////////GREEN///////
void GREEN()
{
 if(C==2)
 {
  if(M==1)
  {
    analogWrite(LEDR, 0);
    analogWrite(LEDG, 255);
    analogWrite(LEDB, 0);
  }
  if(M==2)
  {
    if((Cval==LOW) &&(Mval==LOW))
    {
      analogWrite(LEDR, 0);
      analogWrite(LEDB, 0);
      analogWrite(LEDG, 255);
      delay(1000);
      analogWrite(LEDG, 0);
      delay(1000);
    }
  }
  if(M==3)
  {
    if((Cval==LOW) &&(Mval==LOW))
    {
      for(G=0;G<255;G++)
      {
        analogWrite(LEDG, G);
        delay(10);
      }
      for(G=255;G>0;G--)
      {
        analogWrite(LEDG, G);
        delay(10);
      }
    }
  }
 }
}
/////////BLUE/////////
void BLUE()
{
  if(C==3)
  {
    if(M==1)
    {
      analogWrite(LEDR, 0);
      analogWrite(LEDG, 0);
      analogWrite(LEDB, 255);
    }
    if(M==2)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
        analogWrite(LEDR, 0);
        analogWrite(LEDG, 0);
        analogWrite(LEDB, 255);
        delay(1000);
        analogWrite(LEDB, 0);
        delay(1000);
      }
    }
    if(M==3)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
        for(B=0;B<255;B++)
        {
          analogWrite(LEDB, B);
          delay(10);
        }
        for(B=255;B>0;B--)
        {
          analogWrite(LEDB, B);
          delay(10);
        }
      }
    }
  }
}
//////////YELLOW/////////
void YELLOW()
{
 if(C==4)
 {
  if(M==1)
  {
    analogWrite(LEDB, 0);
    analogWrite(LEDR, 255);
    analogWrite(LEDG, 255);
  }
  if(M==2)
  {
    if((Cval==LOW) && (Mval==LOW))
    {
      analogWrite(LEDB,0);
      analogWrite(LEDR, 255);
      analogWrite(LEDG, 255);
      delay(1000);
      analogWrite(LEDR, 0);
      analogWrite(LEDG, 0);
      delay(1000);
    }
  }
  if(M==3)
  {
    if((Cval==LOW) && (Mval==LOW))
    {
      for(Y=0;Y<255;Y++)
      {
        analogWrite(LEDR, Y);
        analogWrite(LEDG, Y);
        delay(10);
      }
      for(Y=255;Y>0;Y--)
      {
        analogWrite(LEDR, Y);
        analogWrite(LEDG, Y);
        delay(10);
      }
    }
  }
 }
}
/////////PURPLE////////////
void PURPLE()
{
if(C==5)
{
  if(M==1)
  {
    analogWrite(LEDR, 255);
    analogWrite(LEDB, 255);
    analogWrite(LEDG, 0);
  }
  if(M==2)
  {
    if((Cval==LOW) && (Mval==LOW))
    {
      analogWrite(LEDG, 0);
      analogWrite(LEDR, 255);
      analogWrite(LEDB, 255);
      delay(1000);
      analogWrite(LEDR,0);
      analogWrite(LEDB, 0);
      delay(1000);
    }
  }
  if(M==3)
  {
    if((Cval==LOW) && (Mval==LOW))
    {
      for(P=0;P<255;P++)
      {
        analogWrite(LEDR, P);
        analogWrite(LEDB, P);
        delay(10);
      }
      for(P=255;P>0;P--)
      {
        analogWrite(LEDR, P);
        analogWrite(LEDB, P);
        delay(10);
      }
    }
  }
}
}
/////////////CYAN/////////
void CYAN()
{
  if(C==6)
  {
    if(M==1)
    {
      analogWrite(LEDR, 0);
      analogWrite(LEDB, 255);
      analogWrite(LEDG, 255);
    }
    if(M==2)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
        analogWrite(LEDR, 0);
        analogWrite(LEDG, 255);
        analogWrite(LEDB, 255);
        delay(1000);
        analogWrite(LEDG, 0);
        analogWrite(LEDB, 0);
        delay(1000);
      }
    }
    if(M==3)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
        for(Cy=0;Cy<255;Cy++)
        {
          analogWrite(LEDG, Cy);
          analogWrite(LEDB, Cy);
          delay(10);
        }
        for(Cy=255;Cy>0;Cy--)
        {
          analogWrite(LEDG, Cy);
          analogWrite(LEDB, Cy);
          delay(10);
        }
      }
    }
  }
}
//////////////WHITE///////////
void WHITE()
{
  if(C==7)
  {
    if(M==1)
    {
      analogWrite(LEDG, 255);
      analogWrite(LEDR, 255);
      analogWrite(LEDB, 255);
    }
    if(M==2)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
        analogWrite(LEDG, 255);
        analogWrite(LEDB, 255);
        analogWrite(LEDR, 255);
        delay(1000);
        analogWrite(LEDG, 0);
        analogWrite(LEDB, 0);
        analogWrite(LEDR, 0);
        delay(1000);
      }
    }
    if(M==3)
    {
      if((Cval==LOW) && (Mval==LOW))
      {
        for(W=0;W<255;W++)
        {
          analogWrite(LEDR, W);
          analogWrite(LEDB, W);
          analogWrite(LEDG, W);
          delay(10);
        }
        for(W=255;W>0;W--)
        {
          analogWrite(LEDR, W);
          analogWrite(LEDG, W);
          analogWrite(LEDB, W);
          delay(10);
        }
      }
    }
  }
}
///////////////COLOR CONTROL///////////

void CMLED()
{
///////////////OFF///////////
  if(C==0)
  {
    analogWrite(LEDR, 0);
    analogWrite(LEDG, 0);
    analogWrite(LEDB, 0);
  }
  else
  {
    RED();
    GREEN();
    BLUE();
    YELLOW();
    PURPLE();
    CYAN();
    WHITE();
  }
}
///////////////////////


////////////PIN SETUP///////////////////
void setup() 
{
pinMode(LEDB, OUTPUT);
pinMode(LEDG, OUTPUT);
pinMode(LEDR, OUTPUT);
pinMode(CBUTTON, INPUT);
pinMode(MBUTTON, INPUT);
Serial.begin(57600);
}
//////////////////////////////

/////////////BUTTON CONTROL/////////////
void loop() 
{
Cval = digitalRead(CBUTTON);
Mval = digitalRead(MBUTTON);



if((Cval==HIGH) && (C < 7))
{
 C = C + 1;
 delay(2000);
Serial.print(C);
}
Cval = digitalRead(CBUTTON);
Mval = digitalRead(MBUTTON);
if((Cval==HIGH) && (C == 7))
{
 C = 0;
delay(2000);
 Serial.print(C);
}
Cval = digitalRead(CBUTTON);
Mval = digitalRead(MBUTTON);
if((Mval==HIGH) && (M < 3))
{
  M = 1 + M;
  delay(2000);
Serial.print(M);
}
Cval = digitalRead(CBUTTON);
Mval = digitalRead(MBUTTON);
if((Mval==HIGH) && (M == 3))
{
  M = 1;
  delay(2000);
Serial.print(M);
}
CMLED();
}
/////////////////////////////

Moderator edit: BLOODY CODE TAGS, AGAIN.

Look into using "switch case" https://www.arduino.cc/en/Reference/SwitchCase

Using delay stops your code execution for the delay amount of time. look into BWD blink without delay, an example comes with the IDE. Have you ever used State Machine coding? http://www.thebox.myzen.co.uk/Tutorial/State_Machine.html

  1. As the moderator wrote, learn to use code tags when posting your code.
  2. As LarryD said, learn to code using Finite State Machines (FSM), because this will help you with…
  3. Learn to code without using delay(…) which is evil except for the most trivial of programs. Use unsigned long variables and millis() instead, which is going to involve you in FSMs.
  4. Learn to use longer names, and the C++ enum. For example, M should probably be something like colorMode, and C should be something else as well. Instead of 1, 2, and 3, you should be using an enum, which will allow you to use a name instead of a number. See Enums - C++ Forum
  5. As LarryD said, learn to use switch and case statements. This makes it a LOT easier to code FSMs.
  6. Learn to do things when a switch goes from unoperated to operated, not just WHILE it is operated. This way, you will be able to eliminate some of the delay(…) statements that you have.

When you have done all of this and your program is working again - just as it is now - write back again so we can help further with the first three things that you requested.

What LarryD called BWD is often called BWOD (Blink WithOut Delay).

Hello LarryD and vaj4088 thank you for the advice I have gone and edited my code and added enums, switch/case statements, and removed most of the delays (didn’t remove some ones that help the code work). I however didn’t entirely understand FSMs entirely, but i did end up getting it to working where I don’t need to hold the button down until the mode cycle finishes.
One question I still have though is how I would make it fade/blink a random color.

Here is the code:

//////////ENUMERATIONS///////////////
enum ColorType {Red, Green, Blue, Yellow, Purple, Cyan, White};
enum ModeType {Off, On, Blinking, Fading};
//////////////PIN VARIABLES///////////
const int LEDB = 11;
const int LEDG = 10;
const int LEDR = 9;
const int CBUTTON = 7;
const int MBUTTON = 4;
///////////////BLINKING VARIABLES////////////
int LEDState = 0;
unsigned long previousMillis = 0;
const long interval = 1000;
///////////COLOR FADING VARIABLES////////////
int Fade = 0;
const long fadePeriod = 2000;
long fadeTimer = 0;
///////////////BUTTON PRESS VARIABLES////////////
int Cval = 0;
int oldCval = 0;
ColorType Color = Red;
ModeType Mode = Off;
int Mval = 0;
int oldMval = 0;
int colorSelection = 1;
int modeSelection = 0;
////////////////////////////////

///////////////LED CONTROL///////////
void CMLED()
{
  unsigned long currentMillis = millis();
switch (Mode)
{
  case Off:     //LED OFF
  analogWrite(LEDR, 0);
  analogWrite(LEDG, 0);
  analogWrite(LEDB, 0);
  break;
  case On:     //LED MODE ON
  switch(Color)
  {
    case Red:
    analogWrite(LEDR, 255);
    analogWrite(LEDG, 0);
    analogWrite(LEDB, 0);
    break;
    case Green:
    analogWrite(LEDR, 0);
    analogWrite(LEDG, 255);
    analogWrite(LEDB, 0);
    break;
    case Blue:
    analogWrite(LEDR, 0);
    analogWrite(LEDG, 0);
    analogWrite(LEDB, 255);
    break;
    case Yellow:
    analogWrite(LEDB, 0);
    analogWrite(LEDR, 255);
    analogWrite(LEDG, 255);
    break;
    case Purple:
    analogWrite(LEDR, 255);
    analogWrite(LEDB, 255);
    analogWrite(LEDG, 0);
    break;
    case Cyan:
    analogWrite(LEDR, 0);
    analogWrite(LEDB, 255);
    analogWrite(LEDG, 255);
    break;
    case White:
    analogWrite(LEDG, 255);
    analogWrite(LEDR, 255);
    analogWrite(LEDB, 255);
    break;
  }
  break;
  case Blinking:      //LED MODE BLINKING
  switch(Color)
  {
    case Red:
    analogWrite(LEDG, 0);
    analogWrite(LEDB, 0);
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState == 0)
      {
       LEDState = 255;
     }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDR, LEDState);
    }
    break;
    case Green:
    analogWrite(LEDR, 0);
    analogWrite(LEDB, 0);
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState == 0)
      {
       LEDState = 255;
     }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDG, LEDState);
    }
    break;
    case Blue:
    analogWrite(LEDG, 0);
    analogWrite(LEDR, 0);
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState ==0)
      {
       LEDState = 255;
      }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDB, LEDState);
    }
    break;
    case Yellow:
    analogWrite(LEDB, 0);
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState==0)
      {
        LEDState = 255;
      }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDR, LEDState);
      analogWrite(LEDG, LEDState);
    }
    break;
    case Purple:
    analogWrite(LEDG, 0);
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState==0)
      {
        LEDState = 255;
      }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDR, LEDState);
      analogWrite(LEDB, LEDState);
    }
    break;
    case Cyan:
    analogWrite(LEDR, 0);
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState==0)
      {
        LEDState = 255;
      }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDG, LEDState);
      analogWrite(LEDB, LEDState);
    }
    break;
    case White:
    if(currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;
      if(LEDState==0)
      {
        LEDState = 255;
      }
      else
      {
        LEDState = 0;
      }
      analogWrite(LEDR, LEDState);
      analogWrite(LEDG, LEDState);
      analogWrite(LEDB, LEDState);
    }
    break;
  }
  break;
  case Fading:        //LED MODE FADING
  switch(Color)
  {
    case Red:
    analogWrite(LEDG, 0);
    analogWrite(LEDB, 0);
    fadeTimer = millis();
    Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer);
    analogWrite(LEDR, Fade);
    break;
      case Green:
      analogWrite(LEDR, 0);
      analogWrite(LEDB, 0);
      fadeTimer = millis();
      Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer);
      analogWrite(LEDG, Fade);
      break;
        case Blue:
        analogWrite(LEDG, 0);
        analogWrite(LEDR, 0);
        fadeTimer = millis();
        Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer);
        analogWrite(LEDB, Fade);
        break;
    case Yellow:
    analogWrite(LEDB, 0);
    fadeTimer = millis();
    Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer); 
    analogWrite(LEDG, Fade);
    analogWrite(LEDR, Fade);
    break;
      case Purple:
      analogWrite(LEDG, 0);
      fadeTimer = millis();
      Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer);
      analogWrite(LEDB, Fade);
      analogWrite(LEDR, Fade);
      break;
        case Cyan:
        analogWrite(LEDR, 0);
        fadeTimer = millis();
        Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer);
        analogWrite(LEDB, Fade);
        analogWrite(LEDG, Fade);
        break;
      case White:
      fadeTimer = millis();
      Fade = 128+127*cos(2*PI/fadePeriod*fadeTimer);
      analogWrite(LEDB, Fade);
      analogWrite(LEDG, Fade);
      analogWrite(LEDR, Fade);
      break;
  }
  break;
  
}
}
///////////////////////

/////////BUTTON CONTROL//////////
void COLORBUTTONPRESS()
{
  if((Cval==HIGH) && (oldCval==LOW))
{
  if(colorSelection<7)
  {
    colorSelection = colorSelection + 1;
    Serial.print(colorSelection);
  }
  else
  {
    colorSelection = 1;
    Serial.print(colorSelection);
  }
  delay(10);
}
oldCval = Cval;
if((Cval==LOW) && (oldCval==HIGH))
{
  delay(10);
}
}

void MODEBUTTONPRESS()
{
   if((Mval==HIGH) && (oldMval==LOW))
{
  if(modeSelection<3)
  {
    modeSelection = modeSelection + 1;
    Serial.print(modeSelection);
  }
  else
  {
    modeSelection = 0;
    Serial.print(modeSelection);
  }
  delay(10);
}
oldMval = Mval;
if((Mval==LOW) && (oldMval==HIGH))
{
  delay(10);
} 
}
/////////////////////////////

/////////////VARIABLE TO COLOR/MODE///////////////
void CMSELECTION()
{
  switch(colorSelection)
  {
    case 1:
    Color = Red;
    break;
    case 2:
    Color = Green;
    break;
    case 3:
    Color = Blue;
    break;
    case 4:
    Color = Yellow;
    break;
    case 5:
    Color = Purple;
    break;
    case 6:
    Color = Cyan;
    break;
    case 7:
    Color = White;
    break;
  }
  switch(modeSelection)
  {
    case 0:
    Mode = Off;
    break;
    case 1:
    Mode = On;
    break;
    case 2:
    Mode = Blinking;
    break;
    case 3:
    Mode = Fading;
    break;
  }
}
////////////PIN SETUP///////////////////
void setup() 
{
pinMode(LEDB, OUTPUT);
pinMode(LEDG, OUTPUT);
pinMode(LEDR, OUTPUT);
pinMode(CBUTTON, INPUT);
pinMode(MBUTTON, INPUT);
Serial.begin(57600);
}
//////////////////////////////

/////////////MAIN CONTROL/////////////
void loop() 
{
Cval = digitalRead(CBUTTON);
Mval = digitalRead(MBUTTON);


MODEBUTTONPRESS();
COLORBUTTONPRESS();
CMSELECTION();
CMLED();
}
/////////////////////////////

Wow, that was fast!

Arduino has a function for generating random numbers but I have not used it. This is probably what you want to use. Please see https://www.arduino.cc/en/Reference/Random for more information about the random(…) function.

You may want to learn about arrays also. An array can serve as a mapping from a particular random number to the values needed for your LEDs to present this color.

You have not explained how you want to do this. Is it another press of the color button? Is it another press of the mode button?

Once you know what you want to do, it will probably be easy to add it to the FSM for that button.

Keep up the good work, and Good Luck!

Something to study and think about.

Uses arrays, struct’s, arrays of structs, subroutines and passes colors by reference to subroutine.

These features will reduce the amount of code consideerably if understood and used.

// <http://forum.arduino.cc/index.php?topic=369374.0>

#define COUNT_ENTRIES(ARRAY)    (sizeof(ARRAY) / sizeof(ARRAY[0]))

const uint8_t   pinLED_B        = 11;
const uint8_t   pinLED_G        = 10;
const uint8_t   pinLED_R        = 9;

const uint8_t   pinsLED[]       = { pinLED_B, pinLED_G, pinLED_R };

const uint8_t   LED_OFF         = LOW;
const uint8_t   LED_ON          = HIGH;

const unsigned long tmsSECOND   = 1000UL;

struct rgb_t
{
    // ANONYMOUS UNION, USING ANONYMOUS STRUCT
    union
    {
        uint8_t               v[3];
        struct  { uint8_t     r, g, b; };
    };
};

const struct rgb_t  rgbRED      = { 0xFF, 0x00, 0x00 };
const struct rgb_t  rgbGREEN    = { 0x00, 0xFF, 0x00 };
const struct rgb_t  rgbBLUE     = { 0x00, 0x00, 0xFF };
const struct rgb_t  rgbYELLOW   = { 0x00, 0xFF, 0xFF };
const struct rgb_t  rgbPURPLE   = { 0xFF, 0xFF, 0x00 };
const struct rgb_t  rgbCYAN     = { 0x00, 0xFF, 0xFF };
const struct rgb_t  rgbWHITE    = { 0xFF, 0xFF, 0xFF };

struct rgb_t rgbCOLORS[] =
{
      rgbRED
    , rgbGREEN
    , rgbBLUE
    , rgbYELLOW
    , rgbPURPLE
    , rgbCYAN
    , rgbWHITE 
};

void setLED(struct rgb_t& rgb)
{
    const size_t    NUM_LEDS = COUNT_ENTRIES(rgb.v);
    
    for ( size_t i = 0; i < NUM_LEDS; i++ )
    {
        analogWrite(pinsLED[i], rgb.v[i]);
    }
}

void loop()
{
    const size_t    NUM_COLORS = COUNT_ENTRIES(rgbCOLORS);
    
    for ( size_t i = 0; i < NUM_COLORS; i++ )
    {
        rgb_t   rgb = rgbCOLORS[i];

        setLED(rgb);

        delay(tmsSECOND);
    }
}

void setup()
{
    const size_t    NUM_LEDS    = COUNT_ENTRIES(pinsLED);
    
    for ( size_t i = 0; i < NUM_LEDS; i++ )
    {
        pinMode(pinsLED[i], OUTPUT);
    }
}

This is the code I came up with by adding a “Random” mode as mode 4, I didn’t add any arrays though because I tried watching tutorials on them but a lot of them were confusing and didn’t help out much.

(I attached the code because it is to long, and has over 9,000 characters)

RGB_LED_Super_Control.ino (9.43 KB)