Else if statements not working as expected

Projects Programming
1

I have a code that lights up an led on the neotrellis. I press one button to move the led right and another to move it left. My if, else if, else statements are not seeming to work correctly. When I press on of the buttons it seems to activate both one of the if statements and the else statement. here is my code

#include "Adafruit_NeoTrellis.h"  //librairies
#include <BlockNot.h>

BlockNot timer(1000);

Adafruit_NeoTrellis trellis;

//define a callback for key presses
TrellisCallback blink(keyEvent evt) {
  // Check is the pad pressed?
  if (evt.bit.EDGE == SEESAW_KEYPAD_EDGE_RISING) {
    Serial.println("BUTTON PRESS");
    if (evt.bit.NUM == 12) {
      if (trellis.pixels.getPixelColor(12) == 6553700) {
        trellis.pixels.setPixelColor(12, 0);
        trellis.pixels.setPixelColor(15, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else if (trellis.pixels.getPixelColor(13) == 6553700) {
        trellis.pixels.setPixelColor(13, 0);
        trellis.pixels.setPixelColor(12, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else if (trellis.pixels.getPixelColor(14) == 6553700) {
        trellis.pixels.setPixelColor(14, 0);
        trellis.pixels.setPixelColor(13, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else if (trellis.pixels.getPixelColor(15) == 6553700) {
        trellis.pixels.setPixelColor(15, 0);
        trellis.pixels.setPixelColor(14, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else {
        Serial.println("MOVE ERROR");
        while (1) {
          delay(1000);
        }
      }
    } else if (evt.bit.NUM == 15) {
      if (trellis.pixels.getPixelColor(12) == 6553700) {
        trellis.pixels.setPixelColor(12, 0);
        trellis.pixels.setPixelColor(13, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else if (trellis.pixels.getPixelColor(13) == 6553700) {
        trellis.pixels.setPixelColor(13, 0);
        trellis.pixels.setPixelColor(14, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else if (trellis.pixels.getPixelColor(14) == 6553700) {
        trellis.pixels.setPixelColor(14, 0);
        trellis.pixels.setPixelColor(15, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else if (trellis.pixels.getPixelColor(15) == 6553700) {
        trellis.pixels.setPixelColor(15, 0);
        trellis.pixels.setPixelColor(12, 6553700);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else {
        Serial.println("MOVE ERROR");
        while (1) {
          delay(1000);
        }
      }
    }
  } else if (evt.bit.EDGE == SEESAW_KEYPAD_EDGE_FALLING) {
    // or is the pad released?
    return 0;
  }
}

int speedCount = 0;
void setup() {
  Serial.begin(9600);
  if (!trellis.begin()) {
    Serial.println("Could not start trellis, check wiring?");
    while (1) delay(1);
  } else {
    Serial.println("NeoPixel Trellis started");
  }
  //activate all keys and set callbacks
  for (int i = 0; i < NEO_TRELLIS_NUM_KEYS; i++) {
    trellis.activateKey(i, SEESAW_KEYPAD_EDGE_RISING);
    trellis.activateKey(i, SEESAW_KEYPAD_EDGE_FALLING);
    trellis.registerCallback(i, blink);
  }
  //do a little animation to show we're on
  for (uint16_t i = 0; i < trellis.pixels.numPixels(); i++) {
    trellis.pixels.setPixelColor(i, Wheel(map(i, 0, trellis.pixels.numPixels(), 0, 255)));
    trellis.pixels.show();
    delay(50);
  }
  for (uint16_t i = 0; i < trellis.pixels.numPixels(); i++) {
    trellis.pixels.setPixelColor(i, 0x000000);
    trellis.pixels.show();
    delay(50);
  }
  trellis.pixels.setPixelColor(12, 100, 0, 100);
  trellis.pixels.show();
  timer.start(WITH_RESET);
}

void loop() {
  // put your main code here, to run repeatedly:
  trellis.read();
}
uint32_t Wheel(byte WheelPos) {
  if (WheelPos < 85) {
    return trellis.pixels.Color(WheelPos * 3, 255 - WheelPos * 3, 0);
  } else if (WheelPos < 170) {
    WheelPos -= 85;
    return trellis.pixels.Color(255 - WheelPos * 3, 0, WheelPos * 3);
  } else {
    WheelPos -= 170;
    return trellis.pixels.Color(0, WheelPos * 3, 255 - WheelPos * 3);
  }
  return 0;
}

here is my serial output

BUTTON PRESS
MOVE
MOVE ERROR

the serial output shows messages from both the if statement and the else. shouldn't it only do one?

2

I don't see a button library, in fact I don't see any buttons but I am old. If you have not provided debounce then buttons behave erratically.

3

I am unfamiliar with the neo_trellis library, but if I convert some of your code to a standard C file and run it, your if statements are correct. Following @sonofcy's suggestion, make sure that the neo_trellis library is indeed debouncing your switch presses.

#include <iostream>

// TIP To <b>Run</b> code, press <shortcut actionId="Run"/> or click the <icon src="AllIcons.Actions.Execute"/> icon in the gutter.
int main()
{
    int  number;

    while(true)
    {
        printf("Type in a number from 12 to 15 \n");
        scanf("%d", &number);
        printf("The number you typed was %d\n", number);

        if (number == 12)
        {
            printf("MOVE1\n");
        }
        else if (number == 13)
        {
            printf("MOVE2\n");
        }
        else if (number == 14)
        {
            printf("MOVE3\n");
        }
        else if (number == 15)
        {
            printf("MOVE4\n");
        }
        else
        {
            printf("MOVE ERROR\n");
        }
    }
}

//OUTPUT
Type in a number from 12 to 15
14
The number you typed was 14
MOVE3
Type in a number from 12 to 15
16
The number you typed was 16
MOVE ERROR
Type in a number from 12 to 15
12
The number you typed was 12
MOVE1
Type in a number from 12 to 15
//

4

What does this magic number mean/do?

5

it's 0x640064 aka RGB (100, 0, 100) in decimal. Likely a medium-dark purple or magenta-violet hue as you mix equally red and blue.

1 Like
6

@wzctuba your callback promises a return value, but your SEESAW_KEYPAD_EDGE_RISING does not provide one.

So it gets called repeatedly, maybe, because it thinks you told it to.

Fix that, say what. I have no device to test it on.

Go to Preferences in the IDE and turn on all warnings and verbosity. This is a flaw the compiler might help you catch - heed the red ink.

a7

7

Actually, it doesn't:

typedef void (*TrellisCallback)(keyEvent evt);

Add debugging code:
Which MOVE and MOVE ERROR are you actually hitting?
What ARE the values of pixels 12-15?

1 Like
8

Ok, it is larger than the max 16 bit int...
Maybe ul at the end is needed (I am not sure and may depend on the mcu type that I could not find on this thread...).
@wzctuba you can give numbers as their hex equivalent... It would have been immediately clear what this number meant...

uint32_t mycolor = 0x640064;
9

Well no. Whether you write the literal in decimal (6553700) or hexadecimal (0x640064), the compiler treats both as an int if the value fits in an int; otherwise, it promotes them to the smallest larger type in the following order: unsigned int, long, unsigned long, long long, or unsigned long long, unless you explicitly specify a type with a suffix like U or L.

Here it does not fit as an int if it’s A 8 bit MCU so the compiler would make that a long.

Then it’s used in the expression

trellis.pixels.getPixelColor(15) == 6553700

getPixelColor(15) returns a uint32_t, and you write 6553700 without a suffix, the compiler treats 6553700 as an int if it fits, or as a long if not. The comparison is valid but will involve an implicit conversion.

The long literal 6553700 will be promoted to uint32_t before comparison (rule between unsigned and signed types), and the result will be what you expect numerically because 6553700 fits in both types without changing bit patterns.

But you are right that it would be much better to be explicit and clean, writing 6553700UL or 6553700U makes clear to the compiler and readers that you intend it as an unsigned and giving it a suitable constant name will make code much more readable (no magic value).

1 Like
10

You could narrow down the behavior by making the messages distinct: "MOVE 12 12", "MOVE 12 13", "MOVE ERROR 12", etc. But if these are the only such messages, the fact that you also get "BUTTON PRESS" just once -- yeah, there's something wacky going on: a single invocation of the function executing mutually exclusive paths. Hard to diagnose from here.

Meanwhile, the code is verbose and redundant. Cleaning it up might avoid the problem (without having to figure out what it was). Compare the fifty-ish lines for _EDGE_RISING to handle four pixels -- more pixels, more redundant code -- with these thirty-ish

  if (evt.bit.EDGE == SEESAW_KEYPAD_EDGE_RISING) {
    Serial.println("BUTTON PRESS");
    enum {left = -1, neither, right} dir = [](int NUM) {
      switch (NUM) {
        case 12: return left;
        case 15: return right;
        default: return neither;
      }
    }(evt.bit.NUM);
    if (dir) {
      int current = 0;
      for (int p = 12; p <= 15; p++) {
        if (trellis.pixels.getPixelColor(p) == PURPLE) {
          current = p;
          break;
        }
      }
      if (current) {
        int next = current + dir;
        if (next < 12) {
          next = 15;
        } else if (next > 15) {
          next = 12;
        }
        trellis.pixels.setPixelColor(current, 0);
        trellis.pixels.setPixelColor(next, PURPLE);
        trellis.pixels.show();
        Serial.println("MOVE");
      } else {
        Serial.println("MOVE ERROR");
      }
    }
  }

That uses a constant for the repeated color (expressing the RGB value in hex to give the reader a quick idea what it looks like). Lacking a better name indicating "why", call it "what" it is

constexpr uint32_t PURPLE{ 0x640064 };

Note that 12 and 15 are repeated as magic numbers four times each. (Not clear whether the .bit.NUM and pixel number are directly related, or just coincidentally the same.) It might be worth creating constants with good names for those. The code is approximately four parts

  1. Going left or right -- or neither?
  2. Find the current pixel that is on -- if any
  3. Figure the next pixel, with wrap-around. This takes advantage of the fact that they're consecutive in this case
  4. Turn off the current pixel and turn on the next one.

One extravagance is assignment-by-lambda, but it allows the switch to be succinct.

The code still holds the state directly in which pixel is on to begin with, and then manually toggles exactly one pair. This requires more initial setup. It would be more robust to hold the state separately (in a single variable) and then set all the pixels accordingly (as long as a separate show call is required to update the display, to avoid showing intermediate state which might flicker).

  if (evt.bit.EDGE == SEESAW_KEYPAD_EDGE_RISING) {
    static constexpr auto pixelCount = 4;
    static int currentPixel;
    Serial.println("BUTTON PRESS");
    enum { left = -1, neither, right } dir = [](int x) {
      switch (x) {
        case 12: return left;
        case 15: return right;
        default: return neither;
      }
    }(evt.bit.NUM);
    if (dir) {
      currentPixel += dir;
      if (currentPixel < 0) {
        currentPixel = pixelCount - 1;
      } else if (currentPixel >= pixelCount) {
        currentPixel = 0;
      }
      for (int p = 0; p < pixelCount; p++) {
        trellis.pixels.setPixelColor(12 + p, p == currentPixel ? PURPLE : 0);
      }
      trellis.pixels.show();
      Serial.println("MOVE");
    }
  }

Even shorter and simpler, with no invalid state. One potential downside: if there are a lot of pixels, then setting the color of all the them just to change one pair might be slow enough to matter. But this saves time not trying to find the one that is on.

Then the painting loop could be broken out as a separate function, to be called initially; along with making those static-in-function variables global. (Or have both put in an appropriate class.)

11

@kenb4 I followed your first code and the movement is working now.

12

how do you convert the color to a hex number? I was figuring out the color code by first printing the color in rgb and then reading the color which reads the color as a decimal

13

Assuming you have the full RGB color space then you write 0xRRGGBB where each color (RR, GG and BB) can vary between 0 (none) to FF (255) meaning full on.

So pure red would be 0xFF0000, pure blue 0x0000FF and If you want a cyan 0x00FFFF - mixing Green and Blue. Yellow in RGB would be 0xFFFF00, which is full red and full green with no blue.

Then you can convert the hex representation into decimal if you want to be cryptic :slight_smile:

14

I dont understand what the "F"s are. in the code another user provided the hex color was written like this: 0x640064

15

It is hexadecimal... so you need 16 different symbols: 0-9 plus a-f = 16...
0x tells the compiler that it is a hexadecimal representation.

16
17

A function like getPixelColor returns an integer of some specific size. But that number is neither inherently decimal nor hexadecimal. (If anything, arguably it's binary.)

auto theColor = trellis.pixels.getPixelColor(12);
Serial.println(theColor);
Serial.println(theColor, HEX);
Serial.println(theColor, DEC);
Serial.println(theColor, BIN);

println defaults to DEC if not specified.

Some calculator programs have a "programmer's" mode that can do these conversions. In a pinch, you can also use the JavaScript console of your browser

> (6553700).toString(16)
  "640064"
> 0x640064
  6553700
18

Yes it is a binary number.
But 24 characters long...
For us (humans) that is difficult to read. Enter hexadecimal. Each 4 bits are replaced by 1 0-f symbol. Only 6 symbols, 2 per color...
00 = 0 (off)
FF = 255 (fully on).

Typing #640064 into google will show you the matching color... as well as other ways to represent the color such as: rgb(100,0,100), hsl(300,100,20), cmyk(0,100,0,61)

19

I did provide the 0x640064

When you use hexadecimal (base 16) you need 16 symbols. We use 0-9 as in base 10 and then we use letters from A to F (capitalization does not matter a to f works too). A is worth 10 units, B 11 units, … and F is 15 units.

In C++ (and many other languages) we use the prefix 0x to tell the compiler that the value we are writing in in base 16. That is 640064 would be in base 10 and 0x640064 is in base 16.

So back to this number 0x640064 - you have the three RGB individual values R is 0x64, G is 0x00 and B is 0x64

The range for each color component are coded on one byte each - from 0 to 255 in base 10 or if you use hexadecimal from 0x00 to 0xFF

Makes more sense?

Read