Single button dual function [SOLVED]

i am trying to understand the use of millis() and avoid delay(), my simple code is as below

const int btn = 3;
const int led = 4;
int longPress = 600;
int shortPress = 100;
unsigned long currentMilli;
unsigned long previousMilli;
int pressTime;
int btnState;

void setup() {
  pinMode(btn, INPUT_PULLUP);
  pinMode(led, OUTPUT);
  Serial.begin(9600);
}

void loop()
{
  btnState = digitalRead(btn);
  if ( btnState == LOW )
  {
    currentMilli = millis();
    //previousMilli = currentMilli;
    delay(100);
    Serial.print("Press Time: ");
    Serial.println(currentMilli);
    if (millis() - currentMilli < 100)
    { 
      digitalWrite(led, HIGH);
    }
    else if (currentMilli - previousMilli  > 600)
    {
      digitalWrite(led, LOW);
    }
  }
}

from the above i can see in the Serial monitor that the millis() change as i press the button but for some reason the function inside the ‘if statement’ does not seem to be executing, any idea why ?

You have to store the time (mS) when the button is first pressed.
Only when the button is released, can you then determine how long it was pressed for, otherwise you cannot distinguish between short and long presses.
Do not use a delay() statement.

There are two if statements but assuming you’re talking about this one:
if (millis() - currentMilli < 100)

millis() - currentMilli will never be less than 100. Try plugging some numbers in and you will see how/why.

millis() - currentMilli will never be less than 100

yea my bad, wonder what i was thinking

fixed that. now on press the led lights up, how can i determine the time after it was pressed ?

6v6gt:
You have to store the time (mS) when the button is first pressed.
Only when the button is released, can you then determine how long it was pressed for, otherwise you cannot distinguish between short and long presses.
Do not use a delay() statement.

don't know how the press time is calculated, that is how long the pin has gone HIGH?

const int btn = 3;
const int led = 4;
int longPress = 600;
int shortPress = 100;
unsigned long currentMilli;
unsigned long previousMilli;
unsigned long holdTime;
int btnState;

void setup() {
  pinMode(btn, INPUT_PULLUP);
  pinMode(led, OUTPUT);
  Serial.begin(9600);
}

void loop()
{
  btnState = digitalRead(btn);
  if ( btnState == LOW )
  {
    currentMilli = millis();
    previousMilli = currentMilli;
    delay(100);
    Serial.print("Press Time: ");
    Serial.println(currentMilli);
    if (millis() - currentMilli > 10) {
      digitalWrite(led, HIGH);
    }
    holdTime = currentMilli;
    if (btnState == HIGH) {
      delay(100);
      if (previousMilli - holdTime > 10) {
        Serial.print("Hold Time: ");
        Serial.println(holdTime);
        digitalWrite(led, LOW);
      }
    }
  }

don't know how the press time is calculated, that is how long the pin has gone HIGH?

I've made a few changes to your code (untested) to demonstrate a method of achieving this. In principle, it detects changes in the button state, and reacts accordingly.

const int btn = 3;
const int led = 4;
int longPress = 600;
int shortPress = 100;
unsigned long btnPressedAtMilli;
unsigned long holdTime;
int btnState;
int btnStatePrevious = HIGH ;

void setup() {
  pinMode(btn, INPUT_PULLUP);
  pinMode(led, OUTPUT);
  Serial.begin(9600);
}

void loop()
{
  btnState = digitalRead(btn);
  if ( btnState != btnStatePrevious  )
  {
    // change of state detected
    if ( btnState == LOW ) {
       // button just pressed so save time
       btnPressedAtMilli = millis() ;
       Serial.print("Press Time: ");
       Serial.println(btnPressedAtMilli );
    else {
       // button just released
       holdTime = millis() - btnPressedAtMilli ;
       if ( holdTime > longPress ) {
          // handle long button press
          Serial.println("Long Press");
       }
       else if (holdTime > shortPress ) {
          // handle short button press
          Serial.println("Short Press");
       }
     btnStatePrevious = btnState ;
  }
}

saximus:
There are two if statements but assuming you’re talking about this one:
if (millis() - currentMilli < 100)

millis() - currentMilli will never be less than 100. Try plugging some numbers in and you will see how/why.

And if (millis() - currentMilli < 100) will always be less than 100 :smiley: And with the code in reply #4 it will never be greater than 10 :smiley:

@anishkgt
You always set currentMilli to millis() if the button is pressed. The chance that millis() changes between that and the compare do exist (one in a thousand or so :wink: ) but there will never be anything more than a calculated difference of 1 ms.

You need to take a big part of the code out of the first if.

void loop()
{
  static unsigned long startTime = 0;

  btnState = digitalRead(btn);
  if ( btnState == LOW )
  {
    // do some things if needed
    ...
    ...

    // set the start time that the button was pressed
    // this is only done on the first change from HIGH to LOW (indicated by a startTime of 0)
    if(startTime == 0)
    {
      startTime = millis();
    }
  }

  ...
  ...

So now we have a proper startTime when the button is pressed (HIGH to LOW transition). Next we can add the code for the LOW to HIGH transition (button released). It’s in the else block below.

void loop()
{
  static unsigned long startTime = 0;

  btnState = digitalRead(btn);
  if ( btnState == LOW )
  {
    // do some things if needed
    ...
    ...

    // set the start time that the button was pressed
    // this is only done on the first change from HIGH to LOW (indicated by a startTime of 0)
    if(startTime == 0)
    {
      startTime = millis();
    }
  }
  else
  {
    // get the time that the button was released (might be bouncing as well)
    unsigned long endTime = millis();

    // if startTime is not 0, it's a LOW to HIGH transition
    if(startTime != 0)
    {
      // if long press (more than 1 second);
      if(endTime - startTime >= 1000)
      {
        longPressFunction();
        // reset the start time for use with the next button press
        startTime = 0;
      }
      // else if longer than 200ms it's a short press
      else if(endTime - startTime >=200)
      {
        shortPressFunction();
        // reset the start time for use with the next button press
        startTime = 0;
      }
      else
      {
        // it was some bouncing when the button was pressed; ignore
      }
    }
  }
}

Note that the sequence to detect longpress / shortpress is important.

This was written in the hope it helps you with the approach, not as a final solution. There probably will be bouncing as well when you release the button and the code does not cater for that. The result can be a false detection of the button being pressed again. Read up how you can debounce.

An extremely simple example to debounce using delay (applicable to button with pull-up resistor as you use); it might suite your needs.

int readButton(int pin)
{
  if(digitalRead(pin) == LOW)
  {
    // wait a little if the pin is LOW; finetune to your needs
    delay(50);
    // read again and return the new value
    return digitalRead(pin);
  }
  else
  {
    return HIGH;
  }
}

There are better approaches (without delay) and you need to read up on them; see e.g. the debounce example that comes with the IDE or look at some libraries.

And in loop, you can use

void loop()
{
  btnState = readButton(btn);
  ...
  ...
}

None of the code is compiled or tested; as said, just to give you the idea.

Seems like a smple sketch but a lot to understand.

I came across this sketch while learning about the dual function

int LED1 = 12;
int LED2 = 13;
int button = 3;

boolean LED1State = false;
boolean LED2State = false;

long buttonTimer = 0;
long longPressTime = 250;

boolean buttonActive = false;
boolean longPressActive = false;

void setup() {

        pinMode(LED1, OUTPUT);
        pinMode(LED2, OUTPUT);
        pinMode(button, INPUT);

}

void loop() {

        if (digitalRead(button) == HIGH) {

               if (buttonActive == false) {

                       buttonActive = true;
                       buttonTimer = millis();

               }

               if ((millis() - buttonTimer > longPressTime) && (longPressActive == false)) {

                       longPressActive = true;
                       LED1State = !LED1State;
                       digitalWrite(LED1, LED1State);

               }

        } else {

               if (buttonActive == true) {

                       if (longPressActive == true) {

                               longPressActive = false;

                       } else {

                               LED2State = !LED2State;
                               digitalWrite(LED2, LED2State);

                       }

                       buttonActive = false;

               }

        }

}

how does digitalWrite(LED1, LED1State); change the led to HIGH or LOW. Normally its written as digitalWrite(LED1, HIGH or LOW)

. . . because false and LOW and 0 are the same value. Equally, true and HIGH and 1 have the same value.

Thanks !

making sense out of the sketch now.

Figured it out with this sample sketch.

Below is my code, but for some reason, the weld happens upon first boot itself. When idle it works as normal, that is short press and long press.

void setup()
{
  pinMode(btn, INPUT_PULLUP);
  digitalWrite(btn, HIGH);
  attachInterrupt(0, setFlag, FALLING);//zero cross attach
  delay(500);
  LedRun();
  //Serial.begin(9600);
}
void loop()
{ btnState = digitalRead(btn);
  while (!zeroCrossingFlag)
  {
    digitalWrite(rdy, LOW);
  }
  digitalWrite(rdy, HIGH);
  // For debug
  if (OldWeldStepValue != WeldStep())
  {
    WeldStep();
    OldWeldStepValue = WeldStep();
  }
  PulseSetLed();

  // Test for button pressed and store the down time
  if (btnState == LOW && buttonLast == HIGH && (millis() - btnUpTime) > long(debounce))
  {
    btnDnTime = millis();
  }

  // Test for button release and store the up time
  if (btnState == HIGH && buttonLast == LOW && (millis() - btnDnTime) > long(debounce))
  {
    
    if (ignoreUp == false) Weld(100, 500);
    else ignoreUp = false;
    btnUpTime = millis();
  }

  // Test for button held down for longer than the hold time
  if (btnState == LOW && (millis() - btnDnTime) > long(holdTime))
  {
    continousWeld();
    ignoreUp = true;
    btnDnTime = millis();
  }
  buttonLast = btnState;
}

Isn't this part suppose to control what runs at first.

void setup()
{
  pinMode(btn, INPUT_PULLUP);
  digitalWrite(btn, HIGH);
  attachInterrupt(0, setFlag, FALLING);//zero cross attach
  delay(500);
  LedRun();
  //Serial.begin(9600);
}

in my schematic there is not resistor connected in series with the button. I tried deleting the line and that did not help either.Where would be the problem

Where would be the problem

Possibly in the code you didn't post. What part of POST ALL OF YOUR CODE is too hard for you to understand?

I like names that look like they should reasonably be expected to be related, like currSwitchState and prevSwitchState. I do not like random names like btnState and buttonLast. They do not look related at all.

I like for people to state how the switches are actually connected to the Arduino.

void loop()
{ btnState = digitalRead(btn);

I like for people to not put ANYTHING after the {.

int buttonLast = 0; // buffered value of the button's previous state

I like for people to use HIGH and LOW when referring to switch states.

  if (btnState == LOW && buttonLast == HIGH && (millis() - btnUpTime) > long(debounce))

I like for people to use appropriate types, and to recognize when casts are necessary, and when they just add clutter. I like when people do proper casting, and not use those stupid macros.

  while (counter < 6) {
    delay (1000);
    Weld(100, 500);
    counter++;
  }
  counter = 0;

I like when people use for statements, instead of while statements when the number of iterations is fixed. I like when people initialize variables before they expect them to have certain values.

I think you should look at the state change detection example. There are things you want to do when the switch changes state, regardless of which change occurred. So, create a block that just determines if a state change occurred.

There are things that should be done if the state change happened long enough after the previous state change, so put another if statement inside the state change body.

There are things that should be done if the state changed to pressed, and things that should be done if the state changed to released. So, put another if statement and an else statement in the if long enough between events block.

That if and else statement should call a function to actually perform the work.

There are things that should happen even if the switch has not changed state, so do those after the if state changed statement’s body.

This part checks if the key is pressed

Actually, it doesn’t. It checks that the switch HAS BECOME pressed (not IS pressed).

So it was actually the default value of buttonLast int buttonLast = LOW

Changed the value to HIGH and that solved it.