Switch case variable not updating

I'm using a Mega to be an LED Strip controller and I'm trying to use the switch case to cycle through various visual outputs for the LED Strip. The problem I'm running into is that the variable used to count the number of button presses, 'visual', updates fine when the 'Visualize()' isn't included in the loop and only the 'CycleVisual();' is included. Once I add the 'Visualize()' the 'visual' only registers one increment and then doesn't update in the Serial Monitor of any more button presses. I want to assume the logic used in 'CycleVisual();' is ill-suited for 'Visualize();', but I'm lost after a couple of days of trying to figure out the problem. A schematic of the board is included above the code. Please disregard the pot and sound module since I want to include the usage of those after I figure out how to cycle through visuals that don't require sound input for now. Thank you for your time!

#include <Adafruit_NeoPixel.h>  
#ifdef __AVR__
#include <avr/power.h>   
#endif                   

#define LED_PIN   A5  //Pin for the pixel strand. Can be analog or digital.
#define LED_TOTAL 300  //Change this to the number of LEDs in your strand.
#define LED_HALF  LED_TOTAL/2
#define VISUALS   2   //Change this accordingly if you add/remove a visual in the switch-case in Visualize()
#define BUTTON_1  6  

Adafruit_NeoPixel strand = Adafruit_NeoPixel(LED_TOTAL, LED_PIN, NEO_GRB + NEO_KHZ800);  //LED strand objetcs

uint8_t visual = 0;  

void setup() {
  Serial.begin(9600);
  pinMode(BUTTON_1, INPUT);
  digitalWrite(BUTTON_1, HIGH);
  strand.begin(); 
  strand.show();
 }

void loop() { 
  Serial.println(visual);
  CycleVisual(); 
  delay(10); 
}

void CycleVisual() {
   if (digitalRead( BUTTON_1 ) == LOW ) {
    visual++;
    if (visual > VISUALS) visual = 0; 
    delay(350);
  }
}

void Visualize(){ 
  switch (visual) {
    case 0: return Sparkle(0xff, 0xff, 0xff, 50);
    break;
    case 1: return BouncingBalls(0xff, 0xff, 0xff, 5);
    break;
    case 2: return RunningLights(23, 43, 0, 50);
    break;
    default: Sparkle(0xff, 0xff, 0xff, 50);

  }
}

void showstrand() {
 #ifdef ADAFRUIT_NEOPIXEL_H 
   // NeoPixel
   strand.show();
 #endif
 #ifndef ADAFRUIT_NEOPIXEL_H
   // FastLED
   FastLED.show();
 #endif
}

void setPixel(int Pixel, byte red, byte green, byte blue) {
 #ifdef ADAFRUIT_NEOPIXEL_H 
   // NeoPixel
   strand.setPixelColor(Pixel, strand.Color(red, green, blue));
 #endif
 #ifndef ADAFRUIT_NEOPIXEL_H 
   // FastLED
   leds[Pixel].r = red;
   leds[Pixel].g = green;
   leds[Pixel].b = blue;
 #endif
}

void setAll(byte red, byte green, byte blue) {
  for(int i = 0; i < LED_TOTAL; i++ ) {
    setPixel(i, red, green, blue); 
  }
  strand.show();
}

void Sparkle(byte red, byte green, byte blue, int SpeedDelay) {
  int Pixel = random(LED_TOTAL);
  setPixel(Pixel,red,green,blue);
  strand.show();
  delay(SpeedDelay);
  setPixel(Pixel,0,0,0);
  digitalWrite(BUTTON_1, HIGH);
}

void RunningLights(byte red, byte green, byte blue, int WaveDelay) {
  int Position=0;
  
  for(int j=0; j<LED_TOTAL*2; j++)
  {
      Position++; // = 0; //Position + Rate;
      for(int i=0; i<LED_TOTAL; i++) {
        // sine wave, 3 offset waves make a rainbow!
        //float level = sin(i+Position) * 127 + 128;
        //setPixel(i,level,0,0);
        //float level = sin(i+Position) * 127 + 128;
        setPixel(i,((sin(i+Position) * 127 + 128)/255)*red,
                   ((sin(i+Position) * 127 + 128)/255)*green,
                   ((sin(i+Position) * 127 + 128)/255)*blue);
      }
      
      strand.show();
      delay(WaveDelay);
  }
}

void BouncingBalls(byte red, byte green, byte blue, int BallCount) {
  float Gravity = -9.81;
  int StartHeight = 1;
  
  float Height[BallCount];
  float ImpactVelocityStart = sqrt( -2 * Gravity * StartHeight );
  float ImpactVelocity[BallCount];
  float TimeSinceLastBounce[BallCount];
  int   Position[BallCount];
  long  ClockTimeSinceLastBounce[BallCount];
  float Dampening[BallCount];
  
  for (int i = 0 ; i < BallCount ; i++) {   
    ClockTimeSinceLastBounce[i] = millis();
    Height[i] = StartHeight;
    Position[i] = 0; 
    ImpactVelocity[i] = ImpactVelocityStart;
    TimeSinceLastBounce[i] = 0;
    Dampening[i] = 0.90 - float(i)/pow(BallCount,2); 
  }

  while (true) {
    for (int i = 0 ; i < BallCount ; i++) {
      TimeSinceLastBounce[i] =  millis() - ClockTimeSinceLastBounce[i];
      Height[i] = 0.5 * Gravity * pow( TimeSinceLastBounce[i]/1000 , 2.0 ) + ImpactVelocity[i] * TimeSinceLastBounce[i]/1000;
  
      if ( Height[i] < 0 ) {                      
        Height[i] = 0;
        ImpactVelocity[i] = Dampening[i] * ImpactVelocity[i];
        ClockTimeSinceLastBounce[i] = millis();
  
        if ( ImpactVelocity[i] < 0.01 ) {
          ImpactVelocity[i] = ImpactVelocityStart;
        }
      }
      Position[i] = round( Height[i] * (LED_TOTAL - 1) / StartHeight);
    }
  
    for (int i = 0 ; i < BallCount ; i++) {
      setPixel(Position[i],red,green,blue);
    }
    digitalWrite(BUTTON_1, HIGH);
    strand.show();
    setAll(0,0,0);
  }
}

Why are you writing to an input (BUTTON_!) 6 places?

boolrules:
Why are you writing to an input (BUTTON_!) 6 places?

if you're referring to:

#define BUTTON_1 6

I'm defining the pin for the button

for (int i = 0 ; i < BallCount ; i++) {
      setPixel(Position[i],red,green,blue);
    }
    digitalWrite(BUTTON_1, HIGH);

I think that he meant like here. It makes sense to write HIGH to the button in setup() to turn on the internal pullup, but nowhere else.

Have a look at the state change example in the IDE for a way to, reliably, read the switch without using any delays.

  pinMode( BUTTON_1, INPUT );
  digitalWrite (BUTTON_1, HIGH );         // don't get in the habit of doing this

If you need the pullup functionality on a pin, use the following:

pinMode( BUTTON_1, INPUT_PULLUP );

It's clear what you intend and it's self documenting.
Writing a HIGH to the pin works but it's one of those arduinoisms that causes nothing but problens -- as it did in your case.

In the real world,

DDRB &= ~(1 << BUTTON_1);
PORTB |= (1 << BUTTON_1);

is how you set a pin for INPUT_PULLUP.

Knowledge is never a bad thing, getting hung-up on Arduinoisms, not applicable elsewhere in the real world, is.

Hi,
Welcome to the forum.
I prefer;

 pinMode( BUTTON_1Pin, INPUT_PULLUP );

As you scan the code, you instantly know its INPUT and PULLUP in one line.

Also BUTTON_1Pin makes sure you are discriminating between a Pin and a variable/constant.

Tom... :slight_smile:

Like "libraries", missing main( ), or putting returns in functions that never appear to have a caller?
Here is one place, INPUT_PULLUP, where it actually is not a bad idea, even though it is deceptive, but somehow it's wrong among all the "right" stuff.

Hi,

 switch (visual) {
    case 0: return Sparkle(0xff, 0xff, 0xff, 50);
    break;
    case 1: return BouncingBalls(0xff, 0xff, 0xff, 5);
    break;
    case 2: return RunningLights(23, 43, 0, 50);
    break;
    default: Sparkle(0xff, 0xff, 0xff, 50);

  }

Is there a need for the "return" at the START in each of the case statements.
Isn't that what the "break" at the END of each case does?

Remove the "return" see what happens.

Tom... :slight_smile:

I use macros all the time to simplify the reading and debugging of code. I'm not at all suggesting it's a bad thing so long as the individual understands what is happening 'behind the scenes'. All too often the abstraction offered by the Arduino language, while it's great for beginners, detracts from the learning as the individual never really fully understands what (s)he is doing. And it's not portable. Somewhere along the curve we, as mentors, need to make a decision as to when to let go of the apron strings. If the individual demonstrates advanced, as I believe Saltine has, do you not agree the onus is on us (no pun intended) to nudge him/her beyond the comfort zone?

@TomGeorge: I was referring to the usage of a return in loop( ) that some have suggested to "short circuit" the flow and get back to the top of loop( ). It works, but I hate to think what it must do to a mind trying to learn C/C++. A break in a switch just drops you out of the switch.

@DKWatson: Looks to me like we basically agree with one another.

In the real world,

DDRB &= ~(1 << BUTTON_1);
PORTB |= (1 << BUTTON_1);

is how you set a pin for INPUT_PULLUP.

For one specific pin, on one specific Arduino.

For that to work, you need to which DDRx and PORTx the pin is on, and that is not portable.

So after removing the returns and setting the pinmode to INPUT_PULLUP, the code doesn't quite fully work. I have noticed that if I only use the Sparkle() command in the cases,

Ex:

    case 0:  Sparkle(0xff, 0, 0, 50);
    break;
    case 1:  Sparkle(0, 0xff, 0, 50);
    break;
    case 2:  Sparkle(0, 0, 0xff, 50);
    break;

The visual counts correctly with each button press and resets after the last case, but if I add the BouncingBall() or RunningLights() the visual counter no longer updates correctly. Is there something I am missing in the code for those two visualizations causing the visual counter not to update?

ALL the code, please

#include <Adafruit_NeoPixel.h>  
#ifdef __AVR__
#include <avr/power.h>   
#endif                   

#define LED_PIN   A5  //Pin for the pixel strand. Can be analog or digital.
#define LED_TOTAL 300  //Change this to the number of LEDs in your strand.
#define LED_HALF  LED_TOTAL/2
#define VISUALS   2   //Change this accordingly if you add/remove a visual in the switch-case in Visualize()
#define BUTTON_1  6  

Adafruit_NeoPixel strand = Adafruit_NeoPixel(LED_TOTAL, LED_PIN, NEO_GRB + NEO_KHZ800);  //LED strand objetcs

uint8_t visual = 0;  

void setup() {
  Serial.begin(9600);
  pinMode(BUTTON_1, INPUT_PULLUP);
  strand.begin(); 
  strand.show();
 }

void loop() { 
  Serial.println(visual);
  CycleVisual(); 
  Visualize();
  delay(10); 
}

void CycleVisual() {
   if (digitalRead( BUTTON_1 ) == LOW ) {
    visual++;
    if (visual > VISUALS) visual = 0; 
    delay(350);
  }
}

void Visualize(){ 
  switch (visual) {
    case 0:  Sparkle(0xff, 0, 0, 50);
    break;
    case 1:  Sparkle(0, 0xff, 0, 50);
    break;
    case 2:  Sparkle(0, 0, 0xff, 50);
    break;
    default: Sparkle(0xff, 0xff, 0xff, 50);
  }
}

void showstrand() {
 #ifdef ADAFRUIT_NEOPIXEL_H 
   // NeoPixel
   strand.show();
 #endif
 #ifndef ADAFRUIT_NEOPIXEL_H
   // FastLED
   FastLED.show();
 #endif
}

void setPixel(int Pixel, byte red, byte green, byte blue) {
 #ifdef ADAFRUIT_NEOPIXEL_H 
   // NeoPixel
   strand.setPixelColor(Pixel, strand.Color(red, green, blue));
 #endif
 #ifndef ADAFRUIT_NEOPIXEL_H 
   // FastLED
   leds[Pixel].r = red;
   leds[Pixel].g = green;
   leds[Pixel].b = blue;
 #endif
}

void setAll(byte red, byte green, byte blue) {
  for(int i = 0; i < LED_TOTAL; i++ ) {
    setPixel(i, red, green, blue); 
  }
  strand.show();
}

void Sparkle(byte red, byte green, byte blue, int SpeedDelay) {
  int Pixel = random(LED_TOTAL);
  setPixel(Pixel,red,green,blue);
  strand.show();
  delay(SpeedDelay);
  setPixel(Pixel,0,0,0);
}

void RunningLights(byte red, byte green, byte blue, int SpeedDelay) {
  int Position=0;
  for(int i=0; i<LED_TOTAL; i++)
  {
      Position++; // = 0; //Position + Rate;
      for(int i=0; i<LED_TOTAL; i++) {
        setPixel(i,((sin(i+Position) * 127 + 128)/255)*red,
                   ((sin(i+Position) * 127 + 128)/255)*green,
                   ((sin(i+Position) * 127 + 128)/255)*blue);
      }
      strand.show();
      delay(SpeedDelay);
  }
}

Change,

void loop() 
{
    Serial.println(visual);
    CycleVisual();
    Visualize();
    delay(10);
}

to

void loop() 
{
    Serial.println(visual);
    CycleVisual();
    delay(10);
}

and

void CycleVisual() 
{
    if (digitalRead( BUTTON_1 ) == LOW ) 
    {
        visual++;
        if (visual > VISUALS) visual = 0;
        delay(350);
    }
}

to

void CycleVisual() 
{
    if (digitalRead( BUTTON_1 ) == LOW ) 
    {
        visual++;
        if (visual > VISUALS) visual = 0;
        visualize();
        delay(350);
    }
}

This caused the animation speed of the Sparkle() to decrease immensely and the visual counter still does not update correctly :sweat_smile:

Edit: It seems as though the counter gets stuck in the RunningLights()

Purely for test, comment out CycleVisual() in loop(), thereby eliminating the button from the equation.

Cycle through visual in its place with a nice delay so you can see what's happening. So change

void loop() 
{
    Serial.println(visual);
    CycleVisual();
    Visualize();
    delay(10);
}

to

void loop() 
{
    Serial.println(visual);
    //CycleVisual();
    visual = ((visual + 1) % VISUALS);
    Visualize();
    delay(1000);
}

and see what happens.

The counter still gets stuck and the visual sticks to RunningLights() with no update.

However, I was able to cycle completely through the visuals by changing:

void RunningLights(byte red, byte green, byte blue, int SpeedDelay) {
  int Position=0;
  for (int i=0; i<LED_TOTAL; i++){
  Position++;
  for(int i=0; i<LED_TOTAL; i++) {
    setPixel(i,((sin(i+Position) * 127 + 128)/255)*red,
               ((sin(i+Position) * 127 + 128)/255)*green,
               ((sin(i+Position) * 127 + 128)/255)*blue);
    }
    strand.show();
    delay(SpeedDelay);
  }
}

To:

void RunningLights(byte red, byte green, byte blue, int SpeedDelay) {
  int Position=0;
  Position++;
  for(int i=0; i<LED_TOTAL; i++) {
    setPixel(i,((sin(i+Position) * 127 + 128)/255)*red,
               ((sin(i+Position) * 127 + 128)/255)*green,
               ((sin(i+Position) * 127 + 128)/255)*blue);
    }
    strand.show();
    delay(SpeedDelay);
}

This caused the RunningLights() animation to basically not "move" like it should, but I was able to restart the visual counter. Is it getting stuck in one of those 'for' commands?

DDRB &= ~(1 << BUTTON_1);

PORTB |= (1 << BUTTON_1);




is how you set a pin for INPUT_PULLUP.

Knowledge is never a bad thing, getting hung-up on Arduinoisms, not applicable elsewhere in the real world, is.

Meh. I'm with TomGeorge. "pinMode(pin, INPUT_PULLUP)" is clear, obvious, and portable to any device that implements internal pullups in some way (perhaps with a bit of effort.)

Using digitalWrite() while the pin is in input mode, or writing to PORT/DDR registers directly, is utilizing an device-specific and not very elegant property of SOME AVR chips, and it's extremely un-obvious to any non-expert reading the code.

(Did you know that the SAMD chips have an entirely different way to enable pullups, but go to significant expense inside digitalWrite() to duplicate the AVR behavior? That's one of the reasons that digitalWrite() on the Zero isn't 3x faster than Uno's digitalWrite(), despite the 3x higher clock rate. Grr.)

I wonder how long we'll have Arduino Libraries doing "things" in the pre-Arduino-1.0 ways :frowning: