Metro interval error?

Hi :slight_smile:

Im doing a project, where a push button toggles between 4 states. In these 4 states a led and a buzzer behaves differently, to indicate what state is active.

I am using Metro to handle this behavior, and it works fine, except some unexpected behavior when I enter state 2 (led blinking, and buzzer sounding, both with specific intervals.

Instead of doing what it is supposed to (blink led, and turn buzzer on/off), first it "lingers" for maybe half a sec where led is always on, and buzzer is constantly buzzing.

It then starts to act in accordance with the defined intervals, and both the LED and buzzer turns on/off as supposed to.

Really hope someone can help me, as I dont get why this is happening, and I feel it must be an error somewhere in the code.

The code for "state 2":

    // State 2: Warning
  while(counter == 2){
    ledDelay.interval(150);
    buzzDelay.interval(200);
    
    Serial.println("Warning");
    Serial.println(counter);

    
    if(ledDelay.check() == 1) {
      ledState = !ledState;
      digitalWrite(ledPin,ledState);
    } 

    if(buzzDelay.check() == 1) { 
      tone(buzzPin, 1244.1, 100);
    }

    reading = digitalRead(buttonPin);
    if (reading != lastButtonState) {
      lastDebounceTime = millis();
    }

    if ((millis() - lastDebounceTime) > debounceDelay) {
      if (reading != buttonState) {
        buttonState = reading;
        if (buttonState == HIGH) {
          counter+=1;
        }
      }
    }
    lastButtonState = reading;
  }

And entire code (not cleaned up yet sry):

#include <Metro.h>

// constants won't change. They're used here to set pin numbers:
const int buttonPin = 4;    // the number of the pushbutton pin
const int ledPin = 2;       // the number of the LED pin
const int buzzPin = 9;         // the number of the Buzzer-pin

// Variables will change:
int ledState;         // the current state of the output pin
int buttonState;             // the current reading from the input pin
int buzzState = LOW;
int lastButtonState = LOW;   // the previous reading from the input pin
int counter, reading, buzzCount;
long interval = 1000;           // interval at which to blink (milliseconds)

// Metro instatiations
Metro ledDelay = Metro(500);
Metro buzzDelay = Metro(200);


// the following variables are unsigned longs because the time, measured in
// milliseconds, will quickly become a bigger number than can be stored in an int.
unsigned long lastDebounceTime = 0;  // the last time the output pin was toggled
unsigned long debounceDelay = 50;    // the debounce time; increase if the output flickers
unsigned long previousMillis = 0;        // will store last time LED was updated


void setup() {
  pinMode(buttonPin, INPUT);
  pinMode(ledPin, OUTPUT);
  pinMode(buzzPin, OUTPUT);

  // set initial LED state
  digitalWrite(ledPin, ledState);
  Serial.begin(9600);
}

void loop() {
  digitalWrite(ledPin, LOW);
  Serial.println("Main");
  Serial.println(counter);
  // read the state of the switch into a local variable:
  reading = digitalRead(buttonPin);

  // check to see if you just pressed the 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 != lastButtonState) {
    // reset the debouncing timer
    lastDebounceTime = millis();
  }

  if ((millis() - lastDebounceTime) > debounceDelay) {
    // whatever the reading is at, it's been there for longer than the debounce
    // delay, so take it as the actual current state:

    // if the button state has changed:
    if (reading != buttonState) {
      buttonState = reading;

      // only toggle the LED if the new button state is HIGH
      if (buttonState == HIGH) {
        counter+=1;
      }
    }
  }
  lastButtonState = reading;

  // State 1: Ok
  while(counter == 1){
    while (buzzCount < 2){
      buzzCount++;
      tone(buzzPin, 587.33); // Send 1KHz sound signal...
      delay(100);        // ...for 1 sec
      noTone(buzzPin);     // Stop sound...
      delay(50);        // ...for 1sec 
    }
    
    Serial.println("First");
    Serial.println(counter);
    digitalWrite(ledPin, HIGH); 
    
    reading = digitalRead(buttonPin);
    if (reading != lastButtonState) {
      lastDebounceTime = millis();
    }

    if ((millis() - lastDebounceTime) > debounceDelay) {
      if (reading != buttonState) {
        buttonState = reading;
        if (buttonState == HIGH) {
          counter+=1;
        }
      }
    }
    lastButtonState = reading;
  }


    // State 2: Warning
  while(counter == 2){
    ledDelay.interval(150);
    buzzDelay.interval(200);
    
    Serial.println("Warning");
    Serial.println(counter);

    
    if(ledDelay.check() == 1) {
      ledState = !ledState;
      digitalWrite(ledPin,ledState);
    } 

    if(buzzDelay.check() == 1) { 
      tone(buzzPin, 1244.1, 100);
    }

    reading = digitalRead(buttonPin);
    if (reading != lastButtonState) {
      lastDebounceTime = millis();
    }

    if ((millis() - lastDebounceTime) > debounceDelay) {
      if (reading != buttonState) {
        buttonState = reading;
        if (buttonState == HIGH) {
          counter+=1;
        }
      }
    }
    lastButtonState = reading;
  }


   // State 3: Error
  while(counter == 3){
    ledDelay.interval(1000);
    buzzDelay.interval(1000);
    Serial.println("Error");
    Serial.println(counter);
    
    if(ledDelay.check() == 1) {
      ledState = !ledState;
      digitalWrite(ledPin,ledState);
    } 
    if(buzzDelay.check() == 1) { 
      tone(buzzPin, 200, 100);
    }

    reading = digitalRead(buttonPin);
    if (reading != lastButtonState) {
      lastDebounceTime = millis();
    }

    if ((millis() - lastDebounceTime) > debounceDelay) {
      if (reading != buttonState) {
        buttonState = reading;
        if (buttonState == HIGH) {
          counter+=1;
        }
      }
    }
    lastButtonState = reading;
  }

   

   // State 4: Busy
  while(counter == 4){
    ledDelay.interval(40);
    Serial.println("Busy");
    Serial.println(counter);

    if(ledDelay.check() == 1) {
      ledState = !ledState;
      digitalWrite(ledPin,ledState);
    } 
   
    reading = digitalRead(buttonPin);
    if (reading != lastButtonState) {
      lastDebounceTime = millis();
    }

    if ((millis() - lastDebounceTime) > debounceDelay) {
      if (reading != buttonState) {
        buttonState = reading;
        if (buttonState == HIGH) {
          counter+=1;
        }
      }
    }
    lastButtonState = reading;
  }

  if (counter == 5){
    counter = 0;
    buzzCount = 0;
  }
}
unsigned long previousMillis = 0;        // will store last time LED was updated

Wouldn't a name like lastUpdateTime or lastLEDUpdateTime make more sense?

Do you see people naming variables that hold the value returned by digitalRead() like so:

   byte valueDigitalRead = digitalRead(somePin);

Of course not, because there is nothing magical in the name DigitalRead. There is NOTHING magical in the name Millis, either, so stop using it in variable names unless the name actually adds value.

There is NO need to use three variables, reading, buttonState, and lastButtonState, to determine whether the current state is, or is not, the same as the previous state.

There is NO reason to use a while loop in loop(), and read the state of the switch again in the while loop. As incredible as it might sound, loop() loops. Let it handle all the looping.

    while (buzzCount < 2){
      buzzCount++;
      tone(buzzPin, 587.33); // Send 1KHz sound signal...
      delay(100);        // ...for 1 sec
      noTone(buzzPin);     // Stop sound...
      delay(50);        // ...for 1sec
    }

What have you got against for loops?

I gave up on reading your code. The state change detection example presents a clean way of counting switch presses. You appear to have butcher the example, and made it unnecessarily more complicated that it needs to be.

I suggest that you start over.

PaulS:
There is NOTHING magical in the name Millis, either, so stop using it in variable names unless the name actually adds value.

Any variable I use that holds a time gotten from millis() always ends with Millis so I am aware of the units. Some of my projects deal with seconds, most deal with milliseconds.
so I would use 'lastLEDUpdateMillis' since I think it does add value.

Your mileage may differ...

PaulS:
I suggest that you start over.

Thank you for you advice. Did so, used if statements instead of the while-loops, and it seems to work a lot better now!

Don't know if nobody ever told you how to give constructive critical advice, but you seem to be wasting A LOT of time writing in the style, that you choose. Whatever floats your byte tho...