0
int r = 12;
int g = 11;
int b = 10;
int sw = 4;
int x;
int c = 0;

void setup() {
    pinMode(r, OUTPUT);
    pinMode(g, OUTPUT);
    pinMode(b, OUTPUT);
    pinMode(sw, INPUT);
    pinMode(13, OUTPUT);
}

void loop() {
    x = digitalRead(sw);
  if(x==HIGH) {
    delay(250);
    c++;
    if(c==1) {
        digitalWrite(r, HIGH);
        digitalWrite(g, LOW);
        digitalWrite(b, LOW);
       {
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
      }

    }
    else if(c==2) {
        digitalWrite(r, LOW);
        digitalWrite(g, HIGH);
        digitalWrite(b, LOW);
             {
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
      }
    }
        else if(c==3) {
        digitalWrite(r, LOW);
        digitalWrite(g, LOW);
        digitalWrite(b, HIGH);
             {
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
      }
    }
        else if(c==4) {
        digitalWrite(r, HIGH);
        digitalWrite(g, HIGH);
        digitalWrite(b, LOW);
             {
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
      }
    }
            else if(c==5) {
        digitalWrite(r, LOW);
        digitalWrite(g, HIGH);
        digitalWrite(b, HIGH);
             {
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
      }

    }
            else if(c==6) {
        digitalWrite(r, HIGH);
        digitalWrite(g, LOW);
        digitalWrite(b, HIGH);
             {
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        digitalWrite(13,HIGH);
        delay (250);
        digitalWrite(13, LOW);
        delay (250);
        c=0;
      }

    }

  }
}
2
  • What is the purpose of putting the last 3/4 or so of the statements in each if() block into a separate block? Certainly you could considerably shorten your code by putting those statements into a separate function that takes as its argument the number of times to toggle pin 13. Commented May 1, 2016 at 21:53
  • I suggest you use the auto-indent tool in the IDE. Your indentation is all over the place. And as JRobert said, learn about writing functions. Whenever you copy/paste the same code into multiple places like that, you should be writing a function instead. Are you flashing pin 13 the number of times that c is? How about making that into a loop? Commented May 1, 2016 at 22:00

4 Answers 4

1
int r = 12, g = 11,b = 10, sw = 4, x, c = 0;

void setup() {
    pinMode(r, OUTPUT);
    pinMode(g, OUTPUT);
    pinMode(b, OUTPUT);
    pinMode(sw, INPUT);
    pinMode(13, OUTPUT);
}

void loop() {
  x = digitalRead(sw);
  if(x==HIGH) {
    delay(250);
    c++;
    if(c==1)      setRGBandBlink13(HIGH,LOW,LOW,1);
    else if(c==2) setRGBandBlink13(LOW,HIGH,LOW,2);
    else if(c==3) setRGBandBlink13(LOW,LOW,HIGH,3);
    else if(c==4) setRGBandBlink13(HIGH,HIGH,LOW,4);
    else if(c==5) setRGBandBlink13(LOW,HIGH,HIGH,5);
    else if(c==6){
      setRGBandBlink13(HIGH,LOW,HIGH,6);
      c=0;
    }

  }
}

void setRGBandBlink13(bool newR,bool newG,bool newB,int times){
  setRGB( newR, newG, newB);
  blinkPin13(times);
}

void setRGB(bool newR,bool newG,bool newB){
  digitalWrite(r, newR);
  digitalWrite(g, newG);
  digitalWrite(b, newB);
}

void blinkPin13(int times){
  for(int i=0;i<times;i++){
    digitalWrite(13,HIGH);
    delay (250);
    digitalWrite(13, LOW);
    delay (250);
  }
}
1

This Sketch is way shorter and way better to maintain.
Note that 1/3 (26 out of 78) of the lines are comments.

Note:
Your RGB is able to display 8 states, but only 7 can be reached.
binc - c - RGB
000 - 0 -
001 - 1 - R
010 - 2 - G
011 - 3 - RG
100 - 4 - B
101 - 5 - R B
110 - 6 - GB
111 - 7 - RGB - NEVER REACHED

int r = 12;
int g = 11;
int b = 10;
int sw = 4;
int c = 0;
int maxC = 6;
int minC = 0;

void setup() {
    pinMode(r, OUTPUT);
    pinMode(g, OUTPUT);
    pinMode(b, OUTPUT);
    pinMode(sw, INPUT);
    pinMode(13, OUTPUT);
}

// for  (pulses)-times
// set  (targetPin) HIGH
// wait (msDelay)
// set  (targetPin) LOW
// wait (msDelay)
void pulse( int targetPin, int msDelay, int pulses ){
    // count to pulses
    // for each count, set targetPin HIGH and LOW
    // wait msDelay between each state change
    for ( int i = 0; i <= pulses, i++ ){
        digitalWrite( targetPin, HIGH );
        delay( msDelay );
        digitalWrite( targetPin, LOW );
        delay( msDelay );
    }
}

// int rgbBinary will set RGB according to its binary value.
// R ~= BIT_1
// G ~= BIT_2
// B ~= BIT_4
void setRGB( int rgbBinary ){
    // set each LED OFF
    // This is by far more performant than
    // checking which should keep its state.
    digitalWrite( r, LOW );
    digitalWrite( g, LOW );
    digitalWrite( b, LOW );

    // if rgbBinary contains BIT value 1, set R to HIGH
    if ( rgbBinary & 1 ) {
        digitalWrite( r, HIGH) ;
    }   
    // if rgbBinary contains BIT value 2, set R to HIGH
    if ( rgbBinary & 2 ) {
        digitalWrite( g, HIGH );
    }   
    // if rgbBinary contains BIT value 4, set R to HIGH
    if ( rgbBinary & 4 ) {
        digitalWrite( b, HIGH );
    }   
}

void loop() {
    // INPUT on SW ? 
  if ( digitalRead( sw ) == HIGH) {
    // wait 250ms
    delay( 250 );
    // incement c
    c++;
    // increment RGB status
    setRGB( c );
    // pulse OUTPUT 13 (c)-times 
    // starting by HIGH 
    // delay 250ms between each HIGH/LOW
    pulse( 13, 250, c );
    // reset (c) on maxC
    if ( c == maxC ){
        c = minC;
    }
  }
}
0

I don't have an appropriate circuit handy for testing, but I've condensed the code a bit and renamed some things to be more readable. Hopefully the result is the same! Importantly, I've changed your c to step and it is now zero-based, so "step 1" is 0 and "step 6" is 5, etc.

const int rPin = 12;
const int gPin = 11;
const int bPin = 10;
const int swPin = 4;
const int pin13 = 13; // Needs a better name!
const int msDelay = 250;

int step = 0;

// By pure luck, your patterns overlap, so they can be condensed like so.
const byte patterns[13] = {
    // 0         1          2          3          4           5
    HIGH, LOW, LOW, HIGH, LOW, LOW, HIGH, HIGH, LOW, HIGH, HIGH, LOW, HIGH
};

void setup()
{
    pinMode(rPin, OUTPUT);
    pinMode(gPin, OUTPUT);
    pinMode(bPin, OUTPUT);
    pinMode(swPin, INPUT);
    pinMode(pin13, OUTPUT);
}

void loop()
{
    if (digitalRead(swPin) == HIGH)
    {
        delay(msDelay);

        // Write red/green/blue pattern
        int index = step * 2;
        digitalWrite(rPin, patterns[index]);
        digitalWrite(gPin, patterns[index + 1]);
        digitalWrite(bPin, patterns[index + 2]);

        // Blink once for each step (0 through 5)
        for (int i = 0; i <= step; ++i)
        {
            digitalWrite(pin13, HIGH);
            delay(msDelay);
            digitalWrite(pin13, LOW);
            delay(msDelay);
        }

        // Increment or reset step
        step = (step < 5) ? step + 1 : 0;
    }
}
-1

Make all those ints in a single line.

int r = 12, g = 11,b = 10, sw = 4, x, c = 0;
3
  • While valid C, it is really bad coding style to do that. Declare each variable in its own line. Commented May 2, 2016 at 6:15
  • In this manner, compiler will trim code anyway. Declaring vars in a single row will decrease readability by a lot but won't trim the actual binary in the end. Nothing gained by that. Commented May 2, 2016 at 6:18
  • You could argue that putting everything in one line (int r=12;int g=11; int b=10; int sw=4; int x; int c=0;) would also "shorten" the code. But it barely decreases the amount of functional code. I also highly doubt that @USF meant to shorten this part of the code. Commented May 3, 2016 at 7:14

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.