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;
      }

    }

  }
}
  • 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. – JRobert May 1 '16 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? – Nick Gammon May 1 '16 at 22:00
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;
  • While valid C, it is really bad coding style to do that. Declare each variable in its own line. – JayEye May 2 '16 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. – Sempie May 2 '16 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. – Paul May 3 '16 at 7:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.