millis() function with a button press

Question

I am trying to use the millis() function to turn on a pin for a specified interval then turn off and turn on a second pin. It is intended to power a relay and offer a visual cue to when the cycle is over. Several of these need to eventually be running, most likely three, so using delay() won't work. I plan to add additional "button pins" for each input. The LED lights up but gets stuck in the loop and doesn't cycle out. It seemed like a simple program but I can't crack it for some reason.

Is there a much better method to do this? There will be two optical sensors and one pushbutton switch attached to an air driven crushing machine. The two optical sensors will be used only as time indicators with LEDs and the pushbutton will power the relay. Forgive my shortcomings as a programmer, I am learning the programming language still.

const int ledPin =  2;      // the number of the LED pin
const int buttonPin = 3;    // Button pin number
// Variables will change:
long previousMillis = 0;        // will store last time LED was updated
int buttonState = 0;            // Checks if button is pushed
int buttonPush = 0;             // Toggles between 0 to hold state
long interval = 1000;           // interval at which to blink (milliseconds)

void setup() {
// set the digital pin as output:
   pinMode(ledPin, OUTPUT);      
   pinMode(buttonPin, INPUT);
}
void loop()
{
   unsigned long currentMillis = millis();
   buttonState = digitalRead(buttonPin);
   if (buttonState == HIGH) {
      buttonPush = 1;
      if (buttonPush == 1){
         if(currentMillis - previousMillis > interval) {
            previousMillis = millis();   
            digitalWrite(ledPin, HIGH);
         }
         else {
            buttonPush = 0;     
         }  
      }
      else {
         digitalWrite(ledPin, LOW); 
      }
   } 
}

Edit: Thank you for looking over the code, you guys are awesome. There were a couple questions regarding clarity of the final application so I will elaborate. I am going to attempt to clean it up. I had a feeling something was wrong with:

if (buttonState == HIGH) {
   buttonPush = 1;

but I couldn't put my finger on it.

Eventually the system will function with two heating elements to soften plastic, which I will either add an optical sensor to since it will probably get triggered a few hundred times a day, or just a pushbutton to start a timer so the plastic isn't left in too long. A green LED will be illuminated to indicate the system is on. When the plastic object is placed in the heating element it triggers the optical sensor (or the operator pushes a pushbutton to start the timer) that will then illuminate a red LED to indicate heating. When the specified time interval has expired, the LED will revert back to green to show the cycle is complete. The plastic object is then removed and placed in a pneumatic crushing device with a green system on LED. The operator then pushes a pushbutton which turns on a red indicator LED and a relay to power the solenoid on the pneumatic press for a specified amount of time to allow the plastic to cool and crystallise. When that interval has expired, the system will revert back to illuminating the green LED and will end power to the relay allowing the press to release.

The pneumatic press and the heating system works awesome, I just need better control of the time intervals used. The end result needs to be consistent.

I am going to try all suggestions and will post what ended up working if I reach that point or find an answer that functions.

Answer 1

Some code is shown below to illustrate a method for treating three events independently.

This code might not be completely applicable as-is, because the problem description isn't clear about what should happen when a button is pressed and about whether other events to be added to the code are like the one shown there now. However, if you understand how it works you probably can adjust it to meet any special requirements of your application.

During each pass through the loop() routine, the code does the following for each type of event:

If the event is not currently active (as denoted by states[i] < debounced), the code checks whether the event's button has been pressed (or, if its sensor has turned on). If so, the code increments a debounce counter. If the event's counter gets up to debounced, then the output for the event is turned on, and the event's end-time is stored in an array entry.

If the event is currently active (as denoted by states[i] >= debounced), the code tests if the event's end-time has arrived; if so, it turns off the event's output bit and resets its state to 0.

The last statement in loop() busy-waits until milliseconds() returns a different value than before. This is done so that switch-debouncing samples are taken at least a millisecond apart. If your inputs don't need debouncing, you can remove debounce code .

/* Process three asynchronous activities: Upon a given input going
true, activate its associated output(s) for specified lengths of time.
*/

enum { nkinds=3, debounced=5 }; // # of activity kinds;  debounce-criterion
const byte outPins[nkinds] = {2, 4, 6}; // Output pin numbers
const byte inPins[nkinds]  = {3, 5, 7}; // Input pin numbers
const unsigned int actLens[nkinds] = {345, 678, 323}; // Activity lengths, ms
byte states[nkinds] = {0, 0, 0};
unsigned long actEnds[nkinds] = {0, 0, 0};

void setup() {
// Set input/output modes of pins
  for (byte i=0; i<nkinds; ++i) {
    pinMode(outPins[i], OUTPUT);
    pinMode(inPins[i], INPUT);
  }
}

// Once per millisecond, for each kind of activity
// check for new beginning or deal with ongoing action
void loop() {
  unsigned long now = millis();
  for (byte i=0; i<nkinds; ++i) {
    if (states[i] < debounced) {  // Is activity off?
      // It's off, look for a turnon
      if (digitalRead(inPins[i])) {
        ++states[i];        // Button is closed at the moment
        // See if button has been debounced
        if (states[i] >= debounced) { // If so, start its action
          digitalWrite(outPins[i], HIGH);
          actEnds[i] = now + actLens[i];
        }
      }
      else {
        states[i] = 0;      // Button is not closed at moment
      }
    }
    else { // It's on, see if end-time has arrived
      if (now >= actEnds[i]) {
        digitalWrite(outPins[i], LOW); // Clear output when done
        states[i] = 0;        // Return to button-seeking state
      }
    }
  }
  while (now == millis()) {};       // Finish current millisecond
}

The code above compiles ok in the Arduino IDE but I haven't tested it on an Arduino device.

Note, larger systems with numerous events will use priority queues to implement lower-overhead versions of the same ideas.

Answer 2

The direct answer to your first question (LED gets stuck) after formatting and summarizing a section of your code:

  buttonPush = 1;
  if (buttonPush == 1){
     ...
  }
  else {
     digitalWrite(ledPin, LOW); 
  }

is that there's a logic error. You can see that the else clause, turning off the LED, will never be executed because buttonPush is always 1 before you enter this section. I think this is the point @RussellMcMahon was trying to make.

The answer to your second question, needing several of these running at once, is: Get the first one to work first. Then simplify it as much as you can. Then you can choose between using polling loop (repeatedly reading the clock and deciding whether it's time to do something) or setting interrupts (making the clock interrupt your main program to run the routine that does the "something").

Answer 3

The simplest way I can think of is something like this:

unsigned long lastPress = 0;
unsigned long stateOneTime = 2000; //runs until two seconds elapse
unsigned long stateTwoTime = 4000; //runs after state one until 4 seconds from first press
...
if(/*input is active*/) lastPress = millis();
...
if(millis() - lastPress < stateOneTime){
    //set output for first state
    //active for 2 seconds
} else if (millis() - lastPress < stateTwoTime){
    //set output for second state
    //active an additional 2 seconds
} else {
    //set output to the "off" state
}

Many improvments could be made, like going more of a state machine route (separating the time checks from the branches) or paramaterizing the state lengths better (durations would make more sense than total time from start, but be slightly more complicated to code).

I think the key to making your life easier while working on this code will be separating the input segment from the output segment. It will help keep the code easy to read and modular.

Answer 4

Summary:

A fatal problem is caused by the two lines of code below.
A variable is set and then the same variable is immediately tested to see if it is set. This means that the IF test always passes and the ELSE never happens. As the LED is cleared only when the else occurs, the LED stays lit.

Code:

  buttonPush = 1;  
  if (buttonPush == 1){ ...

When the button is pressed you set buttonPush and then immediately test to see if it is set.

As it always is set, the LED never gets turned off as the "else" condition is never met.

Even when that is fixed you have other issues.
I suggest that "expanding" the code along the lines I have shown in (3) below will help you immensely with visualising what your code does.

There are a large number of editors which handle this task in various ways, but just plain tabbing and white space with a basic editor helps muchly.

---

It seems almost certain that your code will NOT do what you intend when it does work "properly", but there are several issues below that need addressing (one fatal, others less so) - see "ISSUES" below.

Apart from those issues, it's not 100% clear how this is intended to be used.
Options include:

• Switch is pressed and released quickly (say 100 mS) - LED lights for one second and then goes out.

• Switch is pressed and held - LED lights for one second then turns off even when switch is held on.

• Other variants are possible.

At present it will not do any of these.

---

ISSUES:

(1) You have assigned

long previousMillis = 0;

This should be

unsigned long previousMillis = 0;

Mixing the two types gives an uncertain result. At best, an error will occur when the value returned by millis() exceeds the length of a "long" data type and when types are mixed there is no guarantee that the result will be valid.

---

(2) I have not checked it, but I'd be wary of the line

if(currentMillis - previousMillis > interval)

as it depend on operator precedence of the "-" being implemented prior to the ">" comparison.
ie this could potentially evaluate as either

" (A-B) > C " .... which is what you intend,
or
" A - (B > C) " .... which, while it is not what is intended, does have a valid if not useful result.

You can rely on operator precedence if you have confidence in yourself and the system or make it explicit as in eg

if( (currentMillis - previousMillis) > interval)

---

Making things easier for yourself:

This has been edited to match my main conclusion which was already covered above (the problem was not nesting but a test that can never fail - as covered above) but the formatting comments are relevant.

Your code is written in a standard (or usual) manner. Some are able to handle such without problem, but I (and many others) find such layout hard to read and, importantly, easy to bury errors in. The following is overdone - I used existing tabs and expanded it in Word :-) - but to me the following makes it immensely more readable. Layout and style are a matter of personal preference, and there are many opinions re what is "best", but something which allows ease of visualisation of program structure is liable to be useful.

The two adjacent lines

  buttonPush = 1;  
  if (buttonPush == 1){ ...

are logically grouped together when indented like this at make it clearer that the "set then test" arrangement can never fail so the subsequent "else" where the LED is turned off is never implemented. So the LED must stay on.

buttonPush = 0; is bolded for unrelated reasons.