2 Wordclock bugs I can't get rid of

Hello all Arduino-amateurs (and pros).
I have made my first steps in Arduino programming in order to build a word clock for Christmas. So far, the breadboard assembly is working fine, but I am having two issues that I don’t seem to be able to identify. The clock is based on the Joergelis Arduino-Wordclock and trzimm’s World Clock in German using Arduino, with a mask in Luxembourgish and the necessary adaptations to the code:

#define BUTTON_HOUR 8             // digital 8 to hour-button
#define BUTTON_MINUTE 7           // digital 7 to minute-button
#define DATA 2                    // digital 2 to pin 14 on the 74HC595
#define LATCH 3                   // digital 3 to pin 12 on the 74HC595
#define CLOCK 4                   // digital 4 to pin 11 on the 74HC595


// variables for setting-buttons:
int buttonStateHOUR = 0;          // current state of the button
int buttonStateMINUTE = 0;        // current state of the button
int lastButtonStateHOUR = 0;      // previous state of the button
int lastButtonStateMINUTE = 0;    // previous state of the button

// notice: read bytes from right to left:
byte registerByte1 = 0;           // 0 SECHS   | 1 AACHT | 2 ZENG | 3 EELEF  | 4 ZWIELEF  | 5 AUER   | 6 VOID | 7 VOID
byte registerByte2 = 0;           // 0 HALLWER | 1 SIWE  | 2 N    | 3 ENG     | 4 DRAI    | 5 VEIER  | 6 ZWOU | 7 FENNEF
byte registerByte3 = 0;           // 0 ET      | 1 ASS   | 2 ZENG | 3 ZWANZEG | 4 VEIEREL | 5 FENNEF | 6 VIR  | 7 OP

tmElements_t tm;

void setup(){
  analogWrite(oePin, 255);  // turn LED-Stripes off

  
  // Buttons
  pinMode(BUTTON_HOUR, INPUT);
  pinMode(BUTTON_MINUTE, INPUT);
  
  // Shift Register
  pinMode(LATCH, OUTPUT);
  pinMode(CLOCK, OUTPUT);
  pinMode(DATA, OUTPUT);
  
} // end of setup

  // read the pushbutton input pin:
  buttonStateHOUR = digitalRead(BUTTON_HOUR);
  buttonStateMINUTE = digitalRead(BUTTON_MINUTE);

  // Detect HOUR Button
  if (buttonStateHOUR != lastButtonStateHOUR) {
    if (buttonStateHOUR == HIGH) {
      addTime(1,0);
    }
    lastButtonStateHOUR = buttonStateHOUR;
  }
  // Detect MINUTE Button
  if (buttonStateMINUTE != lastButtonStateMINUTE) {
    if (buttonStateMINUTE == HIGH) {
      addTime(0,1);
    }
    lastButtonStateMINUTE = buttonStateMINUTE;
  }

  // Generate time and shiftOut
  timeToBytes();
  updateShiftRegisters();
  printTime();
    
} // end of loop


void timeToBytes(){
  if (RTC.read(tm)) {

    int deutschHour = shortHour(tm.Hour);
    int halb = 0;
    registerByte1 = 0;
    registerByte2 = 0;
    registerByte3 = 0;

//registerByte1: 0 SECHS   | 1 AACHT | 2 ZENG | 3 EELEF  | 4 ZWIELEF  | 5 AUER   | 6 VOID | 7 VOID
//registerByte2: 0 HALLWER | 1 SIWE  | 2 N    | 3 ENG     | 4 DRAI    | 5 VEIER  | 6 ZWOU | 7 FENNEF
//registerByte3: 0 ET      | 1 ASS   | 2 ZENG | 3 ZWANZEG | 4 VEIEREL | 5 FENNEF | 6 VIR  | 7 OP
    
    bitSet(registerByte3,0);      // "ET" always on
    bitSet(registerByte3,1);      // "ASS" always on
      
      if (tm.Minute >= 5 && tm.Minute <= 9){
      bitSet(registerByte3,5);    // "FENNEF"
      bitSet(registerByte3,7);    // "OP"
      } 
      else if (tm.Minute >= 10 && tm.Minute <= 14){
      bitSet(registerByte3,2);    // "ZENG"
      bitSet(registerByte3,7);    // "OP"
      }
      else if (tm.Minute >= 15 && tm.Minute <= 19){
      bitSet(registerByte3,4);    // "VEIEREL"
      bitSet(registerByte3,0);    // "OP"
      }
      else if (tm.Minute >= 20 && tm.Minute <= 24){
      bitSet(registerByte3,3);    // "ZWANZEG"
      bitSet(registerByte3,0);    // "OP"      
      } 
      else if (tm.Minute >= 25 && tm.Minute <= 29){
      bitSet(registerByte3,5);    // "FENNEF"
      bitSet(registerByte3,6);    // "VIR"
      bitSet(registerByte2,0);    // "HALLWER"
      halb = 1;
      }
      else if (tm.Minute >= 30 && tm.Minute <= 34){
      bitSet(registerByte2,0);    // "HALLWER"   
      halb = 1;
      }
      else if (tm.Minute >= 35 && tm.Minute <= 39){
      bitSet(registerByte3,5);    // "FENNEF"
      bitSet(registerByte3,7);    // "OP"
      bitSet(registerByte2,0);    // "HALLWER"
      halb = 1;
      }
      else if (tm.Minute >= 40 && tm.Minute <= 44){
      bitSet(registerByte3,3);    // "ZWANZEG"
      bitSet(registerByte3,6);    // "VIR"
      halb = 1;
      }
      else if (tm.Minute >= 45 && tm.Minute <= 49){
      bitSet(registerByte3,4);    // "VEIEREL" 
      bitSet(registerByte3,6);    // "VIR"
      halb = 1;
      }
      else if (tm.Minute >= 50 && tm.Minute <= 54){
      bitSet(registerByte3,2);    // "ZENG"
      bitSet(registerByte3,6);    // "VIR"
      halb = 1;
      }
      else if (tm.Minute >= 55 && tm.Minute <= 59){
      bitSet(registerByte3,5);    // "FENNEF"
      bitSet(registerByte3,6);    // "VIR"
      halb = 1;
      }

    deutschHour = shortHour(deutschHour + halb);
    if ( deutschHour == 1 ) {
  bitSet(registerByte2,3);    // "ENG"      
    }
    else if ( deutschHour == 2 ) {
    bitSet(registerByte2,6);    // "ZWOU"
    }
    else if ( deutschHour == 3 ) {
    bitSet(registerByte2,4);    // "DRAI"
    }
    else if ( deutschHour == 4 ) {
    bitSet(registerByte2,5);    // "VEIER"
    }
    else if ( deutschHour == 5 ) {
    bitSet(registerByte2,7);    // "FENNEF"
    }
    else if ( deutschHour == 6 ) {
    bitSet(registerByte1,0);    // "SECHS"
    }
    else if ( deutschHour == 7 ) {
    bitSet(registerByte2,1);    // "SIWE"
    bitSet(registerByte2,2);    // "N"
    }
    else if ( deutschHour == 8 ) {
    bitSet(registerByte1,1);    // "AACHT"
    }
    else if ( deutschHour == 9 ) {
    bitSet(registerByte2,2);    // "N"
    bitSet(registerByte2,3);    // "ENG"    
    }
    else if ( deutschHour == 10 ) {
    bitSet(registerByte1,2);    // "ZENG"      
    }
    else if ( deutschHour == 11 ) {
    bitSet(registerByte1,3);    // "EELEF"
    }
    else if ( deutschHour == 12 ) {
    bitSet(registerByte1,4);    // "ZWIELEF"
    }

    // only if minute is between 0-4, light up "AUER"
    if (tm.Minute <= 4){
  bitSet(registerByte1,5);    // "AUER"
        }   
  }
}

void updateShiftRegisters()
{
   digitalWrite(LATCH, LOW);
   shiftOut(DATA, CLOCK, MSBFIRST, registerByte3);
   shiftOut(DATA, CLOCK, MSBFIRST, registerByte2);
   shiftOut(DATA, CLOCK, MSBFIRST, registerByte1);
   digitalWrite(LATCH, HIGH);
}

void addTime (int addHour, int addMinute)
{
  // get time from RTC
  if (RTC.read(tm)) {
    if (addHour > 0) {
      tm.Hour = tm.Hour + addHour;
      if (tm.Hour > 12) {
        tm.Hour = tm.Hour - 12;
      }
    }
    if (addMinute > 0) {
      tm.Minute = tm.Minute + addMinute;
      if (tm.Minute > 59) {
        tm.Minute = tm.Minute - 60;
        addTime(1,0);          
      }
    }  
    
    tm.Second = 0;            // set second to 0 
    RTC.write(tm); 
  }
}

int shortHour(int tempHour)
{
  tempHour = tempHour % 12;
  return tempHour == 0 ? 12 : tempHour;
}

So, without further ado, here’s the bugs:

  1. When I cycle through the minutes with the corresponding button, they will increase until 59 (as checked on the Arduino IDE’s serial monitor) and after that each activation of the minute-button will increase the hour, without resetting the minute count to “0”. Thus, the condition if (tm.Minute > 59) remains true, prompting each time an increase in the hour.

  2. The word “past” (“op” in Luxembourgish, registerByte3,0), is being used with minutes 5, 10, 15, 20 and 35. However, it won’t light up in conjunction with 15 and 20 past. It’s fine with the other though. I checked that it is wired correctly, and a voltmeter test confirmed that, indeed, no current passes when it is 15 or 20 past.

I hope that you’ll be able to find the problem because, frankly, I am out of options at this point.

Many thanks in advance!

Sven

P.S.: I had to delete large parts of the code in order to paste it here.

I don’t know what this means but I am curious:

… build a word cock for Christmas.

maybe some old custom from another country?

YAAARRGHHH! Most awkward typo ever! I’m going to edit the post once the 5 minute period has passed.

Word cock, is that one of those nasty robotic chickens that yells at you 8) ?

"nasty robotic chickens"

lol

.

Is there a section of code missing? I can't find the start of loop. I see the code that goes in loop, I just can't find "void loop() {"

if (addMinute > 0) {
      tm.Minute = tm.Minute + addMinute;
      if (tm.Minute > 59) {
        tm.Minute = tm.Minute - 60;
        addTime(1,0);          
      }
    }

Be careful with that recursion there. I don't think you want that first line of addTime in this case. When you do that, it wipes out the changes you just made to tm.Minute by reading a new time into tm. You can either copy in the relevant hour adding code from addTime here or perhaps a better option would be to rewrite addTime to take the time you want to add to as a parameter so you read the RTC in the function call (or not) instead of in the function itself.

Problem 1: I'd suspect this recursively called routine below.

Test. Comment out the line: addTime(1,0); in the routine addTime().

OK. The hours will not be incremented, but it would be interesting to see if the minutes are correctly handled. At the moment you say these get stuck at 59.

If that doesn't show anything, it maybe that something is silently forbiding the setting of tm.Minute to 60 (although I cannot see how) In which case, you may have to recode that part so before adding anything to tm.Minute, you check if it would cause an overflow to 60 or more, and then set it to an appropriate value between 0 and 59.

void addTime (int addHour, int addMinute)
{
  // get time from RTC
  if (RTC.read(tm)) {
    if (addHour > 0) {
      tm.Hour = tm.Hour + addHour;
      if (tm.Hour > 12) {
        tm.Hour = tm.Hour - 12;
      }
    }
    if (addMinute > 0) {
      tm.Minute = tm.Minute + addMinute;  
      if (tm.Minute > 59) {
        tm.Minute = tm.Minute - 60;
        addTime(1,0);          
      }
    }  
    
    tm.Second = 0;            // set second to 0
    RTC.write(tm);
  }
}

As for problem 2, where it works e.g. 5 past, you have used:

bitSet(registerByte3,7);    // "OP"

Where it fails, eg 15 past, you have used:

bitSet(registerByte3,0);    // "OP"

Could that explain it ?

else if (tm.Minute >= 10 && tm.Minute <= 14){
      bitSet(registerByte3,2);    // "ZENG"
      bitSet(registerByte3,7);    // "OP"
      }
      else if (tm.Minute >= 15 && tm.Minute <= 19){
      bitSet(registerByte3,4);    // "VEIEREL"
      bitSet(registerByte3,0);    // "OP"
      }

Why do those two lines with the same comment “OP” have different code? Earlier in the code you have this comment a couple of times:

//registerByte3: 0 ET      | 1 ASS   | 2 ZENG | 3 ZWANZEG | 4 VEIEREL | 5 FENNEF | 6 VIR  | 7 OP

which would lead me to believe that OP should be bit 7 and not 0. Seems the two problem cases are writing to bit 0.

Well, two votes for both issues it is then.

Delta_G: Well, two votes for both issues it is then.

:)

edit: Delta_G has identified that the recursive call to addTime() is causing the statement RTC.read(tm) to overwrite the attempt to reset tm.Minute to 0. I guess the original author, after testing it, made a change to attempt to handle the situation where the RTC was not running.

One solution is this. Move the read/write of tm here:

  // Detect HOUR Button
  if (buttonStateHOUR != lastButtonStateHOUR) {
    if (buttonStateHOUR == HIGH) {
      RTC.read(tm) ;  //new
      addTime(1,0);
      RTC.write(tm); //new
    }
    lastButtonStateHOUR = buttonStateHOUR;
  }
  // Detect MINUTE Button
  if (buttonStateMINUTE != lastButtonStateMINUTE) {
    if (buttonStateMINUTE == HIGH) {
      RTC.read(tm) ;  //new
      addTime(0,1);
      RTC.write(tm); //new
    }
    lastButtonStateMINUTE = buttonStateMINUTE;
  }

and remove it from here by making the following changes:

void addTime(int addHour, int addMinute)
{
  // get time from RTC
  if (true) {  //was RTC.read(tm)

    if (addHour > 0) {
      tm.Hour = tm.Hour + addHour;
      if (tm.Hour > 12) {
        tm.Hour = tm.Hour - 12;
      }
    }
    
    if (addMinute > 0) {
      tm.Minute = tm.Minute + addMinute;
      if (tm.Minute > 59) {
        tm.Minute = tm.Minute - 60;
        addTime(1,0);
      }
    }  

    tm.Second = 0;
    // RTC.write(tm);   //now inactive here
  }
}

IT WORKS! Thanks lads (and lasses), you're geniuses! Sven

Roude_Leiw: IT WORKS! Thanks lads (and lasses), you're geniuses! Sven

:)