Turning off a LED

Question

I am trying to make 2 alarms for 2 different times using 2 LED's which should turn on when the time corresponds to the set time and then if a pushbutton is pressed, the LED should turn off. For the first alarm, the first LED can be turn off with the pushbutton, instead for the second alarm, the second LED is turning on, but if the pushbutton is pressed is will not turn off, even though is the same code. What can I do to turn off the second LED?

#include <RTClib.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x27, 16, 2); 
int nowHr, nowMin, nowSec;
RTC_DS3231 rtc;


int h1 = 15;
int m1 = 51;
int h2 = 15;
int m2 = 52 ;
int pin_buton1 = 8;
int stop_buton =9;
int state_stop_buton = 0;

int led1_pin = 6;
int state_led = LOW;
int pushpressed = 0;

int led2_pin = 7;
int state_led2 = LOW;


void setup()
{

  Wire.begin();
  //rtc.adjust(DateTime(F(__DATE__), F(__TIME__)));           
  rtc.adjust(DateTime(2021, 03, 27, 15, 50, 50));                
 

  lcd.backlight();
  lcd.begin(16, 2);
  lcd.clear();
  lcd.setCursor(0, 0);
  lcd.print("Welcome To Our");                                     
  lcd.setCursor(0, 1);
  lcd.print("device");



  pinMode(pin_buton1, INPUT);
  pinMode(stop_buton, INPUT);
  pinMode(led1_pin, OUTPUT);
  pinMode(led2_pin, OUTPUT);
  delay(2000);
  Serial.begin(9600);
}




void loop() {
  timeScreen();
  first_alarm();
  stopPins();
  second_alarm();
  stopPins2();

  state_stop_buton = digitalRead(stop_buton);

}



void timeScreen() {              

  DateTime now = rtc.now();             
  lcd.clear();
  lcd.setCursor(0, 0);
  lcd.print("Time:");
  lcd.setCursor(6, 0);
  lcd.print(nowHr = now.hour(), DEC);
  lcd.print(":");
  lcd.print(nowMin = now.minute(), DEC);
  lcd.print(":");
  lcd.print(nowSec = now.second(), DEC);
  lcd.setCursor(0, 1);
  lcd.print("Date: ");
  lcd.print(now.day(), DEC);
  lcd.print("/");
  lcd.print(now.month(), DEC);
  lcd.print("/");
  lcd.print(now.year(), DEC);
  delay(500);

}



void stopPins() {                   
  if (state_stop_buton == 1) {

    pushpressed =1;
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("first message ");
    lcd.setCursor(0, 1);
    lcd.print("was read");
    delay(1200);
    lcd.clear();
  }
}

void startBuzz() {                    

  
  if (pushpressed == 0) {
   
    if (state_led == LOW) {                 
      state_led = HIGH;
      digitalWrite(led1_pin, state_led);
     
    }
  }
  else if (pushpressed == 1) {

   
    state_led = LOW;
    digitalWrite(led1_pin, state_led);
  }
}



void first_alarm() {
  DateTime t = rtc.now();
  if (int(t.hour()) == h1 && int(t.minute()) == m1) {

    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("first ");
    lcd.setCursor(0, 1);
    lcd.print("text");
    startBuzz();
    delay(5000);

  }
}



//

void stopPins2() {                   
  if (state_stop_buton == 1) {

    pushpressed = 1;
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("second message ");
    lcd.setCursor(0, 1);
    lcd.print("was read ");
    delay(1200);
    lcd.clear();
  }
}

void startBuzz2() {                   

  
  if (pushpressed == 1) {
   
 
    if (state_led2 == LOW) {                 
      state_led2 = HIGH;
      digitalWrite(led2_pin, state_led2);
  
    }
  }
  else  if (pushpressed == 0 ) {
    state_led2 = LOW;
   digitalWrite(led2_pin, state_led2);
 }
}



void second_alarm() {
  DateTime t = rtc.now();
  if (int(t.hour()) == h2 && int(t.minute()) == m2   ) {

    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print("second");
    lcd.setCursor(0, 1);
    lcd.print("text");
    startBuzz2();
    delay(5000);

  }
}

Answer 1

1. pushpressed gets set, but never reset.

It's set in stopPins() and stopPins2() if the input is HIGH. Since it's never reset back to zero, once it's been set, the logic in startBuzz and startBuzz2 will follow a single pass. You've created a latched switch.

---

General Stuff

---

2. Inconsistent naming: makes reading code harder

Pick snake_case or camelCase or smashitalltogether (not recommended) and stick with it :) Same goes for the "types" of variables: some things are named xxx pin xxx, some are named xxx state xxx, some aren't. It's clearer when same-things are named-same.

3. Duplication: makes understanding code harder

Along with the naming, the (essentially) duplicated code makes tracking down problems harder, as you've noticed trying to debug this problem. There are a ton of ways to go about shrinking the code down to make it understandable. The nutshells are to identify the differences in similar code, and to do less at once.

(And if you want to take it to its logical conclusion, wrapping up each alarm in a class would eliminate all duplication and most function parameters :)

---

Annoying Diversion of Explanation

---

Let's look at startBuzz and startBuzz2.

void startBuzz() {
  if (pushpressed == 0) {
    if (state_led == LOW) {
      state_led = HIGH;
      digitalWrite(led1_pin, state_led);
    }
  } else if (pushpressed == 1) {
    state_led = LOW;
    digitalWrite(led1_pin, state_led);
  }
}

void startBuzz2() {
  if (pushpressed == 1) {
    if (state_led2 == LOW) {
      state_led2 = HIGH;
      digitalWrite(led2_pin, state_led2);
    }
  } else if (pushpressed == 0) {
    state_led2 = LOW;
    digitalWrite(led2_pin, state_led2);
  }
}

The only things different here are the LED pin, and the LED state. Let's normalize our naming so things become clear, using startBuzz as the example. We'll also assume pushPressed can only be 0 or 1 (HIGH or LOW):

void startBuzz() {
  if (pushPressed == LOW) {
    if (led1State == LOW) {
      led1State = HIGH;
      digitalWrite(led1Pin, led1State);
    }
  } else {
    led1State = LOW;
    digitalWrite(led1Pin, led1State);
  }
}

We can make this work for either LED by doing two things: taking in the pin and current state, and returning the new state. Why return the new state instead of setting it here? It makes the mainline code easier (and avoids passing a reference).

void startBuzz(int ledPin, int ledState) {
  int newLedState = ledState;
  
  if (pushPressed == LOW) {
    if (ledState == LOW) {
      newLedState = HIGH;
      digitalWrite(ledPin, newLedState);
    }
  } else {
    newLedState = LOW;
    digitalWrite(ledPin, newLedState);
  }
  
  return newLedState;
}

Then when we call startBuzz or startBuzz2 we replace the current code with:

led1State = startBuzz(led1Pin, led1State);

We're not done.

Since we keep the original ledState we can remove the duplicated calls to digitalWrite; at worst we'll have a superfluous write when pushPressed != LOW && ledState != HIGH.

Since all conditions except one lead to a LOW value we don't even need to keep the original state. And since it's two conditions to flip it HIGH we can combine those. And if we pull out the digitalWrite we're really just calculating the next state--that doesn't rely on the LED pin.

void getNextLedState(int ledState) {
  return (pushPressed == LOW && ledState == LOW) ? HIGH : LOW;
}

And the callsites look like this:

led1State = getNextLedState(led1State);
digitalWrite(led1Pin, led1State);

Similarly for the alarms: create a function called checkAlarm that takes in hrs and mins and returns a boolean. In the loop call checkAlarm(h1, m1) and if true, call runAlarm1(). (Even that could be shrunk further, but you get the idea.) stopPins differs only in the strings written to the LCD. And so on.