Led not doing what I want

linus_kjk2003 · June 23, 2024, 2:39pm

I build an Alarm Clock with my arduino with a rotary encoder to set the timer and a button to change between Sec, Min, H and days and to stop the alarm but when I turn the rotary encoder the led starts blinking but it should start only when the timer is over.please help me with this problem THANKS

Code:

#include <RTClib.h>
#include <LiquidCrystal_I2C.h>
#include <BasicEncoder.h>
#include <Wire.h>
#include <TimeLib.h>
#include <DS1307RTC.h>

const int8_t pinA = 3; // Rotary encoder CLK
const int8_t pinB = 4; // Rotary encoder DT
const int buttonPin = 6; // Button Pin
int buttonState = HIGH; // Initial button state (HIGH due to INPUT_PULLUP)
unsigned long previousMillis = 0;
const long interval = 500;
BasicEncoder encoder(pinA, pinB);
RTC_DS3231 rtc;
LiquidCrystal_I2C lcd(0x26, 16, 2);
long oldPosition = -999;

#define CLOCK_INTERRUPT_PIN 2
int Dalarm = 0;
int Halarm = 0;
int Malarm = 0;
int Salarm = 0;
bool isTimer = false;
bool isAlarm = false;
const int AlarmPin = 12;
int indicator = 0;
const int RTswitch = 5; // Rotary encoder switch
bool isAlarmTimeRunning = false;

void pciSetup(byte pin)  // Setup pin change interrupt on pin
{
  *digitalPinToPCMSK(pin) |= bit(digitalPinToPCMSKbit(pin));  // enable pin
  PCIFR |= bit(digitalPinToPCICRbit(pin));                    // clear outstanding interrupt
  PCICR |= bit(digitalPinToPCICRbit(pin));                    // enable interrupt for group
}

void setup_encoders(int a, int b) {
  uint8_t old_sreg = SREG;     // save the current interrupt enable flag
  noInterrupts();
  pciSetup(a);
  pciSetup(b);
  encoder.reset();
  SREG = old_sreg;    // restore the previous interrupt enable flag state
}

ISR(PCINT2_vect)  // pin change interrupt for D0 to D7
{
  encoder.service();
}

void setup() {
  Serial.begin(115200);
  pinMode(AlarmPin, OUTPUT); // LED
  pinMode(buttonPin, INPUT);
  pinMode(RTswitch, INPUT_PULLUP);
  setup_encoders(pinA, pinB);
  encoder.set_reverse();
  lcd.init();
  lcd.backlight();
  lcd.clear();

  if (!rtc.begin()) {
    Serial.println("Couldn't find RTC!");
    Serial.flush();
    abort();
  }

  if (rtc.lostPower()) {
    Serial.println("RTC lost power, set the time!");
  }

  rtc.disable32K();

  // Making it so, that the alarm will trigger an interrupt
  pinMode(CLOCK_INTERRUPT_PIN, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(CLOCK_INTERRUPT_PIN), onAlarm, FALLING);

  // set alarm 1, 2 flag to false (so alarm 1, 2 didn't happen so far)
  // if not done, this easily leads to problems, as both register aren't reset on reboot/recompile
  rtc.clearAlarm(1);
  rtc.clearAlarm(2);

  // stop oscillating signals at SQW Pin
  // otherwise setAlarm1 will fail
  rtc.writeSqwPinMode(DS3231_OFF);

  // turn off alarm 2 (in case it isn't off already)
  // again, this isn't done at reboot, so a previously set alarm could easily go overlooked
  rtc.disableAlarm(2);
}

void loop() {
  DateTime now = rtc.now();
  DateTime future(now + TimeSpan(0, Halarm, Malarm, Salarm));  //total seconds or // days,hrs,mins,secs
  char timeNow[10] = "hh:mm:ss";
  rtc.now().toString(timeNow);

  int encoder_change = encoder.get_change();
  if (encoder_change) {
    Serial.println(encoder.get_count());
  }

  buttonState = digitalRead(buttonPin);
  unsigned long currentMillis = millis();

  // Entprellung des Buttons
  static unsigned long lastButtonPress = 0;
  if (buttonState == LOW && (currentMillis - lastButtonPress > 50)) {
    lastButtonPress = currentMillis;
    if (indicator < 3 && !isAlarmTimeRunning) {
      indicator = indicator + 1;
    } else {
      if (!isAlarmTimeRunning) {
        indicator = 0;
      }
    }

    if (isAlarm) {
      isAlarm = false;
      isAlarmTimeRunning = false;
    }
  }

  if (isAlarm) {
    lcd.setCursor(0, 0);
    lcd.clear();

    Serial.print("timeNow:");
    Serial.println(timeNow);
    lcd.print(timeNow);
    if (currentMillis - previousMillis >= interval) {
      previousMillis = currentMillis;

      if (digitalRead(AlarmPin) == LOW  && isAlarm) {
        digitalWrite(AlarmPin, HIGH);
      } else {
        digitalWrite(AlarmPin, LOW);
      }
    }
  } else {
    digitalWrite(AlarmPin, LOW);
  }

  if (digitalRead(RTswitch) == LOW) {
    isTimer = true;
    Serial.println("Start Timer");
  }

  if (Salarm > 60) {
    Salarm = Salarm - 60;
  }
  if (Malarm > 60) {
    Malarm = Malarm - 60;
  }
  if (Halarm > 24) {
    Halarm = Halarm - 24;
  }

  if (isTimer) {
    isAlarmTimeRunning = true;
    isTimer = false;
    Serial.println("...");
    if (!rtc.setAlarm1(future, DS3231_A1_Hour)) {
      Serial.println("Error, alarm wasn't set!");
    } else {
      Serial.print("Alarm will occur at: ");
    }

    Serial.print(future.year(), DEC);
    Serial.print('/');
    Serial.print(future.month(), DEC);
    Serial.print('/');
    Serial.print(future.day(), DEC);
    Serial.print(' ');
    Serial.print(future.hour(), DEC);
    Serial.print(':');
    Serial.print(future.minute(), DEC);
    Serial.print(':');
    Serial.print(future.second(), DEC);
    Serial.println();
  }

  if (indicator == 0) {
    Salarm = encoder.get_count();
  }
  if (indicator == 1) {
    Malarm = encoder.get_count();
  }
  if (indicator == 2) {
    Halarm = encoder.get_count();
  }
  if (indicator == 3) {
    Dalarm = encoder.get_count();
  }

  if (!isAlarm && !isAlarmTimeRunning) {
    lcd.setCursor(0, 0);
    lcd.clear();
    lcd.print(timeNow);
    lcd.setCursor(0, 1);
    lcd.print(Dalarm);
    lcd.print(":");
    lcd.print(Halarm);
    lcd.print(":");
    lcd.print(Malarm);
    lcd.print(":");
    lcd.print(Salarm);
  }

  if (rtc.alarmFired(1)) {
    rtc.clearAlarm(1);
    Serial.println("Alarm cleared");
    Serial.print("Temperature: ");
    Serial.print(rtc.getTemperature());
    Serial.println(" C");
  }

  if (isAlarmTimeRunning) {
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print(timeNow);
    lcd.print(" ");
    lcd.print("x");
    lcd.setCursor(0, 1);
    lcd.print(Dalarm);
    lcd.print(":");
    lcd.print(Halarm);
    lcd.print(":");
    lcd.print(Malarm);
    lcd.print(":");
    lcd.print(Salarm);
  } else {
    lcd.clear();
    lcd.setCursor(0, 0);
    lcd.print(timeNow);
    lcd.print(" ");
    lcd.print("o");
    if (indicator == 0) {
      lcd.print(" S");
    }
    if (indicator == 1) {
      lcd.print(" M");
    }
    if (indicator == 2) {
      lcd.print(" H");
    }
    if (indicator == 3) {
      lcd.print(" D");
    }
    lcd.setCursor(0, 1);
    lcd.print(Dalarm);
    lcd.print(":");
    lcd.print(Halarm);
    lcd.print(":");
    lcd.print(Malarm);
    lcd.print(":");
    lcd.print(Salarm);
  }

  delay(1000);
}

void onAlarm() {
  isAlarm = true;
  Serial.println("Alarm occurred!");
}

kolaha · June 23, 2024, 3:36pm

which led?

kolaha · June 23, 2024, 3:40pm

not too much?

hallowed31 · June 23, 2024, 3:47pm

Is this the led part? Is indicator = 0; a state variable for an led?
Did you forget to add the led to the code, there's nothing in the setup() about an led that I can see. If indicator (led?) is less than 3 and if alarm time is not running, increment the led by one?
But if not, that is if the alarm time is not running in any other indicator value, then the indicator is zero?

But all that is also conditional on whatever millis() is right now - 0 being greater than 50 (which it will always be)?
That doesn't make sense to me.

I would start by moving that variable declaration

outside of void loop()

kolaha · June 23, 2024, 3:55pm

linus_kjk2003:

  if (isAlarm) {
    lcd.setCursor(0, 0);
    lcd.clear();

    Serial.print("timeNow:");
    Serial.println(timeNow);
    lcd.print(timeNow);
    if (currentMillis - previousMillis >= interval) {
      previousMillis = currentMillis;

      if (digitalRead(AlarmPin) == LOW  && isAlarm) { // <<---- 
        digitalWrite(AlarmPin, HIGH);
      } else {
        digitalWrite(AlarmPin, LOW);
      }
    }
  } else {
    digitalWrite(AlarmPin, LOW);
  }

why you check isAlarm second time?
and this part oscillating when isAlarm is true:

      if (digitalRead(AlarmPin) == LOW  && isAlarm) { 
        digitalWrite(AlarmPin, HIGH);
      } else {
        digitalWrite(AlarmPin, LOW);
      }

TomGeorge · June 23, 2024, 4:09pm

Hi, @linus_kjk2003
Welcome to the forum.

What model Arduino are you using?

Can you please post a copy of your circuit, a picture of a hand drawn circuit in jpg, png?
Hand drawn and photographed is perfectly acceptable.
Please include ALL hardware, power supplies, component names and pin labels.

Thanks.. Tom..

linus_kjk2003 · June 23, 2024, 4:18pm

led on pin 12 for the alert

linus_kjk2003 · June 23, 2024, 4:19pm

wdym with not too much

linus_kjk2003 · June 23, 2024, 4:21pm

no the led is the AlarmPin in setup() this is that the lcd shows seconds, minutes... to make it easier setting up the alarm

hallowed31 · June 23, 2024, 4:44pm

Ah ok, first thing:

if (digitalRead(AlarmPin) == LOW

Typically we don't read output devices like leds and act on that as a condition whether we should toggle that output.

This whole block:

  if (isAlarm) {
    lcd.setCursor(0, 0);
    lcd.clear();

    Serial.print("timeNow:");
    Serial.println(timeNow);
    lcd.print(timeNow);
    if (currentMillis - previousMillis >= interval) {
      previousMillis = currentMillis;

      if (digitalRead(AlarmPin) == LOW  && isAlarm) {
        digitalWrite(AlarmPin, HIGH);
      } else {
        digitalWrite(AlarmPin, LOW);
      }
    }
  } else {
    digitalWrite(AlarmPin, LOW);
  }

Line by line, can you explain what you think is supposed to be happening, please?
I ask because this occurs twice:

if (isAlarm) {

but again here

if (digitalRead(AlarmPin) == LOW  && isAlarm) {

which has to be true in any case since that's the condition that brought you there in the first place.

Also, again,

static unsigned long lastButtonPress = 0;

should be declared outside of void loop() since that's your main function. You might get away with that in some external function that you call from void loop() (or another function) but every time the main loop cycles, you keep resetting that variable to zero when it is supposed to be keeping track of whatever the free running millis() clock was the last time you checked it.

Therefore these lastButtonPress = :

  static unsigned long lastButtonPress = 0;
  if (buttonState == LOW && (currentMillis - lastButtonPress > 50)) {
    lastButtonPress = currentMillis;

are constantly fighting each other. Why did you declare it as static, anyway?

Similarly, you might as well just declare this

unsigned long currentMillis = millis();

at the top of void loop() since the first time you declare it it keeps running until it overflows (in 49 days or something) anyway. You use the other variables related to it just to keep track of the number of milliseconds the clock was at at some particular time by passing the value then into that new variable since you can't start and stop millis() itself whenever you want to.

linus_kjk2003 · June 23, 2024, 4:45pm

Schedule overview
I hope this helps you

LarryD · June 23, 2024, 5:21pm

For readability, place { and } on separate lines by themselves, place each line of code on a separate line.
Show us good images of your ‘actual’ wiring.

linus_kjk2003 · June 23, 2024, 5:39pm

this should occur at the time when the timer is up

  if (isAlarm) {
    lcd.setCursor(0, 0);  //reset the LCD
    lcd.clear();                 //reset the LCD

    Serial.print("timeNow:");   //just for me
    Serial.println(timeNow);    //same
    lcd.print(timeNow);            //print the acual time on the LCD
    if (currentMillis - previousMillis >= interval) {                           //  check the inteval
      previousMillis = currentMillis;                                                  //     /\

      if (digitalRead(AlarmPin) == LOW  && isAlarm) {// if alarm is on and the led is on 
        digitalWrite(AlarmPin, HIGH);                             //turn off, else turn it on and then back off
      } else {
        digitalWrite(AlarmPin, LOW);
      }
    }
  } else {
    digitalWrite(AlarmPin, LOW);
  }

linus_kjk2003 · June 23, 2024, 5:48pm

Probably not helping but here is a photo of it and as a reminder the only problem is that the led ist turning on when turning the encoder (I dont want that)

linus_kjk2003 · June 23, 2024, 5:58pm

I dont know how but I fixed it maybe it was the static var. (declare at the beginning) so a BIG thanks to all of you and especially to @hallowed31 here is a video of it working if you had questions:Video (sorry for bad quality (480p is google drives max) edit: you wonder about the wron time no wonder I stole the battery of the rtc

alto777 · June 23, 2024, 7:57pm

@linus_kjk2003 please post the complete sketch that now works so we can see what really was the problem, and how tinkering with lastButtonPress and anything else you may have changed solved the problem.

The problem is solved and may have had something to do with the scope of lastButtonPressl or a conflict of names or whatever.

This, however, is perfectly fine:

static unsigned long lastButtonPress = 0;

and in fact the "= 0" is redundant.

static variables persist, they are used in any function so that next time, the value is retained.

You may also initialise them - automatically a static unsigned long would be initially 0, you can say so or not, I'm.ike to save ink and don't usually. But one might need to

static unsigned long lastButtonPress = 42;

some value other than zero. That assignment will only happen once, and does not keep the variable from persisting with whatever the rest of the code assigns it during the function.

a7

LarryD · June 23, 2024, 8:04pm

Please confirm you are using a current limiting resistor with the LED.

TomGeorge · June 23, 2024, 8:51pm

Sorry No..

Make a decent schematic please.

Where in your "overview" are those LEDs.

Thanks Tom...

hallowed31 · June 23, 2024, 9:13pm

  static unsigned long lastButtonPress = 0;
  if (buttonState == LOW && (currentMillis - lastButtonPress > 50)) {
    lastButtonPress = currentMillis;

this was all in void loop(). In this context, what's happening to the variable under discussion with each cycle of the main loop? Is it being passed and holding the value of currentMillis() in its usage then, or not?

alto777 · June 23, 2024, 9:54pm

I find three occurrences of "lastButtonPress" in the code from post #1.

// this makes a static unsigned long variable with initial value of zero
  static unsigned long lastButtonPress = 0;

// this uses the value in the variable to check for the elapsed time since... 
  if (buttonState == LOW && (currentMillis - lastButtonPress > 50)) {

//...  the last time the variable was moved up to the current time
    lastButtonPress = currentMillis;

It's exactly what a static variable is good for. It cannot be seen outside the function, and it retains its value across function calls.

A global variable would work, but then anyone could might mess with it.

loop() is just another function; the only special thing about it is that we usually do not call it from other code in the sketch, that happens from the main() function all C/C++ programs require, but Arduino users may never see, or even wonder about. When you return from loop() explicitly or by running off the end of the code in the function, main() just immediately calls it again, creating the

// place code here to run over and over and over and over

repetition of the loop function that makes our sketches go.

HTH

a7

Topic		Replies	Views
Fancy Dog Bed w/ FastLeds and Rotary Encoder General Guidance	27	132	April 12, 2026
Beginner making a complex/modular alarm clock Programming	10	350	December 17, 2025
Simple "Alarm"anlage Deutsch	23	258	September 18, 2025
Need to add flashing LED subroutine Programming	31	1228	January 14, 2023
Serial monitor=proof…but the µC (Arduino R3) acts differently! Programming	65	237	January 9, 2026

Led not doing what I want

Related topics