Please help

[adafruit_support_bill]

The nesting levels of your curly braces '{' & '}' is not the same in the last two sketches you posted. That changes the logic considerably. Note the differences here:

Code: Select all

         digitalWrite(led1Pin, HIGH);
         delay(timer);
      }   
    }}


vs. here:

Code: Select all

         digitalWrite(led1Pin, HIGH);
         delay(timer);
      }   
    }}}


It would be a lot easier to spot such things if you take some of ChuckM's advice and re-structure the code into manageable pieces.

[Spinsycle]

I am looking to simplify the code. I have been reading about array's for the past hour try'n to understand it. its only different because before i upload any code to the board i compile it and it gave me an error message. putting in the extra curly bracket got rid of the error message and let it compile

[adafruit_support_bill]

Getting code to compile and getting the logic correct are not necessarily the same. You needed an extra curly brace because they need to be balanced overall. But the correct place to insert it was probably back where you inserted the extra if/else clause.

These things are really difficult to spot when they are nested so deeply. Things that will make the logic easier to follow are:

Use a 'switch' statement instead of a deeply nested if/else as suggested by ChuckM
Be consistent about how you indent your code & braces.

[Spinsycle]

Getting code to compile and getting the logic correct are not necessarily the same. You needed an extra curly brace because they need to be balanced overall. But the correct place to insert it was probably back where you inserted the extra if/else clause.

thank you i will look into that.

I think my brain is gunna start blinking soon.

[ChuckM]

Simplifying the code will help you quite a bit I'm sure, for example the code you wrote for the button press is below (I've re-formatted it to use a 'regular' indent pattern.

Code: Select all

    val2 = digitalRead(switchPin);     // read the input again to check for bounces
    if (val == val2) {                 // make sure we got 2 consistant readings!
        if (val != buttonState) {          // the button state has changed!
            if (val == LOW) {                // check if the button is pressed
                if (lightMode == 0) {          // if its off
                lightMode = 1;               // make it all on!
            } else {
                if (lightMode == 1) {        // if its all on
                    lightMode = 2;             // make it blink!
                } else {
                    if (lightMode == 2) {      // if its blinking
                    lightMode = 3;           // make it wave!
                    } else {
                        if (lightMode == 3) {    //  if its waving,
                            lightMode = 4;         // make it dubble wave!
                        } else {
                            if (lightMode == 4) {  // if its dubble waving
                                lightMode = 5;       // make it reverse wave!
                            } else {
                                if (lightMode ==5) {
                                    lightMode =0;
                                }
                            }
                        }
                    }
                }
            }
            buttonState = val;                 // save the new state in our variable
        }


Something to be aware of is that the last brace here (the } character) is still two braces short of closing off the top level "if" statement. What is worse, I don't believe this code is doing what you think it is doing.

This code is what switch statements were designed for, the form of a switch statement is as follows:

Code: Select all

switch (<expression>) {
case <value-1>:
                <statements>;
                break;
case <value-2>:
                <statements>;
                break;
case <value-n>:
                <statements>;
                break;
default:
              <statements>;
}

In English, the compiler evaluates the expression that is inside the parenthesis and gets back a number. Then it looks for a label of the form "case n:" where n is the same number, so if the expression evaluated to the number 1 it would look for "case 1:" and then it does a 'goto' to that label and starts running the statements it finds. Until it sees the statement 'break' at which point it jumps all the way down to the next statement after the closing brace. Finally, the "special" label is default: this tells the compiler that if you can't find the value in the various case statements, do the one labelled default.

So for your bit of code you probably wanted to write:

Code: Select all

switch (lightMode) {                // 'lightMode' is the expression here
    case 0: lightMode = 1;          // if it was 0 make it 1
               break;
    case 1: lightMode = 2;          // if it was 1 make it 2
               break;
    case 2: lightMode = 3;          // if it was 2 make it 3
               break;
    case 3: lightMode = 4;          // if it was 3 make it 4
               break;
    case 4:  lightMode = 0;          // if it was 4 make it go back to 0
               break;
    default: lightMode = 0;           // if it was anything else, make it 0
               break;
}


If you used a switch statement (and thus simpler braces and indentation) you would see that your program (as written) has unbalanced braces and, as written, won't do anything unless you hold the button down.

--Chuck

[Spinsycle]

Thank you i really appreciate the help. because im new i dont know when to use an if statement and when to use array's and switch statement. im gunna try all this out tomorrow.

[adafruit_support_bill]

Actually, the whole 'lightMode' logic could be simplified even further:

Code: Select all

    lightmode = lightMode + 1; // increment the light mode
    if (lightMode > 4)   // if it exceeds the maximum
    {
        lightMode = 0;  // reset to zero
    }


[ChuckM]

And even easier (but more complicated to read)

Code: Select all

lightMode = (++lightMode) % 5;

Sadly its terribly easy to obfuscate with C, can be fun if you know it well, sort of like telling bawdy puns in English.

That being said, if it is a variation on a state machine then the simplest implementation in C is the array so

Code: Select all

int next_states[] = {1, 2, 3, 4, 0 };

lightMode = next_state[lightMode];


Which maintains a lot of flexibility since you can go to any state from any state, but has the downside that if lightMode becomes corrupted and falls outside the range of allowed states then you are in trouble. Also if you wanted a sparse set of state values you can waste a bit of space with an array which, by neccesity, has memory between its minimum and maximum index.

Don't worry Spin, none of this is critical for your issues at the moment, but later you may find it useful.

--Chuck

[adafruit_support_bill]

And even easier (but more complicated to read)

Best to stick with the basics at this stage. It may be fewer keystrokes, but if it is complicated to read, it really isn't easier now is it?