Checkifalarm issue

[Bepsy]

I encountered a problem when using the checkifalarm function. This function gives some issues, since my arduino code is only checking the alarms constantly and not doing anything else,

In the function the flags are red and then reset to 0, even when the flag isn't 1. This is problematic, since my code is nearly all of the time in the checkifalarm function. First the controlbyte is red out in this function, and the flag can be red as zero. In the meanwhile the flag can be set to 1 by the chip, but the code will still set the flag to zero. So the alarm is never triggered. This can easily be solved by the following code change:

bool DS3231::checkIfAlarm(byte Alarm) {
	// Checks whether alarm 1 or alarm 2 flag is on, returns T/F accordingly.
	// Turns flag off, also.
	// defaults to checking alarm 2, unless Alarm == 1.
	byte result;
	byte temp_buffer = readControlByte(1);
	if (Alarm == 1) {
		// Did alarm 1 go off?
		result = temp_buffer & 0b00000001;
		// clear flag
		if (result == 1)
		{
			temp_buffer = temp_buffer & 0b11111110;
			writeControlByte(temp_buffer, 1);
		}
	} else {
		// Did alarm 2 go off?
		result = temp_buffer & 0b00000010;
		// clear flag
		if (result == 0b10)
		{
			temp_buffer = temp_buffer & 0b11111101;
			writeControlByte(temp_buffer, 1);
		}
	}
	return result;
}

I don't know if this is the right place to put it, I am just new on Github. But please take a close look at it.

[jnuernberg]

Hi @Bepsy,
You are absolutely right, the control bit is reset to zero on every call to DS3231::checkIfAlarm and that this can cause issues in polling code (which you are doing as far as I understand). While your code changes overcome this issue, they add a certain overhead to the code.

Another way to prevent the describes issue is to switch over to an interrupt based coding scheme. In that case, you can call DS3231::checkIfAlarm either at the end of your ISR (Interrupt Service Routine) or when you have finished all operations, that are triggered by the alarm. As an additional benefit, you call DS3231::checkIfAlarm only once and are not stuck in a loop querrying this function. I lined out such a scheme here: #17 . Let me know, if this helps.

Best regards, Jacob

[IowaDave]

Hello @Bepsy,

I agree with @jnuernberg that an interrupt-driven approach makes a lot of sense. However, maybe you prefer to check the alarm directly, as you described. As the saying goes, "There is more than one way to do it."

I can think of two things you might try in a "polling" approach, like your own code. In fact, you might combine them.

First, capture the return value of the checkIfAlarm() function in a bool or an int variable. Like this:

bool alarmNeedsAttention = checkIfAlarm(x), where "x" is either 1 for Alarm 1 or 2 for Alarm 2.

The function does report the value that it finds for the flag, false or true (0 or 1) before it clears the flag. You can test it after each time you call the function, like this:

if (alarmNeedsAttention) {
   // your alarm-handing code goes here
}

Second, it sounds like you suspect a so-called "race condition", where your code checks the alarm so fast that the DS3231 never gets a chance to signal an alarm at all! Solution: slow down your code!

Think about it. The DS3231 advances its time internally at one-second intervals. it does no good to check the alarm more frequently than once per second. So, put the call to checkIfAlarm() inside a "smart" delay block. The millis() function is your friend for this purpose. You could try something like the following, which brings both ideas together.

// declare and initialize a couple of global variables
unsigned long nextCheck = millis() + 1000;
bool alarmNeedsAttention;

// later, in your code, use the variable to check at one-second intervals
// using Alarm 1 for the example
// because it is the only alarm that can be set down to the second
if (millis() > nextCheck) // time has advanced by one second (1,000 milliseconds)
{
  alarmNeedsAttention = checkIfAlarm(1); // capture current alarm flag status
  nextCheck += 1000; // check again after 1 second (1,000 milliseconds)
}
if (alarmNeedsAttention)
{
  // your alarm handling code goes here
}

My best to you,

IowaDave

[IowaDave]

Not a Bug?

For what it's worth my view is that clearing an alarm flag bit in the DS3231 is a deliberate side effect of the checkIfAlarm(), function, but not a bug because it works as it was designed to work. Should clearing the flag be removed to its own, separate function? I'd argue No, for two reasons:

1. Doing so would break a lot of people's code that relies on the function working the way it does.
2. The case where someone would want to leave the alarm flag set after reading its value seems like a rare need. Clearing it automatically after checking it makes the most sense.

Workable tools reward the effort invested to learn them.

[jnuernberg]

Hi @Bepsy,

I added a checkIfAlarm function with optional clearing of the flags in branch issue9. Call signature reads as

bool DS3231::checkIfAlarm(byte Alarm, bool clearflag)

I did not manage to test it yet, so I refrained from opening a pull-request.

I hope to manage that soon,

Cheers, Jacob

[awickert]

Closing after merging PR #58 with thanks to @jnuernberg and possible extreme patience from @Bepsy.