Mixing both an RTC and an interrupt pulse counter

I couldn't decide if this question belonged best in the 'Programming' , 'Network-I2C', or 'Sensors' thread, so I'm placing it here. (Moderator, please move to where most appropriate).

As the topic suggests, I'm trying to combine I2C RTC code with interrupt pulse counter code. I'm using the ChronoDot/DS3231. As a quick feasibility test, I grabbed the Chronodot code from Adafruit and the interrupt pulse counting code from elsewhere online. I'm not using any custom libraries.

The RTC code works fine on its own, as does the interrupt code, but they do not work well together. For example, if I comment out each other's code, the other will work, and visaversa. When added together, I can get the serial monitor to display one time and one pulse frequency loop iterations worth, but it doesn't continue. I've tried replacing the Chronodot code with the RTClib.h code, however the main loop still hangs after 1 iteration.

I've also tried replacing the interrupt pulse counting code with the FreqCounter.h library, which does work, however, I prefer not to use custom libraries and would like to know what is going wrong with my seemingly simple code. Is the interrupt not detaching? Is a certain delay needed in one place to allow for something to be available? I'm sure someone must have run into this before, but my search came up empty. Any help would be most appreciated.

The code is as follows,

#include <Wire.h>

int seconds;
int minutes;
int hours;
volatile int Counter;
int Time1;
int Time2;
float PulseFrequency;

void setup() {
  seconds = 0;
  minutes = 0;
  hours = 0;
  Counter = 0;
  Time1 = 0;
  Time2 = 0;
  PulseFrequency = 0.0;
  Wire.begin();
  Wire.beginTransmission(0x68); // Hex I2C address of DS3231
  Wire.send(0x0E); // Select register
  Wire.send(0b00011100); // Write register bitmap, bit 7 is /EOSC
  Wire.endTransmission();
  Serial.begin(115200);
  pinMode(2, INPUT); //Set Arduino pin D2 as an input
  digitalWrite(2, LOW); //Set pin D2's pull-up resistor off since not using switched (Reed/Hall) flow meter
  attachInterrupt(0, CounterFunction, RISING); // Using interrupt 0 on pin D2 as a hardware pulse counter
}

void loop() {
  // Get RTC time
  Wire.beginTransmission(0x68); // 0x68 is DS3231 device address
  Wire.send(0); // Start at register 0
  Wire.endTransmission();
  Wire.requestFrom(0x68, 3); // Request three bytes (seconds, minutes, hours)
  while(Wire.available())
  { 
    seconds = Wire.receive(); // get seconds
    minutes = Wire.receive(); // get minutes
    hours = Wire.receive();   // get hours
 
    seconds = (((seconds & 0b11110000)>>4)*10 + (seconds & 0b00001111)); // Convert BCD to decimal
    minutes = (((minutes & 0b11110000)>>4)*10 + (minutes & 0b00001111)); // Convert BCD to decimal
    hours = (((hours & 0b00110000)>>4)*10 + (hours & 0b00001111)); // Convert BCD to decimal in 24-hour mode
    Serial.print("Time: ");
    Serial.print(hours); Serial.print(":"); Serial.print(minutes); Serial.print(":"); Serial.println(seconds);
  }

  // Interrupt-based frequency counter
  Counter = 0;	//Reset Counter to 0
  sei(); //Enable interrupt
  Time1 = millis();  //millis() time is in milliseconds
  delay (2000);	//Count pulses for 2000 ms (2000 ms works well for the 20-1000 Hz counting range)
  cli(); //Disable interrupt (so the Arduino can do other things)
  Time2 = millis();
  PulseFrequency = (1000.0 * Counter / (Time2 - Time1)); //Result is in pulses/second
  Serial.print("Pulse Frequency (Hz): ");
  Serial.println(PulseFrequency,2);
  delay(1000);
}

void CounterFunction () {    //This is the function that the interupt calls when enabled/attached
  Counter++;  //This function increments the counter when it sees a rising/falling edge pair
}

The delay() function uses the millis() timer which stops running while interrupts are disabled. It won't work when interrupts are disabled. That's why your code hangs after displaying the frequency.

John, thank you for this little insight.

I have replaced:
sei() with attachInterrupt(0, CounterFunction, RISING)
and
cli() with detachInterrupt(0)

while leaving everything else the same. This way I am only attaching/detaching interrupt 0 where the counter is being used. The code now works as expected.

Does it essential to less the delay period?

Make the interrupt routine run all the time and count pulses. Only mask interrupts when reading the (volatile) variables that it is updating. The whole point of interrupts is that they can happen as you do other stuff.

Make the interrupt routine as short as possible of course, something like:

volatile unsigned int count ;

void handle_int ()
{
  count++ ;
}

You then disable interrupts briefly to read the value of count into a local variable, then call delay(), then disable interrupts briefly to read count again... unsigned int wraps round every 65536 pulses though, an unsigned long might be needed in some cases (unsigned char might be enough for very low pulse frequency).