Multiple sets of "if" statements?

I am trying to make a code where there are if statements and when you press a button, if some things are true, then a green light goes on. I also want it to turn a red light on when the same button is pressed but other things are true. So far, when I tried it it only followed the first set and totally disregarded the second one. Please help! :slight_smile:

Show us the code.
We're not psychic!

You probably want to do something like this:

if (something == true && something-else == true && ... )

The && stand for and, so you can create an if statement which will only execute something if all of a certain group of parameters are met.

   if (val == val2) {
     if (val != buttonstate) {
       if (val == LOW) {
         if (redvalue + 30 >= redVal) {
           if (redvalue - 30 <= redVal) {
             if (bluevalue + 30 >= blueVal) {
               if (bluevalue - 30 <= blueVal) {
                 if (greenvalue + 30 >= greenVal) {
                   if (greenvalue - 30 <= greenVal) {
   
         digitalWrite(led, HIGH);
                
               if (val == val2) {
     if (val != buttonstate) {
       if (val == LOW) {
         if (redvalue + 30 <= redVal || redvalue - 30 >= redVal || greenvalue + 30 <= greenVal || greenvalue - 30 >= greenVal || bluevalue + 30 <= blueVal || bluevalue - 30 >= blueVal) {
         digitalWrite(ledtwo, HIGH);

This is the chunk of code I am trying to get to work, it only responds to the first part but does nothing with the second part.

Ouch. Read what I said again.

I havn't been coding for very long (like 4 days ) but that loks really confusing to me, maybe it's because i don't see the rest or how the other variables are being derived i can't see the flow. any whoooo

IMHO it looks like you only want to turn on 'led' if all of the first statements are true.. in which case jezuz has it pegged, work it just like you set up the || in the bottom statement for 'ledtwo' but use && instead.

Just curious...

how can 'redvalue + 30' be >= to 'redVal' just befor you are testing to see if 'redvalue - 30' is <= to 'redVal' with nothing in between to change either of those values? same goes for the other values for each color and both LED's.... like i said maybe it's because i can't see the rest of the code but i can't seem to understand your flow.

Really quite hard to say given the fragment you posted: you omitted every closing brace... That said you can probably simplify a fair bit of the logic

   if (val == val2) {
     if (val != buttonstate) {

...is equivalent to

   if ( (val == val2) && (val != buttonstate)  ) {

...unless there is an else on that first if.

When you start nesting if()s that deeply then it is a good indication that you haven't thought your program's logic flow and structure through far enough.

For logic flow: go ahead and draw a flowchart of the desired behavior. If your logic really is that hairy then you'll be glad of the time spent doing so when you get to implementing. If it isn't then it will become apparent that you went overboard on the if() statements and you'll be glad of the time not spent troubleshooting that mess.

For program structure:

  • Pick USEFUL variable names.
 if (redvalue + 30 >= redVal) {

This is quite meaningless -- why are you comparing redvalue to redVal? They're both red values! What the heck does this mean? How is anyone supposed to know how and why redvalue and redVal differ? It makes no sense.

  • Factor out repetitive portions and put them into functions
         if (redvalue + 30 >= redVal) {
           if (redvalue - 30 <= redVal) {
             if (bluevalue + 30 >= blueVal) {
               if (bluevalue - 30 <= blueVal) {
                 if (greenvalue + 30 >= greenVal) {
                   if (greenvalue - 30 <= greenVal) {

I have no idea why you're doing these +/- 30 tests. The code doesn't express the meaning behind this. But something like...

int isValueInBounds( value, midPoint )
{ 
   if( (value+30>=midPoint) && (value-30<=midPoint) 
   {
       return true;
   }
   return false;
}

...could be reused in the function you pasted like:

if( isValueInBounds( redvalue, redval ) 
{ 
   // whatever you're supposed to do if the condition is met 
}

Good luck!

I don't care what the values are named and about everything else. I just want to know how to make both the parts work because no matter what, led2 will never go on but if the criteria is met, the first led will.

Although it may be clear to you, its difficult to tell from the information you posted what you want your code to do.

Can you say in words what your application is and how you want your logic to work. Don't worry about the syntax, just describe the specific conditions that will cause the lights to go on and off.

I don't care what the values are named and about everything else. I just want to know how to make both the parts work because no matter what, led2 will never go on but if the criteria is met, the first led will.

Without the full code you're trying to get working and a thorough description of what you're trying to accomplish no one here is going to be able to help you. Based on the fragment you provided I offered some suggestions on how you might get to the solution you're looking for. If that's not enough to get you going then you have two real options. 1) Post full code and a description of what you want to do -or- 2) break the problem down into the smallest thing you don't know how to do, post full code and a description for that and ask a specific question about that.

But right now no one can help you because no one seems to understand what your problem is because you've asked broad and vague questions with fragmentary code to examine.

Okay, sorry guys. Here is the full code:

int ledtwo = 8; 
int led = 7;

int redvalue;
int greenvalue;
int bluevalue;

int buttonstate;
int val;
int val2; 
int switchpin = 2;

int redPin = 3;
int bluePin = 5;
int greenPin = 6;

int redIn = 2;
int greenIn = 3;
int blueIn = 4;

int redVal;
int greenVal;
int blueVal;

#ifndef RGBCommonLED_H
#define RGBCommonLED_H

#include "WProgram.h"


class RGBCommonLED {
public:
  RGBCommonLED(int rpin, int gpin, int bpin);
  ~RGBCommonLED();

  void setRGB(byte r, byte g, byte b);
  void setHSV(byte h, byte s, byte v);
  void setColor(unsigned long v);
  void setHue(byte level);
  void setSat(byte level);
  void setVal(byte level);

  void adjHue(int level);
  void adjSat(int level);
  void adjVal(int level);

  void setRandomColor();
  void setRandomHue();
  void setRandomSat();
  void setRandomVal();

  void setHSVLevel(int section, byte level);
  void adjHSVLevel(int section, int level);
  void setRandomHSVLevel(int section);

  void updateLED();

  void HSVToRGB( unsigned int inHue, unsigned int inSaturation, unsigned int inValue,
    unsigned int *oR, unsigned int *oG, unsigned int *oB );

  void RGBToHSV( unsigned int inRed, unsigned int inGreen, unsigned int inBlue,
    unsigned int *oH, unsigned int *oS, unsigned int *oV );


  void updateHSVFromRGB();


  int pins[3];

  int rgb[3];

  int hsv[3];

private:
 
  boolean hsvSet;
  void setRGBFromHSV();
  void setHSVFromRGB();
};

#endif



RGBCommonLED::RGBCommonLED(int rpin, int gpin, int bpin){
  pins[0] = rpin;
  pins[1] = gpin;
  pins[2] = bpin;
  hsvSet = false;
}


RGBCommonLED::~RGBCommonLED(){
}

void RGBCommonLED::setColor(unsigned long v)
{
  this->setRGB((v & 255),((v >> 8) & 255),(v >> 16) & 255);
}


void RGBCommonLED::setRGB(byte r, byte g, byte b){
  rgb[0] = r;
  rgb[1] = g;
  rgb[2] = b;
  hsvSet = false;
}

void RGBCommonLED::setHSV(byte h, byte s, byte v){
  hsv[0] = h;
  hsv[1] = s;
  hsv[2] = v;
  this->setRGBFromHSV();
  hsvSet = true;
}


void RGBCommonLED::setHSVLevel(int section, byte level){
  updateHSVFromRGB();
  hsv[section] = level;
  this->setRGBFromHSV();
}

void RGBCommonLED::setHue(byte level){
  this->setHSVLevel(0,level);
}

void RGBCommonLED::setSat(byte level){
  this->setHSVLevel(1,level);
}

void RGBCommonLED::setVal(byte level){
  this->setHSVLevel(2,level);
}

void RGBCommonLED::adjHSVLevel(int section, int level){
  updateHSVFromRGB();
  int th = hsv[section] + level;

  if( th < 0 ){
    th = 255 + th;
  }
  else if( th > 255 ) {
    th = 255 - th;
  }

  th = constrain(th,0,255);
  hsv[section] = th;
  this->setRGBFromHSV();
}

void RGBCommonLED::adjHue(int level){
  this->adjHSVLevel(0,level);
}

void RGBCommonLED::adjSat(int level){
  this->adjHSVLevel(1,level);
}

void RGBCommonLED::adjVal(int level){
  this->adjHSVLevel(2,level);
}



void RGBCommonLED::RGBToHSV( unsigned int inRed, unsigned int inGreen, unsigned int inBlue,
unsigned int *oH, unsigned int *oS, unsigned int *oV )
{
  double vals[3]; 
  unsigned char maxc=0, minc=0; 
  double hue, sat, val;

  vals[0]=inRed;
  vals[1]=inGreen;
  vals[2]=inBlue;
  //red is set as maximum and minimum
  if(vals[1]>vals[maxc]) maxc=1; 
  if(vals[2]>vals[maxc]) maxc=2; 
  if(vals[1]<vals[minc]) minc=1; 
  if(vals[2]<vals[minc]) minc=2; 
  val = vals[maxc]; 
  if(vals[maxc]==0) 
    sat = hue = 0; 
  else
  { 
    sat=255*(1-(vals[minc]/vals[maxc])); 
    hue = 60 * ((maxc*2) + (vals[(maxc+1)%3] - vals[(maxc+2)%3])/(vals[maxc] - vals[minc])); 
  }
  if(hue < 0) hue += 360; //corrector for hues in -60 to 0 range
  *oH = hue; //map(hue,0,360,0,255);
  *oS = sat;
  *oV = val;
}

void RGBCommonLED::HSVToRGB( unsigned int inHue, unsigned int inSaturation, unsigned int inValue,
unsigned int *oR, unsigned int *oG, unsigned int *oB )
{
  if( inSaturation == 0 )
  {
    // achromatic (grey)
    *oR = *oG = *oB = inValue;
  }
  else
  {
    unsigned int scaledHue = (inHue * 6);
    unsigned int sector = scaledHue >> 8; // sector 0 to 5 around the color wheel
    unsigned int offsetInSector = scaledHue - (sector << 8);      // position within the sector
    unsigned int p = (inValue * ( 255 - inSaturation )) >> 8;
    unsigned int q = (inValue * ( 255 - ((inSaturation * offsetInSector) >> 8) )) >> 8;
    unsigned int t = (inValue * ( 255 - ((inSaturation * ( 255 - offsetInSector )) >> 8) )) >> 8;

    switch( sector ) {
    case 0:
      *oR = inValue;
      *oG = t;
      *oB = p;
      break;
    case 1:
      *oR = q;
      *oG = inValue;
      *oB = p;
      break;
    case 2:
      *oR = p;
      *oG = inValue;
      *oB = t;
      break;
    case 3:
      *oR = p;
      *oG = q;
      *oB = inValue;
      break;
    case 4:
      *oR = t;
      *oG = p;
      *oB = inValue;
      break;
    default:            // case 5:
      *oR = inValue;
      *oG = p;
      *oB = q;
      break;
    }
  }
}

void RGBCommonLED::setRandomColor(){
  this->setColor((unsigned long)random(0x01000000));
}

void RGBCommonLED::setRandomHue(){
  this->setRandomHSVLevel(0);
}
void RGBCommonLED::setRandomSat(){
  this->setRandomHSVLevel(1);
}
void RGBCommonLED::setRandomVal(){
  this->setRandomHSVLevel(2);
}

void RGBCommonLED::setRandomHSVLevel(int section){
  this->setHSVLevel(section, (unsigned int)random(0x0100));
}


void RGBCommonLED::updateHSVFromRGB(){
  this->setHSVFromRGB();
  hsvSet = true;
}
void RGBCommonLED::updateLED(){
  analogWrite(this->pins[0], rgb[0]);
  analogWrite(this->pins[1], rgb[1]);
  analogWrite(this->pins[2], rgb[2]);
  
    Serial.print(" R - ");
Serial.print(rgb[0] , DEC);
Serial.print(" G - ");
Serial.print(rgb[1] , DEC);
Serial.print(" B - ");
Serial.println(rgb[2] , DEC); 

  
  redvalue = rgb[0];
  greenvalue = rgb[1];
  bluevalue = rgb[2];


}

void RGBCommonLED::setRGBFromHSV(){
  unsigned int h = hsv[0];
  unsigned int s = hsv[1];
  unsigned int v = hsv[2];
  unsigned int r = 0;
  unsigned int g = 0;
  unsigned int b = 0;
  HSVToRGB(h,s,v,&r,&g,&b);
  this->setRGB(r,g,b);
}

void RGBCommonLED::setHSVFromRGB(){
  unsigned int r = rgb[0];
  unsigned int g = rgb[1];
  unsigned int b = rgb[2];
  unsigned int h;
  unsigned int s;
  unsigned int v;

  this->RGBToHSV(r,g,b,&h,&s,&v);
  hsv[0] = map(h,0,360,0,255);
  hsv[1] = s;
  hsv[2] = v;
  hsvSet = true;
   
}

RGBCommonLED led1(10,11,9);



void setup()
{
  pinMode(ledtwo, OUTPUT);
  pinMode(led, OUTPUT);
  pinMode(switchpin, INPUT);
  Serial.begin(9600);
  randomSeed(analogRead(0));

  led1.setRandomHue();
  led1.setSat(255);
  led1.setVal(255);
  led1.setRandomColor();
  led1.updateLED();
  
  buttonstate = digitalRead(switchpin);



}

void loop(){

   redVal = analogRead(redIn);
  greenVal = analogRead(greenIn);
  blueVal = analogRead(blueIn);

  
  redVal = map(redVal, 0, 1023, 0, 255);
  greenVal = map(greenVal, 0, 1023, 0, 255);
  blueVal = map(blueVal, 0, 1023, 0, 255);

  analogWrite(redPin, redVal);
  analogWrite(greenPin, greenVal);
  analogWrite(bluePin, blueVal);
  
redVal = 255 - redVal;
blueVal = 255 - blueVal;
greenVal = 255 - greenVal;

 Serial.print("Red is -"); 
Serial.println(redVal, DEC);
 Serial.print("Green is -"); 
Serial.println(greenVal, DEC);
 Serial.print("Blue is -"); 
Serial.println(blueVal, DEC);

 val = digitalRead(switchpin);
  delay(10);
  val2 = digitalRead(switchpin);
   
   if (val == val2) {
     if (val != buttonstate) {
       if (val == LOW) {
         if (redvalue + 30 >= redVal) {
           if (redvalue - 30 <= redVal) {
             if (bluevalue + 30 >= blueVal) {
               if (bluevalue - 30 <= blueVal) {
                 if (greenvalue + 30 >= greenVal) {
                   if (greenvalue - 30 <= greenVal) {
   
         digitalWrite(led, HIGH);
                
               if (val == val2) {
     if (val != buttonstate) {
       if (val == LOW) {
         if (redvalue + 30 <= redVal || redvalue - 30 >= redVal || greenvalue + 30 <= greenVal || greenvalue - 30 >= greenVal || bluevalue + 30 <= blueVal || bluevalue - 30 >= blueVal) {
         digitalWrite(ledtwo, HIGH); 
         
               }
             }
           }
         }
       }
     }
   }
             }
           }
         }
       }
     }
   }
}

It is long and sorry there are no comments but this is what it is for: I am trying to make a "game" of sorts where there are two rgb leds. One turns on to a random color. The other one, you have to try and make as close to the other one's color by using the 3 sliding potentiometers. There is a button to press that when pressed, will turn a red led(ledtwo) on if the values of the led are not within 30 "units". If the colors are close enough, the green led will turn on(led). The green led will turn on if the conditions are met, however, the red led wil not turn on no matter what. I know it's confusing but thank you for your help so far and I hope you can help me fix this. :slight_smile:
P.S. the serial printing is just there for testing purposes, it has no role in the real sketch.

I think I understand what you're trying to do. I think that something like the following should get you on the right track.

void loop()
{
    int guessRed = analogRead(redIn);
    int guessBlue = analogRead(blueIn);
    int guessGreen = analogRead(greenIn);

    guessRed = map(guessRed, 0, 1023, 0, 255);
    guessGreen = map(guessGreen, 0, 1023, 0, 255);
    guessBlue = map(guessBlue, 0, 1023, 0, 255);

    analogWrite(redPin, guessGreen);
    analogWrite(greenPin, guessGreen);
    analogWrite(bluePin, guessGreen);


    int deltaRed = abs( guessRed-actualRed );
    int deltaGreen = abs( guessGreen-actualGreen );
    int deltaBlue = abs( guessBlue-actualBlue );

    val = digitalRead(switchpin);
    delay(10);
    val2 = digitalRead(switchpin);

    if( (val==val2) && (val!=LOW) )
    {
        if( (deltaRed<=30) && (deltaGreen<=30) && (deltaBlue<=30) )
        {
            // Do what you want to do if the guess was right and in bounds
        }
        {
            // Do what you want to do if the guess was wrong and out of bounds
        }
    }
}

Hi,
I don't have much experience in coding and I don't even have an Arduino yet but one question I have on this code is;
Could you use an else statement since as I understand it if all the conditions are correct you want to light the "correct" led ELSE you want to light the "incorrect" led indicating a wrong guess. Would this work and if not can someone correct me.
As I said I have limited experience in coding (mechanical engineering mantra if you can't physically see something working it must be some kind of witchcraft/magic :wink: )

Edit: Just read that last code, and it looks like my comments might not have been needed. But to clear something up, is the "else" implied as part of the "if" statement?
if(something == something})
{do this}
{else do this}

is the "else" implied as part of the "if" statement?

Not in the language of the Arduino (C++). This is how your pseudo-code works...

if(something1 == something2)
{then do this}
{always do this}

Your "else do this" is only an else-clause if the word else is included...

if(something1 == something2)
{then do this}
else
{otherwise do this}

Which leads nicely to a rule you should follow: Always include braces with 'if' statements. In other words, write your if statements like this...

if(something1 == something2)
{
then_do_this();
}
else
{
otherwise_do_this();
}

...not like this...

if(something1 == something2)
then_do_this();
else
otherwise_do_this();

Which leads nicely to a rule you should follow: Always include braces with 'if' statements.

Coding Badly gives advice on how to code nicely ;D
I just had to comment on the irony of that. :wink:

[edit]I completly agree with Coding Badly about the rule by the way.[/edit]

so the the example code should read;

if( (val==val2) && (val!=LOW) )
{
if( (deltaRed<=30) && (deltaGreen<=30) && (deltaBlue<=30) )
{
// Do what you want to do if the guess was right and in bounds
}
else
{
// Do what you want to do if the guess was wrong and out of bounds
}
}
}

Would this be a pratical solution to the original problem as you can only ever be right or wrong never both (i.e if else)

I read your idea, and thought it was funny how much you got out of two leds and three variable resistors.

Whipped up some code, but I completely understand if you want to implement this yourself. Maybe you can draw some inspiration from my code. At best learn something :slight_smile:

[UNTESTED COMPILING CODE]

#include <Button.h>
#include <LED.h>

//program setup
const byte ERROR_MARGIN = 30;


typedef struct { byte r; byte g; byte b; } RGB; //datastructure for an RGB value

RGB targetLED = { 9,10,11 };
RGB targetValue;

RGB guessLED = { 3,5,6 };
RGB guessValue;


const byte guessValuePinR = 0;
const byte guessValuePinG = 1;
const byte guessValuePinB = 2;

Button guessButton = Button(8, PULLUP); //change to PULLDOWN if that's what you use

LED red = LED(12);
LED green = LED(13);

//convenience macro [shame on me]
#define LIGHT( rgb, val ) lightRGB( rgb.r , rgb.g , rgb.b , val.r, val.g, val.b )

void setup(){
      randomSeed(analogRead(0));
      randomize();
}

void loop(){
      //listen for input
      if (guessButton.uniquePress()){
            if (guess()){
                  //indicate success
                  green.fadeIn(500);
                  green.blink(500,4);
                  
                  randomize(); //replay with different variables
            } else {
                  //indicate error
                  red.blink(2000,1);
            }
      }
      
      readGuessedValues();
      
      //show status
      LIGHT(targetLED, targetValue);
      LIGHT(guessLED, guessValue);
}

//randomize the targetValue RGB
//for modularity you could change to: randomize( RGB &targetValue ), but IDE has a but that makes this harder that it should
void randomize(){
      targetValue.r = random(255);
      targetValue.g = random(255);
      targetValue.b = random(255);
}

//get the input from the pots/faders
void readGuessedValues() {
      guessValue.r = map( analogRead( guessValuePinR ) , 0, 1023, 0, 255 );
      guessValue.g = map( analogRead( guessValuePinG ) , 0, 1023, 0, 255 );
      guessValue.b = map( analogRead( guessValuePinB ) , 0, 1023, 0, 255 );
}

//could've been lightRGB( RGB &pin, RGB &value ) but IDE has a bug
void lightRGB( byte targetValuePinR, byte targetValuePinG, byte targetValuePinB, byte targetValueR,  byte targetValueG,  byte targetValueB) {
      analogWrite( targetValuePinR, targetValueR );
      analogWrite( targetValuePinG, targetValueG );
      analogWrite( targetValuePinB, targetValueB );
}

//perform a guessValue and return the status of the guessValue
boolean guess(){
      if ( 
            abs(targetValue.r - guessValue.r) <= ERROR_MARGIN 
            &&
            abs(targetValue.g - guessValue.g) <= ERROR_MARGIN 
            &&
            abs(targetValue.b - guessValue.b) <= ERROR_MARGIN )
            { 
                  //guessValueed RGB value is within the ERROR_MARGIN domain
                  return true; 
      }
      return false;
}

[edit]You'll need these libraries to run the code:
http://www.arduino.cc/playground/Code/LED
http://www.arduino.cc/playground/Code/Button

Happy Coding![/edit]

To mattd (#15)...

so the the example code should read

Yes. It appears Fjornir left out the "else".

Would this be a pratical solution to the original problem as you can only ever be right or wrong never both (i.e if else)

Using else-clauses would certainly have helped. In AlphaBeta's code...

            if (guess()){
                  //indicate success
                  green.fadeIn(500);
                  green.blink(500,4);

                  randomize(); //replay with different variables
            } else {
                  //indicate error
                  red.blink(2000,1);
            }

...that's the purpose of the "else". The then-clause is executed if the human guesses correctly and the else-clause is executed if the human wasn't correct.

AlphaBeta (#14)...

I just had to comment on the irony of that

I'm nothing if I'm not ironic. :slight_smile:

Thanks for clearifying that Coding Badly!
As I said before I am a mechanical engineer so I have a somewhat limited experience with electronics/coding, mainly visual basic and Matlab from 1st year engineering. I know a little about the concepts but still need to become familiar with the Arduino/C++ language.
And sorry to ikestarm for hijacking your thread!