How can I regroup the "if" functions i one?

My code is supposed to control the RGB leds and it works as I wish but is there any solution to make it shorter? I mean the 3 "if" functions. It is possible to regroup them in one "box"? because bassicly they are doing the same thing but with differents variables

#define red 7                     //Define the button pin for red colour
#define green 8                   //Define the button pin for green colour
#define blue 9                    //Define the button pin for blue colour
const int AnalogRED = A0;
const int AnalogGREEN = A1;
const int AnalogBLUE = A2;
unsigned char R = 70;             //Define the initial value for red colour
unsigned char G = 70;             //Define the initial value for green colour
unsigned char B = 70;             //Define the initial value for blue colour

int ColorSwitchRetard = 1500;     //delay before switching the colour off 
int BrightnessSpeed = 50;         //delay between making the led brighter

void setup() {
  Serial.begin(9600);
  
  
  pinMode(AnalogRED, OUTPUT);     //Analog output for the red colour
  pinMode(AnalogGREEN, OUTPUT);   //Analog output for the green colour
  pinMode(AnalogBLUE, OUTPUT);    //Analog output for the blue colour



}

void loop() {
  int RedState = digitalRead(red);          //Read the digital 7 pin state
  int GreenState = digitalRead(green);      //Read the digital 8 pin state
  int BlueState = digitalRead(blue);        //Read the digital 9 pin state



        if (RedState == HIGH) {             //If the button for the red colour is pressed
          R=R+1;                            //Make a sum of the final red value that will be send by the analog pin 0
          Serial.println("Red: ");
          Serial.println(R);
          analogWrite(AnalogRED, R);        //Send the current value of red colour
          delay(BrightnessSpeed);           //Delay before adding another R++
            if(R>254){                      //If the value of the colour is greater than 254
              delay(ColorSwitchRetard);     //Delay before switching off
              R=70;                         //Set the inital value 
              }
        }
      
        else if (GreenState == HIGH) {      //Same thing that abowe but in green
          G=G+1;
          Serial.println("Green: ");
          Serial.println(G);
          analogWrite(AnalogGREEN, G);
          delay(BrightnessSpeed);
          if(G>254){
            delay(ColorSwitchRetard);
            G=70;
              }
        }
      
      
        else if (BlueState == HIGH) {       //Same thing that abowe but in blue
          B=B+1;
          Serial.println("Blue: ");
          Serial.println(B);
          analogWrite(AnalogBLUE, B);
          delay(BrightnessSpeed);
          if(B>254){
            delay(ColorSwitchRetard);
            B=70;
              }
        }







}

Im open to criticism aswell so tell me what would you improve in this code (Im basicly beggining my adventure with arduino).

A little explanation for those who are curious: the goal is to make a little controller for rgb leds. So I'll control the colour with my 3 buttons. As I press one (for red for example) it increases the intensity of the colour. I put the initial value at 70 because it doesn't fire up before it hits 126 so I thought I'll accelerate this process a bit.

Skiermaxhtc:
My code is supposed to control the RGB leds and it works as I wish but is there any solution to make it shorter? I mean the 3 "if" functions. It is possible to regroup them in one "box"? because bassicly they are doing the same thing but with differents variables

An if statement is not a function. But a function is exactly what you want. Now because a led is defined by its intensity and the pin it is connected to, you should group these informations together using a struct:

struct color_led_t
{
  unsigned char intensity;
  int pin;
};

To initialize the three leds (initialize a struct):

color_led_t redLed = {70, LED_RED_PIN};
color_led_t greenLed = {70, LED_GREEN_PIN};
color_led_t blueLed = {70, LED_BLUE_PIN};

To change the intensity of any led, you create a function, which accepts a reference to an existing led:

void increaseLEDIntensity(color_led_t& led)
{
  led.intensity++;               
  analogWrite(led.pin, led.intensity);        
  delay(brightnessSpeed);        
  if (led.intensity > 254) 
  {                
    delay(colorSwitchRetard);    
    led.intensity = 70;                      
  }
}

And in your loop:

if (RedState == HIGH)
  {
    Serial.println("Red: ");
    Serial.println(redLed.intensity);
    increaseLEDIntensity(redLed);
  }
  else if (GreenState == HIGH)
  {
    Serial.println("Green: ");
    Serial.println(greenLed.intensity);
    increaseLEDIntensity(greenLed);
  }
  else if (BlueState == HIGH)
  {
    Serial.println("Blue: ");
    Serial.println(blueLed.intensity);
    increaseLEDIntensity(blueLed);
  }

Skiermaxhtc:
Im open to criticism aswell so tell me what would you improve in this code (Im basicly beggining my adventure with arduino).

As you might have noticed, I renamed some of your variable names and defines. Defines are mostly all upper case letters, e.g. LED_RED_PIN. If the value it is associated with is a pin, then put pin in the name. Variables start with a lower case letter, because classes start with an upper case letter. I've highlighted some keywords for you to look up, if you're not familiar with them.

Full code (compiled but not tested):

#define BUTTON_PIN_RED    7
#define BUTTON_PIN_GREEN  8
#define BUTTON_PIN_BLUE   9

#define LED_RED_PIN       A0
#define LED_GREEN_PIN     A1
#define LED_BLUE_PIN      A2

int colorSwitchRetard = 1500;
int brightnessSpeed = 50;

struct color_led_t
{
  unsigned char intensity;
  int pin;
};

color_led_t redLed = {70, LED_RED_PIN};
color_led_t greenLed = {70, LED_GREEN_PIN};
color_led_t blueLed = {70, LED_BLUE_PIN};

void setup() {
  Serial.begin(9600);
  
  pinMode(LED_RED_PIN, OUTPUT);     
  pinMode(LED_GREEN_PIN, OUTPUT);
  pinMode(LED_BLUE_PIN, OUTPUT);
}

void increaseLEDIntensity(color_led_t& led)
{
  led.intensity++;               
  analogWrite(led.pin, led.intensity);        
  delay(brightnessSpeed);        
  if (led.intensity > 254) {                
    delay(colorSwitchRetard);    
    led.intensity = 70;                      
  }
}

void loop() {
  int RedState = digitalRead(BUTTON_PIN_RED);          //Read the digital 7 pin state
  int GreenState = digitalRead(BUTTON_PIN_GREEN);      //Read the digital 8 pin state
  int BlueState = digitalRead(BUTTON_PIN_BLUE);        //Read the digital 9 pin state

  if (RedState == HIGH)
  {
    Serial.println("Red: ");
    Serial.println(redLed.intensity);
    increaseLEDIntensity(redLed);
  }
  else if (GreenState == HIGH)
  {
    Serial.println("Green: ");
    Serial.println(greenLed.intensity);
    increaseLEDIntensity(greenLed);
  }
  else if (BlueState == HIGH)
  {
    Serial.println("Blue: ");
    Serial.println(blueLed.intensity);
    increaseLEDIntensity(blueLed);
  }
}

There is a way but it’s a bit more advanced a concept. A thing in C called a “structure” is good for this sort of problem. It allows you to group many different things (ints, char *s etc) into a structure that you can then make a variable type.

In your case, such a structure might look like this:

typedef struct 
    {
        int ColorState;
        char *szStr;
        int nColor;
        int pinColor;
        
    } ComparoStruct;

In this example, ColorState corresponds to your input pin readings RedState, GreenState and BlueState.
The “char *szStr” is a pointer to a character string; it supplies the fixed string you print in each case (e.g. "Red: "). nColor is the R, G and B variables and pinColor is the pin used in analogWrite for each color.

A variable can be made from this structure because of the “typedef” – type define. In fact, it can be made an array of size 3, corresponding to each of your colors:

ComparoStruct Comparo[3] = 
    { ...

This variable can be initialized for each of the R, G and B colors like:

ComparoStruct Comparo[3] = 
    {
        {
            .ColorState = LOW,
            .szStr = "Red: ",
            .nColor = R,
            .pinColor = AnalogRED
        },
        {
            .ColorState = LOW,
            .szStr = "Green: ",
            .nColor = G,
            .pinColor = AnalogGREEN            
        },
        {
            .ColorState = LOW,
            .szStr = "Blue: ",
            .nColor = B,
            .pinColor = AnalogBLUE            
        }
    };

Now you can loop through the three colors in this variable using this:

   //go through the three colors in variable array Comparo 
    for( int i=0; i<3; i++ )
    {
        //for i=0, this would be the same as "if (RedState == HIGH)..."
        //for i=1, this would be the same as "if (GreenState == HIGH)..."
        //for i=2, this would be the same as "if (BlueState == HIGH)..." 
        if( Comparo[i].ColorState == HIGH )
        {...

Put together, the code looks like this

#define red 7                     //Define the button pin for red colour
#define green 8                   //Define the button pin for green colour
#define blue 9                    //Define the button pin for blue colour

const int AnalogRED = A0;
const int AnalogGREEN = A1;
const int AnalogBLUE = A2;

unsigned char R = 70;             //Define the initial value for red colour
unsigned char G = 70;             //Define the initial value for green colour
unsigned char B = 70;             //Define the initial value for blue colour

#define RED_IDX     0
#define GRN_IDX     1
#define BLU_IDX     2


//what are we doing each loop:
//- comparing RGB state against a boolean level
//- printing a const string and then an RGB variable
//- writing to an analog pin that variable
//- delaying a fixed value (BrightnessSpeed)
//- comparing that value to 254
//-- if greater than add futher delay
//-- reset variable to 70

typedef struct 
    {
        int ColorState;
        char *szStr;
        int nColor;
        int pinColor;
        
    } ComparoStruct;

ComparoStruct Comparo[3] = 
    {
        {
            .ColorState = LOW,
            .szStr = "Red: ",
            .nColor = R,
            .pinColor = AnalogRED
        },
        {
            .ColorState = LOW,
            .szStr = "Green: ",
            .nColor = G,
            .pinColor = AnalogGREEN            
        },
        {
            .ColorState = LOW,
            .szStr = "Blue: ",
            .nColor = B,
            .pinColor = AnalogBLUE            
        }
    };

const int ColorSwitchRetard = 1500;     //delay before switching the colour off 
const int BrightnessSpeed = 50;         //delay between making the led brighter

//what's driving red/green/blue inputs?
//if normally open type momentary switch (e.g.) uncomment this to enable pullups
//if actively driven or active level is high, leave commented and pins will just
//be "inputs"
//#define NORMALLY_OPEN   1   

void setup() 
{
    Serial.begin(9600);

#ifdef NORMALLY_OPEN
    pinMode( red, INPUT_PULLUP );
    pinMode( green, INPUT_PULLUP );
    pinMode( blue, INPUT_PULLUP );
#else
    pinMode( red, INPUT );
    pinMode( green, INPUT );
    pinMode( blue, INPUT );
#endif
    pinMode(AnalogRED, OUTPUT);     //Analog output for the red colour
    pinMode(AnalogGREEN, OUTPUT);   //Analog output for the green colour
    pinMode(AnalogBLUE, OUTPUT);    //Analog output for the blue colour

}//setup

void loop() 
{
    //read the pin states
    Comparo[RED_IDX].ColorState = digitalRead(red);          //Read the digital 7 pin state
    Comparo[GRN_IDX].ColorState = digitalRead(green);      //Read the digital 8 pin state
    Comparo[BLU_IDX].ColorState = digitalRead(blue);        //Read the digital 9 pin state

    //go through the three colors in variable array Comparo 
    for( int i=0; i<3; i++ )
    {
        //for i=0, this would be the same as "if (RedState == HIGH)..."
        //for i=1, this would be the same as "if (GreenState == HIGH)..."
        //for i=2, this would be the same as "if (BlueState == HIGH)..." 
        if( Comparo[i].ColorState == HIGH )
        {
            //for i=0, this would be the same as "R=R+1"
            //for i=1, this would be the same as "G=G+1"
            //for i=2, this would be the same as "B=B+1"
            //and so on...
            Comparo[i].nColor++;
            Serial.print( Comparo[i].szStr );
            Serial.println( Comparo[i].nColor );
            analogWrite(Comparo[i].pinColor, Comparo[i].nColor);
            //brightnessspeed was not unique to each color like R, G and B and so
            //is fixed (i.e. not a member of that struct)
            delay(BrightnessSpeed);
            if( Comparo[i].nColor > 254 )
            {
                //same with this delay; common to all
                delay(ColorSwitchRetard);
                Comparo[i].nColor = 70;
                
            }//if
                
        }//if
        
    }//for

}//loop

You trade some complexity for less repetitious code.

Edit: I see LightuC was thinking along the same lines. :slight_smile:

Which processor are you using? On the 328P (e.g. Uno), pins that support analogRead (A0…A2) do not support PWM using analogWrite.

Below again similar to previous replies, this time with pointers :slight_smile:

You can use a struct to combine related info; in this case the output pin and the PWM value.

struct LED
{
  byte pin;
  byte value;
};

Next you can create an array of those structs

LED leds[] =
{
  {A0, 70},
  {A1, 80},
  {A2, 90},
};

In setup, you can loop through the array to set the pins to output

 for (byte cnt = 0; cnt < sizeof(leds) / sizeof(leds[0]); cnt++)
  {
    pinMode(leds[cnt].pin, OUTPUT);
  }

Write a function to incrememnt the colour brightness

void incBrightness(struct LED *led)
{
  led->value++;
  analogWrite(led->pin, led->value);
  Serial.println(led->value);
}

This function take a pointer to a struct. It increments the value, write the value to the pin and prints the new value.

To keep the code readable, add the following at the top of your code

// indices in led array
#define REDINDEX 0
#define GREENINDEX 1
#define BLUEINDEX 2

Next in loop you call the function with as parameter the address of the element for the colour. Note that this is only demo.

void loop()
{
 Serial.println("RED");
 incBrightness(&leds[REDINDEX]);
 Serial.println("GREEN");
 incBrightness(&leds[GREENINDEX]);
 Serial.println("BLUE");
 incBrightness(&leds[BLUEINDEX]);

 delay(1500);
}

sterretje:
This function take a pointer to a struct. [...]

This is just my opinion and there are obviously a lot of cases where pointers are the best option, but: For function parameters, intending to change the parameters content, a reference is the better option (unless you want to do pointer arithmetic). Because when the function takes a pointer, I can do this:

incBrightness(0);

It will compile and it is valid code, but this will cause unknown behaviour, because now it is overriding the value stored at address 1 (since value is the second struct member). In this special case, it might also change the output of another analog pin, if the content of address 0 happens to be a valid pin number for the analog function to use. References however are not allowed to contain a null pointer, so the compiler will prevent this from happening. I just wanted to point that out, so Skiermaxhtc might get the full picture.

Or you could skip using structs, and use their (only very slightly) smarter siblings, classes.

Skiermaxhtc:
My code is supposed to control the RGB leds and it works as I wish but is there any solution to make it shorter?

Write code for clarity, not brevity.

Using a struct or a class can add clarity.

...R

Thanks a lot for replying guys ^^ Indeed @sterretje I knew something is wrong so I moved my led pins to digital PWM.

However there is one more thing that I would improve.

As the intensity of the led hits 255 it stops for a while to let go the button. I want to to the same when the value hits 0 anew so the red won't mix with the blue for example. I tried various things but I didn't achieve the goal.

if (led.intensity > 254) {
    delay(colorSwitchRetard);
  }

  else if (led.intensity = 0){

    delay(colorSwitchRetard);
    }

I tried this but as I press the button it continously set the led.intensity at 0.

if (led.intensity = 0){ Oops

Nevermind I just found a solution :smiley:

if (led.intensity < 1){
    delay(colorSwitchRetard);
    led.intensity = led.intensity + 1;
    }

I don’t know if I made it propely but it works anyway ^^

Thanks you guys!

Skiermaxhtc:
Nevermind I just found a solution :smiley:

if (led.intensity < 1){

delay(colorSwitchRetard);
    led.intensity = led.intensity + 1;
    }




I don't know if I made it propely but it works anyway ^^

Thanks you guys!

See AWOL’s reply. To elaborate

= is an assigment
== is a compare