Adding pushbutton code to sketch-not working

Hi, I’m working on a cylon display with 30 leds (ws2811) with analogue controls for colour, brightness and speed. This is all working brilliantly!

I’m very new to coding but have tried to add a pushbutton, initially to switch on and off ( I want it to do more than eventually that but I have to have a starting point) and have used the arduino tutorial here
http://arduino.cc/en/Tutorial/ButtonStateChange. The button and circuit work fine for the tutorial but when I’ve tried to add the button code into my sketch I still get everything I had but no reaction to the button. Can anyone tell me what I’ve done wrong with the code please?

(I’m using fast led library for the main code)

//FastLED_AnalogueInput.ino
 
 //everything here works except the pushbutton code which I added!
 
/*
   Using a potentiometer to control colour, brightness and speed.
 Wire up as per http://arduino.cc/en/Tutorial/AnalogInput
 You can connect the wiper, to any analogue input pin, and 
 adjust the settings below.
 You will need three 10k potentiometers.
 */
 
 
#include <FastLED.h>
 
 
#define LED_PIN 10              // which pin are LEDS connected to?
#define NUM_LEDS 30
#define COLOR_ORDER RGB
#define LED_TYPE WS2811        // i'm using WS2811s, FastLED supports lots of different types.
 
/* 
 set your desired minimum and maxium brigtness settings here.
 Valid values are 0 - 255
 With 0 being fully dim, or not lit, and 255 being fully on.
 Therefore half power, or 50%, would be 128
 */
 
#define MAX_BRIGHTNESS 164      // Thats full on, watch the power!
#define MIN_BRIGHTNESS 32       // set to a minimum of 25%
 
const int brightnessInPin = A0;  // The Analog input pin that the brightness control potentiometer is attached to.
const int speedInPin = A1;       // Analog input pin that the speed control potentiometer is attached to.
const int colourInPin = A2;      // The Analog input pin that the colour control potentiometer is attached to.
 
 
 
const int  buttonPin = 2;    // the pin that the pushbutton is attached to
int buttonPushCounter = 0;   // counter for the number of button presses
int buttonState = 0;         // current state of the button
int lastButtonState = 0;     // previous state of the button

 
 
 
struct CRGB leds[NUM_LEDS];
 
void setup() {
  delay(3000); // in case we do something stupid. We dont want to get locked out.
 
  LEDS.addLeds<LED_TYPE, LED_PIN, COLOR_ORDER>(leds, NUM_LEDS).setCorrection(TypicalLEDStrip);
  FastLED.setBrightness(MAX_BRIGHTNESS);
  
  
  
   // initialize the button pin as a input:
  pinMode(buttonPin, INPUT);
  // initialize the LED as an output:
  pinMode(LED_PIN, OUTPUT);
  
  
  
}
 
void loop() {
  
  
  
   // read the pushbutton input pin:
  buttonState = digitalRead(buttonPin);

  // compare the buttonState to its previous state
  if (buttonState != lastButtonState) {
    // if the state has changed, increment the counter
    if (buttonState == HIGH) {
      // if the current state is HIGH then the button
      // wend from off to on:
      buttonPushCounter++;
      Serial.println("on");
      Serial.print("number of button pushes:  ");
      Serial.println(buttonPushCounter);
    } 
    else {
      // if the current state is LOW then the button
      // went from on to off:
      Serial.println("off"); 
    }
  }
  // save the current state as the last state, 
  //for next time through the loop
  lastButtonState = buttonState;

  
  // turns on the LED every two button pushes by 
  // checking the modulo of the button push counter.
  // the modulo function gives you the remainder of 
  // the division of two numbers:
  if (buttonPushCounter % 2 == 0) {
    digitalWrite(LED_PIN, HIGH);
  } else {
   digitalWrite(LED_PIN, LOW);
  }
  
  
  
  
  
  // read the analog brightness value:
  //int brightValue = analogRead(brightnessInPin);            
  // map it to the range of the FastLED brightness:
  int mappedValue = map(analogRead(brightnessInPin), 0, 1023, 0, 255);
 
  /* 
   At this point, brightness could be full off (mappedValue == 0)
   or it could be fully on (mappedValue == 255).
   if you are ruuning from a battery pack, or in a dark room, you 
   may not want full brightness.
   Or if you are in daylight, you may not want the pixels to go out.
   the following code, checks if mappedValue is above or below our defined
   brightness settings above.
   It works like this.
   
   we get mappedValue: if mappedValue is between MIN_BRIGHTNESS and MAX_BRIGHTNESS.
   we get MIN_BRIGHTNESS: if mappedValue is less than our defined MIN_BRIGHTNESS.
   we get MAX_BRIGHTNESS: if mappedValue is greater than our defined MAX_BRIGHTNESS
   
   so, it limits range of brightness values.
   
   */
 
  //int outputValue = constrain(mappedValue, MIN_BRIGHTNESS, MAX_BRIGHTNESS);
 
  // now we set the brightness of the strip
  FastLED.setBrightness(constrain(mappedValue, MIN_BRIGHTNESS, MAX_BRIGHTNESS));
 
  // read the analog speed value:          
  // map it to a value used in delay();
  int delayValue = map(analogRead(speedInPin), 0, 1023, 0, 50);  
 
  int mappedHue;
  // read the analog brightness value:
  //int hueValue = analogRead(colourInPin);            
  // map it to the range of the FastLED brightness:
 
 
 
  // First slide the led in one direction
  for(int i = 0; i < NUM_LEDS; i++) {
    mappedHue = map(analogRead(colourInPin), 0, 1023, 0, 255);
    // Set the i'th led to the chosen colour
    leds[i] = CHSV(mappedHue, 255, 255);
    // Show the leds
    FastLED.show();
    // now that we've shown the leds, reset the i'th led to black
    leds[i] = CRGB::Black;
    // Wait a little bit before we loop around and do it again
    delay(delayValue);  
  }
 
 
  // Now go in the other direction.  
  for(int i = NUM_LEDS-1; i >= 0; i--) {
    mappedHue = map(analogRead(colourInPin), 0, 1023, 0, 255);
    // Set the i'th led to the chosen colour 
    leds[i] = CHSV(mappedHue, 255, 255);
    // Show the leds
    FastLED.show();
    // now that we've shown the leds, reset the i'th led to black
    leds[i] = CRGB::Black;
    // Wait a little bit before we loop around and do it again
    delay(delayValue);  
  }
 
 
 
}

Looks fine except that the code implies that you're using an external pulldown resistor - are you?

Are you saying that you see nothing about button state on your serial output or that the button doesn't impact your LED as you expect?

Thanks, yes I have a 10Kohm tesistor connected to ground.

I hadn't checked the serial monitor before but I have now;

I get nothing on the serial monitor at all. (I added the line // initialize serial communication: Serial.begin(9600); in the set up as I'd missed it before. )

I can upload the state change detection sketch without touching my circuit and I get what I'd expect on the serial monitor, but nothing when I upload my cylon sketch :(

Serial.begin(9600);

What baud rate have you got the Serial monitor set to ?

Add

Serial.println("Starting");

immediately after the Serial.begin() in setup(). Do you see the message ?

What Arduino board are you using ?

Thanks for that- it makes a little more sense now...

I got the "Starting" message and then I got a few seemingly random "on" and "off" responses from the serial monitor but I've worked out it's only reading the button at the beginning of the loop, so if I press the button while the lights are some way along the cylon it does not register. If I press when the lights are at the beginning of the first sweep of the loop-bingo! How can I overcome this and ask the controller to be aware of button input throughout the cylon sequence?

Despite getting some comms from the button I still had no reaction from the leds- they just carried on regardless....

I am using arduino uno.

How can I overcome this and ask the controller to be aware of button input throughout the cylon sequence?

Start by getting rid of the delay()s, especially as they are inside for loops which increases the time period when the code cannot read inputs.

Use millis() for timing instead of delay(). Save the time from millis() when an event starts, then each time through loop() check whether the required time period has elapsed. If not then go round again, else do what you need and start timing again. Using this method allows you to read inputs in loop() each time round.

To use this method you will need to refactor your code considerably. See the BlinkWithoutDelay example in the IDE and Several things at the same time for ideas how to do it.

Thanks UKHeliBob,

I've had a look at those things you suggested and I sort of follow how that works, but now feel really out of my depth with trying to "refactor" the sketch. I'm plodding away with reading and trying to work it out. Do I still use the map function for speed? Any help with the actual writing/ re-writing would be greatly appreciated ;D Thanks

Actually I wrote this just now - probably very poor code, trying to incorporate the blink without delay, but what I got was a cylon that worked (surprise!) that ranged from very fast to so fast you can barely see it via the pot analogue input.

//FastLED_AnalogueInput.ino
 
/*
   Using a potentiometer to control colour, brightness and speed.
 Wire up as per http://arduino.cc/en/Tutorial/AnalogInput
 You can connect the wiper, to any analogue input pin, and 
 adjust the settings below.
 You will need three 10k potentiometers.
 */
 
 
#include <FastLED.h>
 
 
#define LED_PIN 10              // which pin are LEDS connected to?
#define NUM_LEDS 30
#define COLOR_ORDER RGB
#define LED_TYPE WS2811        // i'm using WS2811s, FastLED supports lots of different types.
 
/* 
 set your desired minimum and maxium brigtness settings here.
 Valid values are 0 - 255
 With 0 being fully dim, or not lit, and 255 being fully on.
 Therefore half power, or 50%, would be 128
 */
 
#define MAX_BRIGHTNESS 164      // Thats full on, watch the power!
#define MIN_BRIGHTNESS 32       // set to a minimum of 25%
 
const int brightnessInPin = A0;  // The Analog input pin that the brightness control potentiometer is attached to.
const int speedInPin = A1;       // Analog input pin that the speed control potentiometer is attached to.
const int colourInPin = A2;      // The Analog input pin that the colour control potentiometer is attached to.
 
 
 
 // Generally, you shuould use "unsigned long" for variables that hold time
// The value will quickly become too large for an int to store
unsigned long previousMillis = 0;        // will store last time LED was updated


 
 
struct CRGB leds[NUM_LEDS];
 
void setup() {
  delay(3000); // in case we do something stupid. We dont want to get locked out.
 
  LEDS.addLeds<LED_TYPE, LED_PIN, COLOR_ORDER>(leds, NUM_LEDS).setCorrection(TypicalLEDStrip);
  FastLED.setBrightness(MAX_BRIGHTNESS);
}
 
void loop() {
  // read the analog brightness value:
  //int brightValue = analogRead(brightnessInPin);            
  // map it to the range of the FastLED brightness:
  int mappedValue = map(analogRead(brightnessInPin), 0, 1023, 0, 255);
 
  /* 
   At this point, brightness could be full off (mappedValue == 0)
   or it could be fully on (mappedValue == 255).
   if you are ruuning from a battery pack, or in a dark room, you 
   may not want full brightness.
   Or if you are in daylight, you may not want the pixels to go out.
   the following code, checks if mappedValue is above or below our defined
   brightness settings above.
   It works like this.
   
   we get mappedValue: if mappedValue is between MIN_BRIGHTNESS and MAX_BRIGHTNESS.
   we get MIN_BRIGHTNESS: if mappedValue is less than our defined MIN_BRIGHTNESS.
   we get MAX_BRIGHTNESS: if mappedValue is greater than our defined MAX_BRIGHTNESS
   
   so, it limits range of brightness values.
   
   */
 
  //int outputValue = constrain(mappedValue, MIN_BRIGHTNESS, MAX_BRIGHTNESS);
 
  // now we set the brightness of the strip
  FastLED.setBrightness(constrain(mappedValue, MIN_BRIGHTNESS, MAX_BRIGHTNESS));
 
  // read the analog speed value:          
  // map it to a value used in delay();
  long interval = map(analogRead(speedInPin), 0, 1023, 0, 50);  
  
  
   // check to see if it's time to blink the LED; that is, if the 
  // difference between the current time and last time you blinked 
  // the LED is bigger than the interval at which you want to 
  // blink the LED.
  unsigned long currentMillis = millis();
 
  if(currentMillis - previousMillis >= interval) {
    // save the last time you blinked the LED 
    previousMillis = currentMillis;  
    
    interval = millis();
  }
  
  
 
  int mappedHue;
  // read the analog brightness value:
  //int hueValue = analogRead(colourInPin);            
  // map it to the range of the FastLED brightness:
 
 
 
  // First slide the led in one direction
  for(int i = 0; i < NUM_LEDS; i++) {
    mappedHue = map(analogRead(colourInPin), 0, 1023, 0, 255);
    // Set the i'th led to the chosen colour
    leds[i] = CHSV(mappedHue, 255, 255);
    // Show the leds
    FastLED.show();
    // now that we've shown the leds, reset the i'th led to black
    leds[i] = CRGB::Black;
    // Wait a little bit before we loop around and do it again
interval = millis();
  }
 
 
  // Now go in the other direction.  
  for(int i = NUM_LEDS-1; i >= 0; i--) {
    mappedHue = map(analogRead(colourInPin), 0, 1023, 0, 255);
    // Set the i'th led to the chosen colour 
    leds[i] = CHSV(mappedHue, 255, 255);
    // Show the leds
    FastLED.show();
    // now that we've shown the leds, reset the i'th led to black
    leds[i] = CRGB::Black;
    // Wait a little bit before we loop around and do it again
    interval = millis();  
  }
 
 
 
}

If you want to be able to read the button while the LEDs are slowly cycling you will need to use an interrupt routine. Have a look at http://gammon.com.au/interrupts

Russell.

viksta66:
Can anyone tell me what I’ve done wrong with the code please?

That’s an easy question:

 int delayValue = map(analogRead(speedInPin), 0, 1023, 0, 50);  
...
 for(int i = 0; i < NUM_LEDS; i++) 
 {
    ...
   // Wait a little bit before we loop around and do it again
   delay(delayValue);

Delay is BAD! And delay all the time is even WORSE!

It’s easy like that:
If your program is based on a delay() programming logic, meaning blocking the program execution for some time every now and then, you CANNOT create any interactive program that’s responsive at any time. Not a chance.

Do you need a program design which is always responsive to button presses?
And code for reading different buttons for being pressed?

russellz:
If you want to be able to read the button while the LEDs are slowly cycling you will need to use an interrupt routine. Have a look at Gammon Forum : Electronics : Microprocessors : Interrupts

Russell.

I don’t think so

Try this. UNTESTED - see below

#include <FastLED.h>

#define LED_PIN 10              // which pin are LEDS connected to?
#define NUM_LEDS 30
#define COLOR_ORDER RGB
#define LED_TYPE WS2811        // i'm using WS2811s, FastLED supports lots of different types.

#define MAX_BRIGHTNESS 164      // Thats full on, watch the power!
#define MIN_BRIGHTNESS 32       // set to a minimum of 25%

const byte brightnessInPin = A0;  // The Analog input pin that the brightness control potentiometer is attached to.
const byte speedInPin = A1;       // Analog input pin that the speed control potentiometer is attached to.
const byte colourInPin = A2;      // The Analog input pin that the colour control potentiometer is attached to.

unsigned long previousMillis = 0;        // will store last time LED was updated
unsigned long currentMillis = 0;
byte currentState = 0;

struct CRGB leds[NUM_LEDS];

void setup() 
{
  delay(3000); // in case we do something stupid. We dont want to get locked out.

  LEDS.addLeds<LED_TYPE, LED_PIN, COLOR_ORDER>(leds, NUM_LEDS).setCorrection(TypicalLEDStrip);
  FastLED.setBrightness(MAX_BRIGHTNESS);
}

void loop()
{
  int mappedValue = map(analogRead(brightnessInPin), 0, 1023, 0, 255);
  FastLED.setBrightness(constrain(mappedValue, MIN_BRIGHTNESS, MAX_BRIGHTNESS));

  long interval = map(analogRead(speedInPin), 0, 1023, 0, 50);
  unsigned long currentMillis = millis();
  int mappedHue;

  switch (currentState)
  {
    case 0:  //First slide the led in one direction
      static int i = 0;
      currentMillis = millis();
      mappedHue = map(analogRead(colourInPin), 0, 1023, 0, 255);
      // Set the i'th led to the chosen colour
      leds[i] = CHSV(mappedHue, 255, 255);
      // Show the leds
      FastLED.show();
      // now that we've shown the leds, reset the i'th led to black
      leds[i] = CRGB::Black;
      if (previousMillis - currentMillis >= interval)    //time for the next show
      {
        i++;
        previousMillis = currentMillis;                  //save the time the show started
      }
      if (i > NUM_LEDS)                                  //reached the end ?
      {
        currentState = 1;                                //if so, move to next state
      }
      break;

    case 1:      // Now go in the other direction.
      i = 255;
      currentMillis = millis();
      mappedHue = map(analogRead(colourInPin), 0, 1023, 0, 255);
      // Set the i'th led to the chosen colour
      leds[i] = CHSV(mappedHue, 255, 255);
      // Show the leds
      FastLED.show();
      // now that we've shown the leds, reset the i'th led to black
      leds[i] = CRGB::Black;
      if (previousMillis - currentMillis >= interval)
      {
        i--;
        previousMillis = currentMillis;
      }
      if (i < 0)
      {
        currentState = 0;
      }
      break;
  }
}

It does not compile but neither did your original code, but it shows the concept of the system being in different states, timing when in a state to move between LEDs and moving to the next state at the appropriate time.

millis() timing is used in each case of the switch/case commands so the program only moves on to the next LED after a period.