Code Problem, Not Doing What was Intended

Hi All
So basically, i was hoping for one button to switch between functions, and the other button to just turn everything off.
Right now its just really buggy, and the lights blink really fast sometimes, other times it just goofs up entirely.
Below is a picture of my project, my code, and my little plan for my finished

//LED Mood Light
//Edited By Billho
//Control RGB Led's, smoothly transition between colours
//Switch functions with a button
//Set lights off with another button
//Control lights with potentiometers

int maxBright = 255; //Max Brightness the pins will reach

int delayTime = 20; //Delay Time, large delay time for longer transitions

int buttonPin = 1; //Function button
int buttonPinOff = 2;  //Off Button

//Which pin for which colour
int redPin = 9;
int greenPin = 10;
int bluePin = 11;

//Pot pins
int knobvalueRed;
int potRed = 0;
int knobvalueGreen;
int potGreen = 1;
int knobvalueBlue;
int potBlue = 2;

//Initial colour value for each pin
int red = 0;
int green = 170;
int blue = 170;

//For setting the lights to off
int redOff = 0;
int greenOff = 0;
int blueOff = 0;

// Indicates whether a colour is incrementing (1) or decrementing (0).
int incR = 1;
int incG = 1;
int incB = 0;

//Button Pin1
int state = HIGH;      // the current state of the output pin
int reading;           // the current reading from the input pin
int previous = LOW;    // the previous reading from the input pin

//Button Pin Off
int state2 = HIGH;      // the current state of the output pin
int reading2;           // the current reading from the input pin
int previous2 = LOW;    // the previous reading from the input pin

// the follow variables are unsigned long's because the time, measured in miliseconds,
// will quickly become a bigger number than can be stored in an int.
unsigned long time = 0;         // the last time the output pin was toggled
unsigned long debounce = 200;   // the debounce time, increase if the output flicker

//Function for pot control of LEDs
void fader() {
  knobvalueRed = analogRead(potRed);
  knobvalueRed = map(knobvalueRed, 0, 1023, 0, 255);
  knobvalueGreen = analogRead(potGreen);
  knobvalueGreen = map(knobvalueGreen, 0, 1023, 0, 255);
  knobvalueBlue = analogRead(potBlue);
  knobvalueBlue = map(knobvalueBlue, 0, 1023, 0, 255);
  analogWrite(9, knobvalueRed);
  analogWrite(10, knobvalueGreen);
  analogWrite(11, knobvalueBlue);
}

//Function for the blending of lights
void transition() { 
	if (red >= maxBright)
    incR = 0;
  else if (red <= 0)
    incR = 1;
  if (green >= maxBright)
    incG = 0;
  else if (green <= 0)
    incG = 1;
  if (blue >= maxBright)
    incB = 0;
  else if (blue <= 0)
    incB = 1;
 
  if (incR)
    red++;
  else
    red--;
  if(incG)
    green++;
  else
    green--;
  if(incB)
    blue++;
  else
    blue--;
	
}

//Sets colours to start of loop
void setColour() {
  analogWrite(redPin, red);
  analogWrite(greenPin, green);
  analogWrite(bluePin, blue);
	
}

//Sets LEDs to off
void setOff() {
  analogWrite(redPin, redOff);
  analogWrite(greenPin, greenOff);
  analogWrite(bluePin, blueOff);
}

void setup()
{
 pinMode(buttonPin, INPUT);
 pinMode(buttonPinOff, INPUT);
}

void loop(){
 // read the pushbutton input pin:
  reading = digitalRead(buttonPin);
  reading2 = digitalRead(buttonPinOff);
 
  // compare the reading to its previous state
  if (reading != previous) {
	      if (reading == HIGH) {
		  state = !state; //toggle state
		  }
	  previous = reading;
  }
    // if the state has changed, increment the counter
    if (state == HIGH) {
      // if the current state is HIGH then the button
      // went from off to on:
      transition();
      setColour();
      delay(delayTime);
    } 
    else {
      // if the current state is LOW then the button
      // went from on to off:
      fader(); 
    
    
   if (reading2 != previous2) {
	      if (reading2 == HIGH) {
		  state2 = !state2; //toggle state
		  }
	  previous2 = reading2;
  }
    // if the state has changed, increment the counter
    if (state2 == HIGH) {
      // if the current state is HIGH then the button
      // went from off to on:
      // do nothing
    } 
    else {
      // if the current state is LOW then the button
      // went from on to off:
      setOff(); 
      
  	   
  }
 }
}/code]

Those are some beefy-looking switches for this application - lots of moving metal inside when they switch. Contact bounce?

I think there is some incorrect logic in this

  if (reading2 != previous2) {
      if (reading2 == HIGH) {
  state2 = !state2; //toggle state
  }
  previous2 = reading2;
  }
    // if the state has changed, increment the counter
    if (state2 == HIGH) {
      // if the current state is HIGH then the button  <<== HIGH OR LOW HAPPENS WHEN THE BUTTON IS HIGH
      // went from off to on:
      // do nothing
    }
    else {
      // if the current state is LOW then the button
      // went from on to off:
      setOff();
  }
 }

I suspect the simple

state2 = reading2;
if (state2 == HIGH) {
 //etc

would do what you are trying to achieve

Also, it may be wise to wait a little time between successive reads of the buttons. See how this is done with millis() in several things at a time.

...R

I think there is some incorrect logic in this

I have never understood why it takes three variables to keep track of the current and previous state of a switch. Some stupid example on the internet keeps getting propagated (and, yes, I know which one. Why some of us with a clue can't be given permission to fix the stupid examples escapes me.).

PaulS:
I have never understood why it takes three variables to keep track of the current and previous state of a switch.

Sounds like a debounce algorith. A bad one.

A better one is here: Software Debounce - Exhibition / Gallery - Arduino Forum

Here's your code with the debounce algorithm substituted, but I'm finding it a little weird and I'm thinking that bouncing is not really your problem.

Your comment says "if the state has changed, increment the counter", but that section of code is going to be executed repeatedly for as long as buttonPin is high. 'state' is going to change its value whenever buttonPin goes from low to high ... oh, I see: it's a push button emulating a toggle. Well, that's ok. And the other button is the same.

You only look at that second button when the first "toggle" is low, which means that it might do unexpected things if someone pushes both buttons at the same time, but meh.

Oh, hang on. You say "So basically, i was hoping for one button to switch between functions, and the other button to just turn everything off". Well, at present, that other button only gets looked at if 'state' (the virtual toggle) is LOW. Maybe that's the behaviour that you are finding buggy.

//LED Mood Light
//Edited By Billho
//Control RGB Led's, smoothly transition between colours
//Switch functions with a button
//Set lights off with another button
//Control lights with potentiometers

const int maxBright = 255; //Max Brightness the pins will reach

const int delayTime = 20; //Delay Time, large delay time for longer transitions

const int buttonPin = 1; //Function button
const int buttonPinOff = 2;  //Off Button

//Which pin for which colour
const int redPin = 9;
const int greenPin = 10;
const int bluePin = 11;

//Pot pins
int knobvalueRed;
const int potRed = 0;
int knobvalueGreen;
const int potGreen = 1;
int knobvalueBlue;
const int potBlue = 2;

//Initial colour value for each pin
int red = 0;
int green = 170;
int blue = 170;

//For setting the lights to off
int redOff = 0;
int greenOff = 0;
int blueOff = 0;

// Indicates whether a colour is incrementing (1) or decrementing (0).
int incR = 1;
int incG = 1;
int incB = 0;

// button state and debounce filters

word buttonPinDebounce;
word buttonPinOffDebounce;

boolean buttonPinPrevious;
boolean buttonPinOffPrevious;

//Button Pin1
int state = HIGH;      // the current state of the output pin

//Button Pin Off
int state2 = HIGH;      // the current state of the output pin


// debounce algoritm from http://forum.arduino.cc/index.php?topic=339321

inline boolean extractState(unsigned int state) {
  return !!(state & 0x0100);
}

inline word debounce(word prevstate, boolean data) {
  // ok, the low 8 bits of the filter contain the first
  // low-pass filter,
  // the top 8 bits contain the second low pass filter,
  // EXCEPT FOR BIT 8, which contains the final output state.
  // this final output is always one sample behind

  register word newstate =
    // multiply curent state of both filters by 75%.
    // Note that 7F + 3F is BE, which is 65 short of 255
    ((prevstate >> 1) & 0x7F7F) //
    + ((prevstate >> 2) & 0x3F3F) //
    // add data to first low-pass filter
    + (data ? 65 : 0);

  // with the low filter processed, ad the new data from the
  // low filter into the high filter

  newstate = newstate
             // add output of first low-pass filter to second filter
             + ((newstate << 6) & 0x3F00)
             // add feedback from prevous state, for a total max of 65
             + ( (prevstate & 0x0100) << 1);

  // ok. now we apply the schmitt trigger using bit 8
  // for hysteresis
  // the tigger bounds: 0x70 and 0x90 are experimental.
  // tighter bounds will make the filter more responsive
  // and better able to catch double-clicks,
  // wider bounds make it less prone to noise.

  newstate =
    ( newstate & ~0x0100 ) // zap bit 8
    |
    (
      prevstate & 0x0100 // what was the previous state
      ? (newstate > 0x7000 ? 0x0100 : 0) // moving down
      : (newstate > 0x9000 ? 0x0100 : 0) // moving up
    )
    ;

  // modification - if the state changes, then saturate the filter.
  // this should help to get rid of bouncing in particular

  if ((prevstate ^ newstate) & 0x0100) {
    if (newstate & 0x0100) newstate = 0xFFFF;
    else newstate = 0;
  }

  return newstate;
}

//Function for pot control of LEDs
void fader() {
  knobvalueRed = analogRead(potRed);
  knobvalueRed = map(knobvalueRed, 0, 1023, 0, 255);
  knobvalueGreen = analogRead(potGreen);
  knobvalueGreen = map(knobvalueGreen, 0, 1023, 0, 255);
  knobvalueBlue = analogRead(potBlue);
  knobvalueBlue = map(knobvalueBlue, 0, 1023, 0, 255);
  analogWrite(9, knobvalueRed);
  analogWrite(10, knobvalueGreen);
  analogWrite(11, knobvalueBlue);
}

//Function for the blending of lights
void transition() {
  if (red >= maxBright)
    incR = 0;
  else if (red <= 0)
    incR = 1;
  if (green >= maxBright)
    incG = 0;
  else if (green <= 0)
    incG = 1;
  if (blue >= maxBright)
    incB = 0;
  else if (blue <= 0)
    incB = 1;

  if (incR)
    red++;
  else
    red--;
  if (incG)
    green++;
  else
    green--;
  if (incB)
    blue++;
  else
    blue--;

}

//Sets colours to start of loop
void setColour() {
  analogWrite(redPin, red);
  analogWrite(greenPin, green);
  analogWrite(bluePin, blue);

}

//Sets LEDs to off
void setOff() {
  analogWrite(redPin, redOff);
  analogWrite(greenPin, greenOff);
  analogWrite(bluePin, blueOff);
}

void setup()
{
  pinMode(buttonPin, INPUT);
  pinMode(buttonPinOff, INPUT);
}

void loop() {
  // read the pushbutton input pin:

  buttonPinDebounce = debounce(buttonPinDebounce, digitalRead(buttonPin));
  buttonPinOffDebounce = debounce(buttonPinOffDebounce, digitalRead(buttonPin));

  boolean buttonPinReading = extractState(buttonPinDebounce);
  boolean buttonPinOffReading = extractState(buttonPinOffDebounce);

  // compare the reading to its previous state
  if (buttonPinReading != buttonPinPrevious) {
    if (buttonPinReading) {
      state = !state; //toggle state
    }
    buttonPinPrevious = buttonPinReading;
  }
  // if the state has changed, increment the counter
  if (state == HIGH) {
    // if the current state is HIGH then the button
    // went from off to on:
    transition();
    setColour();
    delay(delayTime);
  }
  else {
    // if the current state is LOW then the button
    // went from on to off:
    fader();


    if (buttonPinOffReading != buttonPinOffPrevious) {
      if (buttonPinOffReading) {
        state2 = !state2; //toggle state
      }
      buttonPinOffPrevious = buttonPinOffReading;
    }
    // if the state has changed, increment the counter
    if (state2 == HIGH) {
      // if the current state is HIGH then the button
      // went from off to on:
      // do nothing
    }
    else {
      // if the current state is LOW then the button
      // went from on to off:
      setOff();


    }
  }
}

Hi,
Have you got pull-up or pull-down resistors on your input switches?

Can you please post a copy of your circuit, in CAD or a picture of a hand drawn circuit in jpg, png?

Tom.... :slight_smile:

That servo looks rather big and I don't see any other power supplies in that image. Aside from your debounce issue, you may want to get another power supply for that servo when you decide to put a load on it.

just saying.

Hi fellas
thank you all for your replys
i dont really understand a lot of the language as of now but i appreciate the help
me thinks maybe instead of just having a a second button to turn everything off, it might be a better idea to add a switch later that just physically stops the power
I've tried editing another code but its buggy again and the servo acts kinda weird,
basically this just uses an if and else statement but im not too sure
heres the updated simpler code, and a diagram of the previous circuit

//LED Mood Light
//Control RGB Led's, smoothly transition between colours
//Switch functions with a button
//Control lights with potentiometers
//Open servo latch with potentiometers

#include <Servo.h> //Servo library, outside of the sketch

Servo myservo; //Declare the Servo

int maxBright = 255; //Max Brightness the pins will reach

int delayTime = 20; //Delay Time, large delay time for longer transitions

int buttonPin = 1; //Function button

int position = 0; //Servo position

//Which pin for which colour
int redPin = 9;
int greenPin = 10;
int bluePin = 11;

//Pot pins
int knobvalueRed;
int potRed = 0;
int knobvalueGreen;
int potGreen = 1;
int knobvalueBlue;
int potBlue = 2;

//Initial colour value for each pin
int red = 0;
int green = 170;
int blue = 170;

// Indicates whether a colour is incrementing (1) or decrementing (0).
int incR = 1;
int incG = 1;
int incB = 0;

//Button Pin1
int state = HIGH;      // the current state of the output pin
int reading;           // the current reading from the input pin
int previous = LOW;    // the previous reading from the input pin

// the follow variables are unsigned long's because the time, measured in miliseconds,
// will quickly become a bigger number than can be stored in an int.
unsigned long time = 0;         // the last time the output pin was toggled
unsigned long debounce = 200;   // the debounce time, increase if the output flicker

//Function for pot control of LEDs
void fader() {
  knobvalueRed = analogRead(potRed);
  knobvalueRed = map(knobvalueRed, 0, 1023, 0, 255);
  knobvalueGreen = analogRead(potGreen);
  knobvalueGreen = map(knobvalueGreen, 0, 1023, 0, 255);
  knobvalueBlue = analogRead(potBlue);
  knobvalueBlue = map(knobvalueBlue, 0, 1023, 0, 255);
  analogWrite(9, knobvalueRed);
  analogWrite(10, knobvalueGreen);
  analogWrite(11, knobvalueBlue);
}

//Function for the blending of lights
void transition() { 
	if (red >= maxBright)
    incR = 0;
  else if (red <= 0)
    incR = 1;
  if (green >= maxBright)
    incG = 0;
  else if (green <= 0)
    incG = 1;
  if (blue >= maxBright)
    incB = 0;
  else if (blue <= 0)
    incB = 1;
 
  if (incR)
    red++;
  else
    red--;
  if(incG)
    green++;
  else
    green--;
  if(incB)
    blue++;
  else
    blue--;
	
}

//Sets colours to start of loop
void setColour() {
  analogWrite(redPin, red);
  analogWrite(greenPin, green);
  analogWrite(bluePin, blue);
	
}

void setup()
{
 pinMode(buttonPin, INPUT);
 myservo.attach(12); //Pin the servo is attatched to
}

void loop(){
 // read the pushbutton input pin:
  reading = digitalRead(buttonPin);
 
  // compare the reading to its previous state
  if (reading != previous) {
	      if (reading == HIGH) {
		  state = !state; //toggle state
		  }
	  previous = reading;
  }
    // if the state has changed, increment the counter
    if (state == HIGH) {
      // if the current state is HIGH then the button
      // went from off to on:
      transition();
      setColour();
      delay(delayTime);
      int analog1 = analogRead(A0);
      potRed = analog1 * (10.0 / 1023.0);
      int analog2 = analogRead(A1);
      potGreen = analog2 * (10.0 / 1023.0);
      int analog3 = analogRead(A2);
      potBlue = analog3 * (10.0 / 1023.0);	

      if(position < 180 && potRed == 0 && potGreen == 0 &&
      potBlue == 0){

      myservo.write(position++);
    } 
    else {
      // if the current state is LOW then the button
      // went from on to off:
      fader(); 
      myservo.write(position--);
      
Serial.print(potRed);
Serial.print(potGreen);
Serial.print(potBlue);
Serial.println();   
    
  }
 }
}

Sorry, but those Fritzing diagrams are next to useless.

Make a pencil drawing of the circuit and post a photo of that.

...R

It looks like you are still trying to have the Arduino power the servo, as well as control it. Come on back when you wise up. The Arduino can NOT power that servo.