Difficulties displaying time within a function

Firstly, I’m very new to Arduino and C++ programming, so forgive me if there is something very basic I’m missing here.

I’m trying to write some code that includes a timer set to trigger once per second.

For the moment I’m just trying to test this with the display of time from a real time clock when the timer triggers. For some reason, my program is hanging when it tries to run the line “now=rtc.now();” in the ISR routine. If I comment that line out, everything works perfectly. Obviously the displayed time is wrong since it could not retrieve it from the RTC, but otherwise all is good.

More Detail regarding the symptoms:

You will note that I have a couple of test messages in the code. First, there is the message “Ready to setup timer” and this gets displayed to the console. Next, there is the message “Timer setup done” and this does not get displayed but I’ve figured out that this is OK. This is because the timer is fired the first time before it gets to that line.

In the ISR, the message “Just prior to getting the time” prints the first two characters (Ju) and then it hangs.

Again, if I comment out the “now=rtc.now();” line, it runs fine and that message “Timer setup done” gets displayed after the first time through the ISR routine.

Note too that the issue seems to be unrelated to the fact that I’m using a timer. I tried simply creating a standard function and it fails there as well, but works outside of a function. So it seems to be related only to it being within a function (either a standard function or my ISR function).

Can anyone shed any light on what I’m missing here?

Here is the code in question:

#include <Wire.h>
#include "RTClib.h"

RTC_DS3231 rtc;

char daysOfTheWeek[7][12] = {"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"};
DateTime now;

void setup ()
{
  Serial.begin(115200);

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


  Serial.println("Ready to setup timer");
  delay(2000);

  // Setup the timer to fire once per second

  cli();//stop interrupts while setting up the timer

  //set timer1 interrupt at 1Hz
  TCCR1A = 0;// set entire TCCR1A register to 0
  TCCR1B = 0;// same for TCCR1B
  TCNT1  = 0;//initialize counter value to 0
  // set compare match register for 1hz increments
  OCR1A = 15624;// = (16*10^6) / (1*1024) - 1 (must be <65536)
  // turn on CTC mode
  TCCR1B |= (1 << WGM12);
  // Set CS12 and CS10 bits for 1024 prescaler
  TCCR1B |= (1 << CS12) | (1 << CS10);
  // enable timer compare interrupt
  TIMSK1 |= (1 << OCIE1A);

  sei();//allow interrupts

  // End of timer setup
  Serial.println("Timer setup done");
}


// ISR - Interrupt Service Routine
ISR(TIMER1_COMPA_vect) { //timer1 interrupt 1Hz

  Serial.println("Just prior to getting the time");

  now = rtc.now();

  Serial.println("Current Date & Time: ");
  Serial.print(now.year(), DEC);
  Serial.print('/');
  Serial.print(now.month(), DEC);
  Serial.print('/');
  Serial.print(now.day(), DEC);
  Serial.print(" (");
  Serial.print(daysOfTheWeek[now.dayOfTheWeek()]);
  Serial.print(") ");
  Serial.print(now.hour(), DEC);
  Serial.print(':');
  Serial.print(now.minute(), DEC);
  Serial.print(':');
  Serial.print(now.second(), DEC);
  Serial.println();
  return;
}


void loop ()
{
  while (1);
}

EDIT:

I forgot to note that is an Arduino Mega 2560 and the RTC is a DS3231SN.

You are using Serial in an interrupt routine, which deadlocks the processor as Serial waits for interrupts.

An ISR should do the absolute minimum and return. Things never to call in an ISR include delay() and
any Serial method.

To trigger every second this will work:

unsigned long last_time = 0L;

void loop()
{
  if (millis() - last_time >= 1000)
  {
    last_time += 1000 ;
   
    Serial.println("Just prior to getting the time");

    now = rtc.now();

    Serial.println("Current Date & Time: ");
    Serial.print(now.year(), DEC);
    Serial.print('/');
    Serial.print(now.month(), DEC);
    Serial.print('/');
    Serial.print(now.day(), DEC);
    Serial.print(" (");
    Serial.print(daysOfTheWeek[now.dayOfTheWeek()]);
    Serial.print(") ");
    Serial.print(now.hour(), DEC);
    Serial.print(':');
    Serial.print(now.minute(), DEC);
    Serial.print(':');
    Serial.print(now.second(), DEC);
    Serial.println();
  }
}

Also this line is entirely redundant:

  return;
}

Since every function returns at the end. If your function returned a value then you'd need a return statement
to select the value.

ISR(TIMER1_COMPA_vect) { //timer1 interrupt 1Hz

  Serial.println("Just prior to getting the time");

  now = rtc.now();

I think the .now() method uses the Wire library to fetch the current date and time and the Wire library requires interrupts. The Serial output manages to get two characters out before the processor gets stuck in a loop.

Note: Serial output WILL work with interrupts disabled. See: HardwareSerial.cpp:

  // If the output buffer is full, there's nothing for it other than to 
  // wait for the interrupt handler to empty it a bit
  while (i == _tx_buffer_tail) {
    if (bit_is_clear(SREG, SREG_I)) {
      // Interrupts are disabled, so we'll have to poll the data
      // register empty flag ourselves. If it is set, pretend an
      // interrupt has happened and call the handler to free up
      // space for us.
      if(bit_is_set(*_ucsra, UDRE0))
	_tx_udr_empty_irq();
    } else {
      // nop, the interrupt handler will free up space for us
    }
  }

If interrupts are disabled and the output buffer is full, the .write() function will wait for the interrupt flag to get set. The .flush() function is similar so you can flush the output buffer through even with interrupts disabled.

Thanks greatly for the help.