Simple Arduino code rookie question

[IvoS]

Using ATTiny85,
I am trying to turn ON 3 LEDs in sequence with 1s delay when I press the button. When I press the button again, I want to turn all 3 LEDs OFF simultaneously. And then, the cycle repeats again.
This is what I have: When I press the button for the first time, the initial sequence turning LEDS ON with 1s delay starts OK.  But then, I can not turn LEDs OFF when I press the button again. Any help? I am not sure if I am supposed to write "delay(1000)" in setMode. Thanks.

Code: [Select]

const int LED1 = 0;
const int LED2 = 1;
const int LED3 = 2;
const int button = 3;

boolean lastButton = LOW;
boolean currentButton = LOW;
int ledMode = 0;

void setup ()
{
  pinMode (LED1, OUTPUT);
  pinMode (LED2, OUTPUT);
  pinMode (LED3, OUTPUT);
  pinMode (button, INPUT);
 
}
boolean debounce (boolean last)
{
  boolean current = digitalRead (button);
  if (last != current)
  {
    delay (5);
    current = digitalRead (button);
  }
  return current;
}
void setMode (int mode)
{
  if (mode == 1)
  {
    digitalWrite (LED1, HIGH);
    delay(1000);
    digitalWrite (LED2, HIGH);
    delay(1000);
    digitalWrite (LED3, HIGH);
  }
  else if (mode == 2)
  {
    digitalWrite (LED1, LOW);
   digitalWrite (LED2, LOW);
  digitalWrite (LED3, LOW);
  }
  else
  {
    digitalWrite (LED1, LOW);
   digitalWrite (LED2, LOW);
  digitalWrite (LED3, LOW);
}
}

void loop ()
{
  currentButton = debounce(lastButton);
  if (lastButton == LOW && currentButton == HIGH)
  {
    ledMode++;
  }
  lastButton = currentButton;
  if (ledMode == 2 ) ledMode = 0;
  setMode (ledMode);
}




[senso]

Switch this two:
if (ledMode == 2 ) ledMode = 0;
  setMode (ledMode);

And them think about what you are doing to that ledMode variable BEFORE it is passed to the setMode function.

[IvoS]

I tried that, unfortunately it doesn't work. I am doing something stupid.

[zapta]

Try to add print statements at key points. It will help you to understand what's going on.

[m98]

Well, your Arduino executes the setMode() function in one piece and won't call the loop() function until finished. To respond to the button while the LED blinks, you need to use interrupts.

[Muxr]

I am not convinced that " if (lastButton == LOW && currentButton == HIGH)" is ever satisfied the second time. As I don't see how lastButton gets set to LOW on the 2nd push.

In your loop change your "lastButton = currentButton;" to "lastButton = digitalRead (button);". Just to make sure it's actually being read from the button instead of getting stuck in a logical false state in the debounce function.

Use serial for debugging if you can:

Code: [Select]

Serial.println("sending to serial..");

[SL4P]

This is a really good learning project... don't rush it for someone else to hand you the answer on a plate.
The ideas you are trying to use are all valid, but there are some fundamental oversights which will help you learn.

As Zapta said - add some debugging print statements to see what your code is actually doing.
it should not take long to figure out.

Good luck.   a great next step after the blinking LED "

[IvoS]

Thank you. It kind of works, but I have to hold the button down for 2 sec to turn those LEDs OFF. Sometime 2 seconds, sometimes it turns OFF instantly. I think it gets stuck for 2x delay(1000); to wait for the "setMode" to finish? I thought the "void setMode" is set up only once when ATTiny is powerd up whereas void loop () runs over and over again..... 

[SL4P]

ok, so what happens if you press the button while in, or just about to enter one of the the delay() calls?
What if it just happens elsewhere?

[hneve]

I love pseudokode.

I would think like this:

? Is button pressed
      ? Is timestamp set.
      If set then clear and turn leds off.
      Else set timestamp from millis.

? Is time since timestamp 1 sec.
Then led 1 on.
........
? Is time 3 sec.
Then led 3 on.



This is just a Idea.... I might have thought wrong...

[SL4P]

OP once you have this working as you want it to...
Rewrite it without delay() -- that will teach you a lot about realtime, async event programming (the button).

That will give you a whole new insight to 'better' code development. and structure.   This should be the mandatory second lesson after the blinking LED ,

delay() - in most incarnations - is a 'blocking' function - which inhibits anything else happening for the duration of the delay.

[m98]

Thank you. It kind of works, but I have to hold the button down for 2 sec to turn those LEDs OFF. Sometime 2 seconds, sometimes it turns OFF instantly. I think it gets stuck for 2x delay(1000); to wait for the "setMode" to finish? I thought the "void setMode" is set up only once when ATTiny is powerd up whereas void loop () runs over and over again..... 

As you call setMode() in loop(), it gets called in a ...loop. And because the uC waits for setMode() to complete before doing something else, it can't check the state of the button before the blink sequence is finished. So you could use interrupts or non-blocking code. First ones interrupt the uC from whatever is happening and call a handler, where you could for e.g. change the mode and call your setMode() function again. Non-blocking code would be the more elegant solution, where @hneve posted a good example.

[zapta]

OP once you have this working as you want it to...
Rewrite it without delay() -- that will teach you a lot about realtime, async event programming (the button).

That will give you a whole new insight to 'better' code development. and structure.   This should be the mandatory second lesson after the blinking LED ,

+1

Delay is good for initial blinky but is a dead end otherwise.

After you get this reasonably working start to learn how to program with state machines (a fancy word) and polling instead of blocking delay. You can easily model it as an arbitrary number of arduinos running on the same MCU, one to track the button events, one to control the overall state, one to blink Leeds, etc. All without having to use  interrupts.

Delay is not your friend.

[IvoS]

Thank you. It kind of works, but I have to hold the button down for 2 sec to turn those LEDs OFF. Sometime 2 seconds, sometimes it turns OFF instantly. I think it gets stuck for 2x delay(1000); to wait for the "setMode" to finish? I thought the "void setMode" is set up only once when ATTiny is powerd up whereas void loop () runs over and over again..... 

As you call setMode() in loop(), it gets called in a ...loop. And because the uC waits for setMode() to complete before doing something else, it can't check the state of the button before the blink sequence is finished. So you could use interrupts or non-blocking code. First ones interrupt the uC from whatever is happening and call a handler, where you could for e.g. change the mode and call your setMode() function again. Non-blocking code would be the more elegant solution, where @hneve posted a good example.

I don't need to use the button in the middle when LEDs are sequencing. So I kind of understand what you wrote about it can't check the button before sequence is finished. The problem alone is to turn OFF all 3 LEDs after they been turned ON.
But you are right. I tried to shorten the delay to delay(200) between digitalWrite (LED1, HIGH) etc., and the button responded much quickly to turn LEDs OFF. It must be pressed and held down until it turns OFF.

If I write the code without delays for lets say 8 modes, every button press = next mode, then, there is no problem. So the code structure written like this is no good for this task.
I have a book "Exploring Arduino" from Jeremy Blum, but there are no good examples on this kind of thing. Just a simple examples on digital read write, sensors etc. but very basics on each topic. So, I don't know where to look, tried google, no luck.

[hneve]

Here is some reading for you:
http://playground.arduino.cc/Code/AvoidDelay

[SL4P]

last hint:
write button code that does what you want, then wrap the LEDs around that.

We're not trying to be mean, and you are obviously getting the idea from your most recent attempts.  The essence is that the LEDs are a consequence of the button press, so the button is more important than the LEDs.

Imagine you are developing a bomb detonator... press to 'arm' then press again to stop the countdown... It won't work so well if the bpmb goes off while you're waiting!

The state-machine sounds complicated, and often the explanation makes it sound that way too...  Just think of a counter that increments at each state of operation... idle, press, LED1, LED2, LED3... etc.

In this simple example, one state will be unique (idle)... waiting for something to start the ball rolling, and then to sequence through the remaining 'states'.  Your second button press will clear the runtime conditions, and revert to that first idle condition.

Even with this minimal example, there are a number of options you can consider... eg whether to stop the sequence immediately 'mid-LED' or wait for the current state to complete.   This would mean 'catching' the second press, but holding the event as 'pending' until other condition are met...

This discussion can go on forever. The methodology,  you *must* master timing and asynchronous events before your code is worth talking about!

[IvoS]

Here is some reading for you:
http://playground.arduino.cc/Code/AvoidDelay

This is helpful, thank you.

[Dragon88]

Here's some code I wrote that does what you want (I think). I tested it on an Uno and it works fine:

Code: [Select]

#define redLED 2             //define IO
#define greenLED 3
#define blueLED 4
#define button 5

boolean lightsOn = false;    // state variables
int whichLED = 2;

int counter = 0;            // main counter

void setup()
{
  pinMode(redLED, OUTPUT);      // set up IO
  pinMode(greenLED, OUTPUT);
  pinMode(blueLED, OUTPUT);
  pinMode(button, INPUT);
}

void loop()
{
  if (digitalRead(button) == LOW)    // The button is pressed, handle it
  {
    delay(200);                      // debounce and delay
    lightsOn = !lightsOn;            // toggle state
   
   
    if (lightsOn == false)          // turn LED off and clean up
    {
      counter = 0;
      digitalWrite(whichLED, LOW);
      whichLED = 2;
    } else {                         // Or if we are activating LEDs
      digitalWrite(whichLED, HIGH);  // Turn first LED on to start
    }
  }
 
  if (lightsOn == true)              // Manage LED state
  {
    counter++;                      // Increment counter
 
    if (counter >= 1000)            // ~ 1 second has passed
    {                               // So increment LED state
      counter = 0;                 
      digitalWrite(whichLED, LOW);  // Don't forget to turn old LED off
      whichLED++;
     
      if (whichLED >= 5)            // Don't let whichLED go to invalid state
        whichLED = 2;
       
      digitalWrite(whichLED, HIGH); // Turn new LED on
    }
  }
 
  delay(1);                          // 1 ms delay for 1000 count = ~ 1 second
}

I do believe in figuring it out yourself, but I also know from teaching people that an example is worth a thousand words. Now this is very quick, inefficient code. There are a thousand ways to do things and this is just one. The timing is only approximately 1 second per LED (do you see why?) and it ties up the entire microcontroller from doing other things. But you can modify and grow this code into something much better.

Notice the structure of the main loop. The LED doesn't really get turned on until the end. The entire first part of the loop is figuring out what state we are in, handling the button if it's pressed, and cleaning up as necessary. Once we know what state we should be in, acting on the state is too easy. Also notice that by using a counter and avoiding excessive delays, we can make the button far more responsive. Debouncing (really just delaying while the user removes their finger) becomes very important though with it being checked so frequently. There are far better ways to debounce. But this type of debounce also adds a "feature" or alternate mode of sorts, test it and see how.

Good luck. Make sure you understand this code fully, modify it, and make it your own.

[SL4P]

Dragon88 is right, in giving some basic ideas to push you along...

Another 'feature' you can might become aware of in this type of operation is a 'long press'... e.g hold for 3-secs to clear and reset some other output - buzzer, a cycle counter or whatever...

keep us posted!

[obiwanjacobi]

You can imagine how readable your code gets if you would add a couple of more buttons and a couple of more functions to perform.

That would be a good next step to solve. How to manage you code readability/maintainability with increased complexity...?

For instance think of how a robot main loop would look. How would all the sensor information interleave with all the processing?

Don't feel bad if you don't know, not a lot of people do it right in my view (no using a real-time os with multi threading is not the way to go on a tiny mcu).

[hneve]

I remember once I used the watchdog timer to poll the buttons. A fun experiment, worked fine thou.

[obiwanjacobi]

I remember once I used the watchdog timer to poll the buttons. A fun experiment, worked fine thou.

You have a good point though. I think that interrupts are the secret to making your code 'multi-threading' on MCUs. Think about it, it is hardware supported and has minimal overhead (check the compiled assembler code to make sure).
You have to change how you think about your code - which is hard for most.

There is also another (software) option that is not known to many. It has to do with a strange construction you can do with a switch statement... (see Task.h in my library)

[retrolefty]

It's essential when programming microcontrollers to understand the difference between 'blocking' functions and 'non-blocking' functions and how they can or might effect your application coding. A simple blocking function like delay() can be used in many cases without side effects but other times it can be a deadly bug. There can be many ways to structure your code to avoid such problems but does take understanding of the problem and how best to deal with it.

[IvoS]

Thank you "Dragon88 " so much and all of you guys as well for helping me!
Using "Dragon88" code, I re-arranged few things and I finally have what I wanted! It is a one time sequence:
LED1 ON, delay 1sec, LED2 ON, delay 1 sec, LED3 ON.
I didn't involve all of you into this small project just for fun to blink LEDs!  This is a start up (turn ON) sequencer for my audio amplifier. LEDs will get replaced with:
- relay (to release bleeder resistors),
- opto coupler (to turn POWER (triac) ON)
- relay (to bypass inrush current limiting resistor)

I am also planning to use 2 remaining pins on ATTiny85 for LM35 temperature sensor to sense an overheat and another pin for overheat shut down.
So, many thanks again.
I may still do some tweaks on that code later as I would like to have a different (uneven) time values. I know I can just change the counter value to different value but it always gives me the same delay between LEDs.
Here is the final code:

Code: [Select]

#define redLED 0         //define IO
#define greenLED 1
#define blueLED 2
#define button 3

boolean lightsOn = false;    // state variables
int whichLED = 0;

int counter = 0;            // main counter

void setup()
{
  pinMode(redLED, OUTPUT);      // set up IO
  pinMode(greenLED, OUTPUT);
  pinMode(blueLED, OUTPUT);
  pinMode(button, INPUT);
}

void loop()
{
  if (digitalRead(button) == LOW)    // The button is pressed, handle it
  {
    delay(200);                      // debounce and delay
    lightsOn = !lightsOn;            // toggle state
   
   
    if (lightsOn == false)          // turn LED off and clean up
    {
      counter = 0;
      digitalWrite(redLED, LOW);
      digitalWrite(greenLED, LOW);
      digitalWrite(blueLED, LOW);
      whichLED = 0;
    } else {                         // Or if we are activating LEDs
      digitalWrite(whichLED, HIGH);  // Turn first LED on to start
    }
  }
 
  if (lightsOn == true)              // Manage LED state
  {
    counter++;                      // Increment counter
 
    if (counter >= 1000)            // ~ 1 second has passed
    {                               // So increment LED state
      counter = 0;                 
      digitalWrite(whichLED, HIGH);  // Don't forget to turn old LED off
      whichLED++;
     
      if (whichLED >= 3)            // Don't let whichLED go to invalid state
        whichLED = 3;
       
      digitalWrite(whichLED, HIGH); // Turn new LED on
    }
  }
 
  delay(1);                          // 1 ms delay for 1000 count = ~ 1 second
}





[zapta]

This code is complicated.