Frustrated, Frustrated, Im back again with more Frustration.

Hi Guys, Re my post, Frustrated, Frustrated with "while()" date May 24th, I thought I had this problem sorted, Im finding as soon as I solve one problem another two show up. Re advice and code given by Nick Gammon, it works ok to a degree. Problem (1) I want the LEDs to come on and off when the button is pressed and released, at my timeing ie, do away with all the "Delays", once FIRST LED13 has gone off, the code should move on to next or second SECOND LED. I am thinking of a " GOTO " command here, but thats discouraged in Arduino. Problem (2) I forsee at start of the run, FIRST LED13, will be LOW so code will run straight to SECOND LED12 ( before I have time to activate FIRST LED13)Does this make sense ? I think if I had a different "input" to each LED, my problem would be easier, but it has to be one input controlling all LEDs. Am I asking the impossible ? Thanks, Chez7. PS, How do I download my sketch to this forum ? I try " Copy to forum", It shows on post as a lot of #color cc bla bla,in the code.

define FIRST_LED 13 // LED connected to: digital pin 13

define SECOND_LED 12

void setup() { pinMode(FIRST_LED, OUTPUT); // sets the digital pin as output pinMode(SECOND_LED, OUTPUT); // and for the second LED } // end of setup

void loop() { int input; // variable to read into

do { input = digitalRead(7); //using pin 7 as input } while (input == 0); // end of do loop

digitalWrite(SECOND_LED, HIGH); // turns the LED on delay(2000); // waits for a second digitalWrite(SECOND_LED, LOW); // turns the LED off delay(2000); // waits for a second

do { input = digitalRead(7); //using pin 7 as input } while (input == 0); // end of do loop

digitalWrite (FIRST_LED, HIGH); // turns the LED on delay(2000); // waits for a second digitalWrite(FIRST_LED, LOW); // turns the LED off delay(2000); // waits for a second

} // end of loop

Chez7

I'd like to see you provide a better description of what you wish to do. Including how many LED's and EXACTLY the FULL sequence of what you wish to happen. Doing so will make it easier for you to work out your problem as well as others to provide assistance.

Make no assumptions about who read previous threads.

Oooooooo oOoooooo ooOooooo oooOoooo ooooOooo oooooOoo ooooooOo oooooooO ooooooOo oooooOoo ooooOooo oooOoooo ooOooooo oOoooooo Oooooooo

The following is one of many possible ways to reorganize your code for modularity and easier reading. Note the removal of your somewhat redundant and uninformative comments.

// --- USEFULL ENUM

// --- PIN ASSIGNMENT
enum
{
    // INPUT(S)
      pinBUTTON =  7

    // OUTPUT(S)
    , pinLED1   = 13
    , pinLED2   = 12
};


// THESE MAKE IT EASIER TO READ LED STATES WITHOUT THE USELESS CODE COMMENTS
enum
{
      LED_OFF   = LOW
    , LED_ON    = HIGH
};


// --- SUBROUTINE(S)

// FLASH THE LED AT PIN 'pinLED' ON FOR 'dtOn' MILLISECONDS, TURN IT OFF AND WAIT 'dtReturn'
// MILLISECONDS BEFORE RETURNING
//
// pinLED   - pin number of LED
// dtOn     - delta time for ON, miliseconds for the LED to be ON
// dtReturn - delta time for OFF, miliseconds for the LED to be OFF before returning

void flashLED_ON_OFF(uint8_t pinLED, int dtOn, int dtReturn)
{
    digitalWrite(pinLED, LED_ON);
    delay(dtOn);

    digitalWrite(pinLED, LED_OFF);
    delay(dtReturn);
}


// --- REQUIRED CODE

void loop()
{
    do
    {
    } while ( !digitalRead(pinBUTTON) );
    
    flashLED_ON_OFF(pinLED2, 2000, 2000);

    do
    {
    } while ( !digitalRead(pinBUTTON) );
    
    
    flashLED_ON_OFF(pinLED1, 2000, 2000);
}


void setup()
{
    pinMode(pinBUTTON, INPUT);

    pinMode(pinLED1, OUTPUT);
    pinMode(pinLED2, OUTPUT);
}
[CODE]

[/code]

Hi Des, First of all at this stage I am only useing Two LEDs until I can get the ON _ OFF sequence right ( later I will add others) I have, 2 LEDs connected to pins 13 and 12 1 Pushbutton connected to input pin 7 What I am trying to achieve is :-

a) After upload,------ No Leds HIGH,(ON)

b) Depress pusbutton,--------- LED13 ON and stays ON while pushbutton depressed.

c) Release pushbutton,---------LED13 OFF.

d) Depress pusbutton,--------- LED12 ON and stays ON while pushbutton depressed.

e) Release pushbutton,---------LED12 OFF.

Once I can sort out the basics of this sequence, I will add more LEDs to the programme. Thanks Chez7.

Based on your frustration I thought it best to provide a solution, that hopefully works, as a starting point from which you should have a better chance of learning to move forward.

What follows is a slight modification of my last post. Note ‘flashLED_ON_OFF’ is never called and could be removed.

Code compiles but is otherwise untested!

// --- USEFULL MACROS

#define SIZEOF_ARRAY(ARRAY)     (sizeof(ARRAY)/sizeof(ARRAY[0]))


// --- USEFULL ENUM

// --- PIN ASSIGNMENT
enum
{
    // INPUT(S)
      pinBUTTON =  7

    // OUTPUT(S)
    , pinLED1   = 13
    , pinLED2   = 12
};


// THESE MAKE IT EASIER TO READ LED STATES WITHOUT THE USELESS CODE COMMENTS
enum
{
      LED_OFF       = LOW
    , LED_ON        = HIGH
};

enum
{
      BUTTON_UP     = HIGH
    , BUTTON_DOWN   = LOW
};


// --- GLOBALS

const uint8_t arrayLED[] = { pinLED1, pinLED2 };

int indexLED;


// --- SUBROUTINE(S)

// FLASH THE LED AT PIN 'pinLED' ON FOR 'dtOn' MILLISECONDS, TURN IT OFF AND WAIT 'dtReturn'
// MILLISECONDS BEFORE RETURNING
//
// pinLED   - pin number of LED
// dtOn     - delta time for ON, miliseconds for the LED to be ON
// dtReturn - delta time for OFF, miliseconds for the LED to be OFF before returning

void flashLED_ON_OFF(uint8_t pinLED, int dtOn, int dtReturn)
{
    digitalWrite(pinLED, LED_ON);
    delay(dtOn);

    digitalWrite(pinLED, LED_OFF);
    delay(dtReturn);
}


// --- REQUIRED CODE

void loop()
{
    if ( BUTTON_DOWN == digitalRead(pinBUTTON) )
    {
        // TURN ON LED AT CURRENT 'indexLED'
        digitalWrite(arrayLED[indexLED], LED_ON);

        // MAY WISH TO PLACE A SMALL DELAY HERE TO ALLOW FOR BUTTON DEBOUNCE
        // BUSY WAIT WHILE BUTTON IS DEPRESSED
        while ( BUTTON_DOWN == digitalRead(pinBUTTON) )
        {}

        // TURN OFF LED AT CURRENT 'indexLED'
        digitalWrite(arrayLED[indexLED], LED_OFF);

        // INCREMENT 'indexLED'
        ++indexLED;
        
        // IF 'indexLED' IS AT END OF ARRAY SET 'indexLED' BACK TO FIRST LED ARRAY ENTRY
        // (% is MODULO FUNCTION - LOOK IT UP)
        indexLED %= SIZEOF_ARRAY(arrayLED);
    }
}


void setup()
{
    pinMode(pinBUTTON, INPUT);

    for ( int i = 0; i < SIZEOF_ARRAY(arrayLED); i++)
    {
        pinMode(arrayLED[i], OUTPUT);
        digitalWrite(arrayLED[i], LED_OFF);
    }

    // SET LED ARRAY INDEX TO FIRST LED ARRAY ENTRY
    indexLED = 0;
}

Chez7:
What I am trying to achieve is :-

a) After upload,------ No Leds HIGH,(ON)

b) Depress pusbutton,--------- LED13 ON and stays ON while pushbutton depressed.

c) Release pushbutton,---------LED13 OFF.

d) Depress pusbutton,--------- LED12 ON and stays ON while pushbutton depressed.

e) Release pushbutton,---------LED12 OFF.

I like to keep things simple. Now I know what you are trying to do, this does it:

const byte button = 7;  // pin for button
const byte leds [] =  { 13, 12, 11, 10, 9, 8 };  // pins for LEDs

// get number of items in an array
#define NUMITEMS(arg) ((unsigned int) (sizeof (arg) / sizeof (arg [0])))

void setup () 
 {
 // set all LEDs to outputs
 for (byte i = 0; i < NUMITEMS (leds); i++)
   pinMode (leds [i], OUTPUT);
   
 digitalWrite (button, HIGH);  // enable pull-up for button
 } // end of setup

void loop ()
{
  
 for (byte i = 0;  i < NUMITEMS (leds); i++)
 {
    // wait for button press
    do {} while (digitalRead (button) == HIGH);
    
    delay (5);  // debounce
    
    // turn this LED on
    digitalWrite (leds [i], HIGH); 
    
    // wait for button release
    do {} while (digitalRead (button) == LOW);

    delay (5);  // debounce
    
    // turn the LED off again
    digitalWrite (leds [i], LOW); 
   
 } // end of for loop
  
} // end of loop

You specify the pins for your LEDs in the second line of the sketch. They don’t have to be in sequence. It then automagically cycles through all of them turning them on when you press the button, and turning them off when you release it.

Pretty much what I have with a few exceptions.

I use anonymous enums to help document the code making it easier to read and modify should pin numbers change and button states invert, etc... It also cuts reduces program size and memory usage.

Example: const byte button = 7;

While this only occupies a single byte after awhile a byte here and a byte there an soon you're looking at random resets or worse. Not for a project this size but something larger ...

Habits based upon years of experience. And something I never see presented here.

Yes, good idea. Another approach is to use #define.

My personal view though is that this just makes it harder to find what is going on:

const uint8_t arrayLED[] = { pinLED1, pinLED2 };

Why not just say:

const uint8_t arrayLED[] = { 13, 12 };

Then you see your LEDs in a nice list, and in the order in which they will turn on.

But, hey, I don't want to get into a language war. There is more than one way of cooking a goose. :-)

I'm not looking for, or planning on starting, a language war.

I'm not fan of the preprocessor, especially for new users. Macro expansion "bugs" can be very hard to track down so I only used them where there's no real alternative.

And as to:

enum
{
    // INPUT(S)
      pinBUTTON =  7

    // OUTPUT(S)
    , pinLED1   = 13
    , pinLED2   = 12
};

const uint8_t arrayLED[] = { pinLED1, pinLED2 };

Forgetting this small project and something for along the line of:

if ( HIGH == digitalRead(7) )
{
 ...
}

if ( HIGH == digitalRead(6) )
{
 ...
}

 ...

What is on pin 7 & 6. What about if I wish to use pin 3 & 5 instead. What does a check for HIGH signify. ON, OFF, UP, DOWN ...?

WIth the enum (yes define could be used - again my preference) method I can change the value assigned to the enum - in one place - and all code the that uses those values need not be modified (which if something is missed leads to a debugging session). Also they are more self documenting requiring less noisy comments that become less accurate as development progresses.

Yes there are a near infinite number of ways to write even this simple application and I think mine is more self documenting and easier to debug and modify. And yes definitely a little more verbose up front but easier to read later ...

EDIT:

And oh, for my own code I'd definitely name and comment things differently. It's hard for to know what each of the people here know or don't know so I try to add things I wouldn't do for myself or an employers code. Each one of which has their own programming and naming conventions anyway ...

Thanks guys for your input, Lots for me to think about, will work on both suggestions, good to keep it simple and straightforward for a newbie. Am really pleased that my idea is workable, and I will be able to apply it to my musical project. Cheers Chez7.

In case all the comments and white space scare you mine really condenses down to -

#define SIZEOF_ARRAY(ARRAY)     (sizeof(ARRAY)/sizeof(ARRAY[0]))

enum
{
    // INPUT(S)
      pinBUTTON =  7

    // OUTPUT(S)
    , pinLED1   = 13
    , pinLED2   = 12

    // LIGHT STATES
    , LED_OFF       = LOW
    , LED_ON        = HIGH

    // SWITCH STATES
    , BUTTON_UP     = HIGH
    , BUTTON_DOWN   = LOW
};

const uint8_t   arrayLED[] = { pinLED1, pinLED2 };
int             indexLED;

void loop()
{
    if ( BUTTON_DOWN == digitalRead(pinBUTTON) )
    {
        digitalWrite(arrayLED[indexLED], LED_ON);

        while ( BUTTON_DOWN == digitalRead(pinBUTTON) )
        {}

        digitalWrite(arrayLED[++indexLED], LED_OFF);
        indexLED %= SIZEOF_ARRAY(arrayLED);
    }
}


void setup()
{
    pinMode(pinBUTTON, INPUT);

    for ( int i = 0; i < SIZEOF_ARRAY(arrayLED); i++)
    {
        pinMode(arrayLED[i], OUTPUT);
        digitalWrite(arrayLED[i], LED_OFF);
    }

    indexLED = 0;
}

lloyddean: Example: const byte button = 7;

While this only occupies a single byte after awhile a byte here and a byte there an soon you're looking at random resets or worse. Not for a project this size but something larger ...

Common misconception. It doesn't take any space. Try it.

#define first_pin 1
const int second_pin = 2;
int third_pin = 3;

void setup(void)
{
  pinMode(first_pin,OUTPUT);
  pinMode(second_pin,OUTPUT);
  pinMode(third_pin,OUTPUT);

  digitalWrite(first_pin,HIGH);
  digitalWrite(second_pin,HIGH);
  digitalWrite(third_pin,HIGH);
}

void loop(void)
{
}

...compiles down to...

.global setup
    .type   setup, @function
setup:
    .stabd  46,0,0
    .stabn  68,0,5,.LM2-.LFBB2
.LM2:
.LFBB2:
/* prologue: function */
/* frame size = 0 */
    .stabn  68,0,7,.LM3-.LFBB2
.LM3:
    ldi r24,lo8(1)
    ldi r22,lo8(1)
    call pinMode
    .stabn  68,0,8,.LM4-.LFBB2
.LM4:
    ldi r24,lo8(2)
    ldi r22,lo8(1)
    call pinMode
    .stabn  68,0,9,.LM5-.LFBB2
.LM5:
    lds r24,third_pin
    ldi r22,lo8(1)
    call pinMode
    .stabn  68,0,11,.LM6-.LFBB2
.LM6:
    ldi r24,lo8(1)
    ldi r22,lo8(1)
    call digitalWrite
    .stabn  68,0,12,.LM7-.LFBB2
.LM7:
    ldi r24,lo8(2)
    ldi r22,lo8(1)
    call digitalWrite
    .stabn  68,0,13,.LM8-.LFBB2
.LM8:
    lds r24,third_pin
    ldi r22,lo8(1)
    call digitalWrite
/* epilogue start */
    .stabn  68,0,14,.LM9-.LFBB2
.LM9:
    ret
    .size   setup, .-setup
.Lscope2:
    .stabs  "",36,0,0,.Lscope2-.LFBB2
    .stabd  78,0,0
.global third_pin
    .section    .data.third_pin,"aw",@progbits
    .type   third_pin, @object
    .size   third_pin, 2
third_pin:
    .word   3

Notice the LDI's for pins 1 & 2. Same treatment for a #define'd pin as a const pin. Only pin #3 uses a LDS, and you can see only third_pin has a data section which takes up space.

Indeed, and you can see it in the way this compiles:

const int count = 10;
char buf [count];
void setup () {}
void loop () {}

That compiles OK, but how can it if the size of the array has to be allocated from an int at runtime? The fact is that const variables are effectively as efficient as enums (perhaps less type-safe but that wasn't the argument).

Get rid of the const and you get this:

int count = 10;
char buf [count];
void setup () {}
void loop () {}

Which gives this error:

sketch_may30d:3: error: array bound is not an integer constant

Attention, Nick Gammon and Des Moines, Thanks for your help guys, But neither system works.

Nick operation : On upload-------Led13 on and stays on ( no button pressed )

Press button--------Led13 off ( nothing on ) Keep pressing button, both Leds
remain Off.

Des operation : On upload-------Led13 on and stays on ( no button pressed )

Press button-------Led13 stays on PLUS Led12 comes On ( and stays this way even when button released )

Must be more to this problem than meets the eye. Back to the drawing board. Cheers Chez7

I don’t know if this is helpful, but I tried to take a shot at your problem and I got the LEDs to toggle from a single input switch, but the toggling seems inconsistent and i think it’s due to the overall speed of the void loop of the arduino board. This has sorta had me stuck for a bit… does anyone know how to deal with that?

Notice that i dropped the time of the delay, for faster shutoff of each LED… hope this helps a bit oh and look at the input and output pins

Chez7: Attention, Nick Gammon and Des Moines, Thanks for your help guys, But neither system works.

Nick operation : On upload-------Led13 on and stays on ( no button pressed )

Press button--------Led13 off ( nothing on ) Keep pressing button, both Leds remain off ...

Must be more to this problem than meets the eye. Back to the drawing board. Cheers Chez7

Well I did test it, you know. So your hardware setup must be different to mine. I had my switch set up so that you grounded it to press it. That is because it had a pull-up resistor (set in the program).

So for you to find that pressing turns it off rather than on, sounds like you have the switch wired differently (maybe to +5V). Also, since I had this:

const byte leds [] =  { 13, 12, 11, 10, 9, 8 };  // pins for LEDs

If you don't have that many LEDs then you will find switch presses don't seem to do anything because there are no LEDs there. If you just have 13 and 12 you (rather obviously) change it to read:

const byte leds [] =  { 13, 12 };  // pins for LEDs

Chez7: Attention, Nick Gammon and Des Moines...

Reminds me of the time someone got called Dallas.

Dallas is a city. Des Moines is a city. The name of the poster is in orange.

Finally got the code to work correctly, sorry it's messy and can be cleaned up... But I can assure you this works. Just as Mr. Gammon said earlier, If you don't have a resistor connected to the ground of your push button switch, then upon the upload of this code, one your circuit's LED will start in the ON position and this code will turn the LED off upon each button push.*** I have this setup and tested this circuit, so I can provide video or pictures if necessary. Hope this helps...

Oh, again notice my digital pins are set differently then your original setup i'm using digital pins 8 and 12 for the LEDs and digital pin 7 for the button

const int FIRSTLED = 8;                      //referring to the digital pin 8
const int SECONDLED = 12;                         //referring to the digital pin 12
const int buttonpin = 7;                              //buttonpin is your original "input"
int buttonstate = 0;                                    //this will be designated to the buttonpin variable in order to monitor the state of the switch
 int nxt = 0;                                               // this is a counter to keep track of which if statement to cycle through (so that the LEDs will toggle between one another)
void setup() {
  
pinMode(FIRSTLED, OUTPUT);                      //digital output referring to first LED pin 8
pinMode(SECONDLED, OUTPUT);                  //digital output referring to second LED pin 12
pinMode(buttonpin, INPUT);                        //input for button (digital input)
 Serial.begin(9600);                                   //This was just to monitor the push switch via serial monitor (not needed)
}

void loop() {
   buttonstate=digitalRead(buttonpin);           //Will read the state of the push button
  int sensorValue = digitalRead(buttonpin);     //Used just to monitor the connection (not needed)
  Serial.println(sensorValue, DEC);                //Used just to monitor the connection (not needed)


 //  The counter, nxt, was initially set to zero.  So after upload and once the button is pressed and held down, the board will enter this first "if" statement set. (b


                                                                // beginning of first "if" statement set---------------------

  if (buttonstate==HIGH && nxt==0)               // If the switch is pushed AND the counter  (nxt) is 0 it will continue to turn on the SECONDLED
  {
      do                                                      // Will only enter this do loop when above if statement it met
    {
     buttonstate=digitalRead(buttonpin);          // looks redundant but could not get the LED to turn off without this do_while loop
     if (buttonstate == HIGH)
       {
       digitalWrite(SECONDLED, HIGH);             // turns the SECONDLED on
      delay(2);                                             // small delay
      digitalWrite(FIRSTLED, LOW);                 // turns the FIRSTLED off
      delay(2);                                             // small delay
       }  
        else   
          {
          digitalWrite(SECONDLED, LOW);           // turns the SECONDLED off
           }
            nxt = 1;                                         //Turns the counter to a 1 so that next time the button is pushed the alternate "if" statement set is used
        }                                                      // end of do loop 
       while (buttonstate == HIGH);                 // will continue to loop while the button is pressed 
   }                                                           //end of first "if" statement set  ------------


  
                                                               // beginning of second "if" statement set---------------------

  if (buttonstate==HIGH && nxt==1)              // If the switch is pushed AND the counter  (nxt) is 1 it will continue to turn on the FIRSTLED
  {
   do                                                        // Will only enter this do loop when above if statement it met
    {
      buttonstate=digitalRead(buttonpin);       // Reads the state of the button
       if (buttonstate == HIGH)
       {
      digitalWrite(FIRSTLED, HIGH);               // turns the FIRSTLED on
      delay(2);                                           // small delay
      digitalWrite(SECONDLED, LOW);            // turns the SECONDLED off
      delay(2);                                           // small delay
      }
        else 
         {
          digitalWrite(FIRSTLED, LOW);          // turns the FIRSTLED off
         }
          nxt=0;                                         //Turns the counter to a 0 so that next time the button is pushed the alternate "if" statement set is used
     }                                                     // end of do loop
   while (buttonstate == HIGH);                 // will continue to loop while the button is pressed 
  }                                                        // of second "if" statement set-----------------
    
}                                                         //end of void loop

" EUREKA", Shame on me, I got my wires crossed. I have a small defence, I am useing a " Freetronic Eleven board " and the pin indications leave a lot to be desired I need a magnifying glass to read them. However everything is working just fine now Thankyou, 21CE and Mr Gammon. Cheers Chez7.