State variables and millis help.

Hi guys,

First post, hello all!

I’ve only recently started with arduino, primarily as a way into programming with C. I’ve done a little assembly with PIC’s, but all the advice suggested learning C.

So on to the problem… I’m trying to write a programme for a UV exposure box, so it’s all about timers and LCD’s really.

I’ve managed to track down code/library for the LCD menu and modified that to suit my needs. Now I need to get to grip with timers, so I thought start small and move on from there. What I’m trying to achieve is when a button is pressed I get a beep from my buzzer.

I’ve looked at blink without delay, and also read this very useful post by Robin2:-

I’ve cobbled some code together but I’m not getting the behavior I expect…
When the button is pressed, I get a recurring bleep and on release the buzzer remains in one state or the other.

Here is the code:-

const int buttonPinEnter = 11;   // pin for the Enter button
const int buzzerPin = 12;        // pin for the buzzer

int lastButtonPushed = 0;

int lastButtonEnterState = LOW;   // the previous reading from the Enter input pin

long lastEnterDebounceTime = 0;  // the last time the output pin was toggled
long debounceDelay = 200;    // the debounce time

int buzzerState = LOW;
long timeThen = 0;    //declare local variables
long AlarmDelay = 200;


void setup()
{
    pinMode(buttonPinEnter, INPUT);
    pinMode(buzzerPin, OUTPUT);

    
}  // setup()...

void loop()
{

  readButtons();  
  if (lastButtonPushed == buttonPinEnter){
    AlarmTest();
  }
    else {
      lastButtonPushed =0;
    
  }
  //lastButtonPushed = 0;
                  
} //loop()... 

void  readButtons(){                    //read buttons status
  int reading;
  int buttonEnterState=LOW;             // the current reading from the Enter input pin
  int buzzerPin=LOW;                    //the current buzzer pin state

  //Enter button
                  // read the state of the switch into a local variable:
                  reading = digitalRead(buttonPinEnter);

                  // check to see if you just pressed the enter button 
                  // (i.e. the input went from LOW to HIGH),  and you've waited 
                  // long enough since the last press to ignore any noise:  
                
                  // If the switch changed, due to noise or pressing:
                  if (reading != lastButtonEnterState) {
                    // reset the debouncing timer
                    lastEnterDebounceTime = millis();
                  } 
                  
                  if ((millis() - lastEnterDebounceTime) > debounceDelay) {
                    // whatever the reading is at, it's been there for longer
                    // than the debounce delay, so take it as the actual current state:
                    buttonEnterState=reading;
                    lastEnterDebounceTime=millis();
                  }
                  
                  // save the reading.  Next time through the loop,
                  // it'll be the lastButtonState:
                  lastButtonEnterState = reading;

                  //records which button has been pressed
                  if (buttonEnterState==HIGH){
                    lastButtonPushed=buttonPinEnter;

                  }else{
                    lastButtonPushed=0;
                  }                  
}



void AlarmTest() {
  
  
  unsigned long timenow = millis();
  
  if ((timenow - timeThen) > AlarmDelay) {
    timeThen = timenow;  // save last time buzzer on
    
    if (buzzerState == LOW)  //toggle buzzer
      buzzerState = HIGH;
    else
      buzzerState = LOW;
      
    digitalWrite(buzzerPin, buzzerState);  //set buzzer action
    
  }
}

Can anyone point me towards the problem?

Thanks in advance, Steve.

Is there a pulllup or pulldown on your button?
Best practice: change this line
pinMode(buttonPinEnter, INPUT);
to
pinMode(buttonPinEnter, INPUT_PULLUP);
and have your button connect the pin to Gnd when pushed.
Then check for a LOW and act on that - button will read HIGH when not pushed.

// (i.e. the input went from LOW to HIGH), and you've waited
changes to
// (i.e. the input went from HIGH to LOW), and you've waited

In addition to what @CrossRoads said, I would change the organization of the code a little – for example

void loop() {
   currentMillis = millis();
   readButtons();
   checkAlarm();
   soundAlarm();

}

void activateAlarm() {
   if (buttonPressed = true) {
     alarmStartMillis = millis();
     alarmSounding = true;
   }
}

void soundAlarm() {
  if (alarmSounding == true) {
      if (currentMillis - alarmStartMillis <= alarmDurationMillis) {
         // whatever makes the noise
      }
      else {
         // whatever stops the noise
         alarmSounding = false;
      }

  }
  else
  }
}

And I suggest you start with a simplified sketch that doesn’t bother with switch debouncing.

…R

You need a function to shut off the alarm when you release the button.

However, the way you’ve debounced your button is confusing. There’s no need for a lastButtonPushed variable, just test the state of the button itself, like this:

  if (lastButtonEnterState == LOW){
    AlarmTest();
  }
    else {
      lastButtonPushed =0;
      digitalWrite( buzzerPin, LOW ); // <== Turns off buzzer when the button is off.
    
  }

Thanks for your replies,

@CrossRoads

I have 10k resistors to ground so pulldowns, it won't be a problem to change them, but as a matter of interest why is it best practice?
Is it something to do with how much the MCU can sink or source current?

@Robin2

OK, I thought that I had reduced the code to small enough functions, but clearly I have a great deal more to learn!!! I had an idea that if I could get the bleep on a button press, then later I could expand that to a short series of bleeps as an alarm-test. I will do as you suggest and simplify the code further.

@Jiggy-Ninja

I thought it was something like that, I wasn't sure exactly what was going wrong though!

To all,
I'm sorry about the confusion over the lastButtonPushed variable, I should have made it clearer this an abstract from other code.
There are actually four buttons to control the menus and they do need to be de-bounced, since eventually they will be incrementing and decrementing counters for the various timers.

I am quite happy to post the full code, if it helps, but I thought it might confuse matters.

Once again THANKS for the help it is most appreciated.

Steve.

bance:
and simplify the code further.

I try (and probably fail frequently) to make my functions as self contained as possible so that they can be copied into a simpler test sketch and should work.

However, in a contradiction of that, I prefer to use global (rather than static local) variables in Arduino code because I can see more clearly how I am using the limited memory. So if I copy a function to another sketch I probably also have to copy the relevant global variables as well.

In general I would not use global variables in a PC program where there is almost unlimited memory (well, how long would it take me to type 2 gigabytes of rubbish?).

...R

Connecting a pin to Gnd is best practice because you are less likely to damage something vs accidentally grounding your +5V to Gnd, which we have seen happen. Take out your regulator, take out your PC USB supply.
Internal pullup resistor limits current flow to <0.5mA, nice & safe. No extra parts needed.

   if (buttonPressed = true) {

Oops.

OK guys,

I’m sorry I’m a little late replying, we’ve had some illness in the family, luckily it’s not serious, but it has consumed more of my time than usual!

Well where shall I start? HHHMM…

@CrossRoads

That makes complete sense, I shall make it “Force of habit” !!!

@Robin2

I had assumed that your intention, by breaking the code down to separate “activate” and “soundalarm” functions was to make it clearer for a noob like me to understand what was going on in the code.

Anyway, I ended up re-writing the code several times before I was able to achieve something even remotely like what I require.

I added some serial output to help with debugging, something I shall bear in mind for the future…

Here is the code as it stands:-

const int buttonPinEnter = 11;   // pin for the Enter button
const int buzzerPin = 12;        // pin for the buzzer

int lastButtonPushed = 0;
int lastButtonState = HIGH;   // the previous reading from the Enter input pin
int buzzerState = LOW;
int buttonEnterState=HIGH;             // the current reading from the Enter input pin

boolean alarmSounding = false;

long AlarmDelay = 200;
long currentMillis =0;
long alarmStartMillis = 0;

void setup()
{
  pinMode(buttonPinEnter, INPUT);
  pinMode(buzzerPin, OUTPUT);
  Serial.begin(9600);   //for debugging    
}  // setup()...

void loop(){
  currentMillis = millis();
  readButtons();  
  activateAlarm();
  soundAlarm();  
} //loop()... 

void  readButtons(){                    //read buttons status
  int buttonState;


  //Enter button
  // read the state of the switch into a local variable:
  buttonState = digitalRead(buttonPinEnter);
  // check to see if you just pressed the enter button 
  // (i.e. the input went from HIGH to LOW )                  
  if (buttonState != lastButtonState) {
    // If the switch changed, due to noise or pressing:
    if (buttonState == LOW) {
      buttonEnterState = buttonState;
      Serial.println("button pushed");
    }                

    // save the reading.  Next time through the loop,
    // it'll be the lastButtonState:
    lastButtonState = buttonState;

    //records which button has been pressed
    if (buttonEnterState==buttonState){
      lastButtonPushed=buttonEnterState;
      Serial.println("buttton set");
    }
    else{
      lastButtonPushed=1;
    }                  
  }
}           

void activateAlarm(){

  if (lastButtonPushed == buttonEnterState) {
    alarmStartMillis = millis();
    alarmSounding = true;
    Serial.println("timer started");
  }
}

void soundAlarm() {
  if (alarmSounding == true) {
    if (currentMillis - alarmStartMillis <= AlarmDelay) {
      buzzerState = HIGH;
      Serial.println("alarm started");
    }
    else {
      buzzerState = LOW;
      alarmSounding = false;
      Serial.println("alarm stopped");
    }
  }  
  digitalWrite(buzzerPin, buzzerState);
}

As you can see, I’ve used the button change state code to trigger the alarmdelay (duration). (at least, that’s what I think I’m doing!!)

However, what is actually happening, is that the alarm(buzzer) sounds as soon as the button is pressed, and the alarmdelay(duration) “period” is ‘run’ when the button is released.

This is particularly noticable if the delay time is extended, for instance to 500ms.

Looking at the serial output :-

button pushed
buttton set
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm stopped

We get multiple instances of ‘timer started’, is this due to switch bounce, and the alarm actually starts at the first instance of ‘alarm started’ ?

No it isn’t, it can’t be, since if the button is held down, the alternating pattern continues…

button pushed
buttton set
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
timer started**
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm started
alarm sto

I have a feeling that I should change something in the function ‘void activateAlarm()’?

I should say I’m trying to get a bleep of a certain duration whether the button is pressed or held…

Thanks in advance,

Steve.

I like to start with very simple stuff and only make it more complex if possible. So what about this

byte buttonState = false; // global variable

void readButtons() {
  buttonState = digitalRead(enterButtonPin);
}

void activateAlarm(){

  if (buttonState) {
    alarmStartMillis = millis();
    alarmSounding = true;
    Serial.println("timer started");
  }
}

etc etc

If you keep the button pressed strange things will happen and you could extend the code to deal with that - but is that necessary?

...R

Hi Robin,

Thank’s for your patience, I’m sure it must get tedious, answering the same questions over and over again!

I’ve re-written the code, simplifying the button press as you suggested, but I’m afraid I get exactly the same behavior as before.
i.e. The buzzer sounds as soon as the button is pressed, and when the button is released, then the delay kicks in. Just to be clear the behavior I was expecting is:-

Button pressed => buzzer sounds =>timed delay(100/200ms) => buzzer stops (regardless of whether the button is held down or not.)

The behavior I get is:-

Button pressed => buzzer sounds => button released => (read as delay starts here) buzzer continues until delay is up => buzzer stops.

Perhaps, we could leave that aside for a moment (I’m a little frustrated.) As far as I can see it, the concept of using Millis() as a timed delay, works like this:-

We issue a variable “A” with a timestamp (Millis()), next we issue another variable “B” with a later timestamp (+Millis()), then we compare the difference of the two variables against a known quantity “I” (interval), then depending on whether the difference is larger or smaller than the quantity “I” (interval) a descision is made.

So to make a truth table of this we get:-

B - A > I = true
B - A = I = true
B - A < I = false

Am I correct?

Here is the simplified version of my/your code:-

//Code:


const int buttonPinEnter = 11;   // pin for the Enter button
const int buzzerPin = 12;        // pin for the buzzer


int buzzerState = LOW;          //variable to hold the state of the buzzer
byte buttonState = true;        //variable to hold the state of the button

boolean alarmSounding = false;

long AlarmDelay = 500;
long currentMillis =0;
long alarmStartMillis = 0;

void setup()
{
  pinMode(buttonPinEnter, INPUT);
  pinMode(buzzerPin, OUTPUT);
  Serial.begin(9600);   //for debugging    
}  // setup()...

void loop(){
  currentMillis = millis();
  readButtons();  
  activateAlarm();
  soundAlarm();  
} //loop()... 

void  readButtons(){                    //read buttons status

  //Enter button
  // read the state of the switch into a variable:
  buttonState = digitalRead(buttonPinEnter);
  
}           

void activateAlarm(){

  if (buttonState == false) {
    alarmStartMillis = millis();
    alarmSounding = true;
    Serial.println("timer started");
  }
}

void soundAlarm() {
  if (alarmSounding == true) {
    if (currentMillis - alarmStartMillis <= AlarmDelay) {
      buzzerState = HIGH;
      Serial.println("alarm started");
    }
    else {
      buzzerState = LOW;
      alarmSounding = false;
      Serial.println("alarm stopped");
    }
  }  
  digitalWrite(buzzerPin, buzzerState);
}

I’ve stepped through the code and I think it should work… Probably a loose wire in front of the keyboard :blush:

If you keep the button pressed strange things will happen and you could extend the code to deal with that - but is that necessary?

This comment got me thinking… and actually I should be able to do this simply using delay(), since the MCU will only countdown, update the lcd and switch a mosfet on and off.

However I might need an external interupt to abort the countdown sequence should there be a problem, would that be a problem if using delay()?

Anyway I feel that I should persist with millis() delay, even just so that I understand it and am able to use the full flexibility of the MCU.

Regards Steve.

I see several problems with your code, starting with over-dependence on global variables.

void  readButtons(){                    //read buttons status

  //Enter button
  // read the state of the switch into a variable:
  buttonState = digitalRead(buttonPinEnter);
  
}

There is no reason for this function to not return a value. Actually, there is no need for a function that does nothing but call another function.

void activateAlarm(){

  if (buttonState == false) {
    alarmStartMillis = millis();
    alarmSounding = true;
    Serial.println("timer started");
  }
}

You should not even be calling this function if buttonState is not HIGH (or LOW).

The digitalRead function does not return true or false. Don't test for true or false.

The Serial.print() statement is wrong. You are not using a timer. You recorded when an event occurred.

  if (alarmSounding == true) {

If alarmSounding is true, this statement reads if (true == true). Since true is equal to true, the result of the comparison is true. If, on the other hand, alarmSounding is false, the statement reads if (true == false). Since true is not equal false, the result of the comparison is false. So, you can see that the result of the comparison is always the same as the value in alarmSounding. So, look like a real programmer and ditch the == true part of that statement.

The biggest problem with your code, though, is that you are not testing for when the switch BECOMES pressed, but rather testing when the switch IS pressed. So, as long as the switch IS pressed, the event time (when the switch became pressed) keeps moving forward. It doesn't stop moving until you release the switch.

Look at the state change detection example.

I disagree with @PaulS about global variables - I like them.

I also think that "if (alarmSounding == true)" is likely to be more obvious to a newbie than "if (alarmSounding)" which does exactly the same thing.

I have a lot of sympathy for Paul's comment about the button "becoming" pressed, but I think there is more to it than that.

The question of what to do if the user continues to press the button needs a little thought. My first assumption is that the user will never keep his finger on the button for any appreciable length of time compared with the buzzer duration. It would be simple to ignore button presses until the buzzer completes its buzz, but if the user still has his finger on the button the buzzer will immediately start again with no apparent interval. If you introduce a deliberate interval between buzzes it will make the button appear unresponsive.

Over to you ...

...R

Wow strong responses,

that's Ok by me, they were useful and constructive.

I think at this point I should post my complete code, and give a full explanation of what I'd like to achieve. So here goes:-

The first thing I'd like to say is that I got this code from a search and then modified it to suit my needs. Without the hard work and determination of others, I wouldn't have got this far! Many thanks to Giuseppe Di Cillo (www.coagula.org) for providing the framework, and Alexander Brevig for providing the Library.

Now I need to fill in the gaps. And I thought it would be relatively easy. 8)

Code is too large to fit in post so I posted it here http://pastebin.com/R9Lg1tX5

Initially, I was only trying to get the Millis() delay to work, hence the alarmtest() function (lines 307-330.)

I don't know if it's obvious from the code, but whilst tinkering with it I realised that I ought to simply make a bleep() function, and then it could be multiplied to produce the alarm function(). That is the subject of this thread.

I also thought it would be cool to have an acceleration feature if the button is held down whilst incrementing/decrementing the timers.

I hope that this post has put some things in context.

It's late now, so I will experiment with your suggestions tomorrow!

Thank's for your contribution

Steve

bance:
Perhaps, we could leave that aside for a moment (I’m a little frustrated.) As far as I can see it, the concept of using Millis() as a timed delay, works like this:-

We issue a variable “A” with a timestamp (Millis()), next we issue another variable “B” with a later timestamp (+Millis()), …

Steve-

I know that the BlinkWithoutDelay uses two variables but I believe this really only needs one.

The “later timestamp” is currentMillis in the BlinkWithoutDelay example but I never understood the logic of having a variable called currentMillis because as soon as it’s assigned a value it’s no longer current! (a bit of an oxymoron I think)

Why not simply compare against the output of millis() itself? By definition, you can’t get any more current than that.

Just a thought.

Regards,

Brad
KF7FER

Some applications require a clean snapshot of the time. Most do not.

millis disables interrupts. Repeatedly disabling interrupts can interfere with some applications. For most it is irrelevant.

I like to use currentMillis for a couple of reasons.

It only requires one call to get the value (hopefully reducing program run time).

It allows several tests to be done against the same value.

...R

currentMillis or an equivalent name for me too for the reasons that Robin has outlined. Apart from anything else, using a meaningful name makes the program more readable as in
if (currentMillis - startMillis >= interval)but as in many things it is a matter of personal preference.

The "later timestamp" is currentMillis in the BlinkWithoutDelay example but I never understood the logic of having a variable called currentMillis because as soon as it's assigned a value it's no longer current! (a bit of an oxymoron I think)

It actually stays current for quite a few iterations of loop(), if loop() isn't doing a whole lot (and there is no blocking code). At 16MHz, quite a few instructions are executed in the 1024 microseconds between changes to the value that millis() returns.

Thanks for all the replies,

I think I must have been more tired than I thought last night, since I didn’t give a very good account of what this project is about!

I am making the controller for a UV exposure box, when I first thought about this, an up button, a down button and a start button (to set the time, and start the countdown,) would have sufficed.
But as often happens with projects, feature creep sets in. When I found the ‘menubackend library’ great, now I can introduce all sorts of options (most of which I don’t need and will never use!)
Nevertheless it does provide a real and valuable learning experience.

So back on topic, I’ve tried to take in what everybody has said, and I’m quite pleased, because. I’ve managed to get something that works, almost!

I’m very grateful to all that have helped me along the way. I think the thing that opened my eyes was this comment from PaulS:-

The biggest problem with your code, though, is that you are not testing for when the switch BECOMES pressed, but rather testing when the switch IS pressed. So, as long as the switch IS pressed, the event time (when the switch became pressed) keeps moving forward. It doesn’t stop moving until you release the switch.

The relationship between actual time and code time is the key, I’m not sure if I’ve put that succinctly, but anyway the code seemed much clearer after reading the quote above.

Here is the code:-

//Code:


const int buttonPinEnter = 11;   // pin for the Enter button
const int buzzerPin = 12;        // pin for the buzzer


int buzzerState = LOW;          //variable to hold the state of the buzzer
byte buttonState = HIGH;        //variable to hold the state of the button
byte lastButtonState = 0;        //previous state of the button
boolean alarmSounding = false;

long AlarmDelay = 200;
long currentMillis =0;
long alarmStartMillis = 0;

void setup()
{
  pinMode(buttonPinEnter, INPUT);
  pinMode(buzzerPin, OUTPUT);
  Serial.begin(9600);
}  // setup()...

void loop(){
  currentMillis = millis();
  readButtons();  
  activateAlarm();
  soundAlarm();  
} //loop()... 

void  readButtons(){                    //read buttons status

  //Enter button
  // read the state of the switch into a variable:
  buttonState = digitalRead(buttonPinEnter);
  
}           

void activateAlarm() {
  if (buttonState != lastButtonState) {
    if (buttonState == LOW) {
    alarmStartMillis = millis();
    alarmSounding = true;
    Serial.println("button change start event");
    }
    else {
      buttonState = HIGH;
      alarmSounding = false;
      Serial.println("button change stop event");
    }
  }
  lastButtonState = buttonState;
}


void soundAlarm() {
    
  if (alarmSounding == true) {
    if (currentMillis - alarmStartMillis <= AlarmDelay) {
      buzzerState = HIGH;
      Serial.println("alarm run");
    }
    else {
      buzzerState = LOW;
      alarmSounding = false;
      Serial.println("alarm stop");
    }
  }
    
  digitalWrite(buzzerPin, buzzerState);
}

This works most of the time but sometimes the buzzer “locks on” and it takes a few more presses of the button to resume “normal” operation. I want to say that this because of bouncing in the switch, but I thought that the whole point of edge detect was to lock out changes after the first “key” change.

Anyway comments are most welcome.

Regards Steve.