PauseClock option in checkAlarms uses wrong register - Missing alarms "race condition" concern is not valid

[oschonrock]

if(PauseClock)                                                                                                                                                                      
  {                                                                                                                                                                                   
    if(rtc_i2c_read_byte(0xE,StatusByte))

But it should be 0x0F: https://datasheets.maximintegrated.com/en/ds/DS3231.pdf

Status Register (0Fh)
Bit 7: Oscillator Stop Flag (OSF).
....

On a related matter: Initially I agreed with the comments in code regarding the small possibility of a race condition and therefore missing the alarm. Hence the PauseClock option. However, given this code has had the wrong register in it since it was first published and given it is a popular library which has been used by thousands (?) of people, and no one ever complained about missing alarms... It's a hint.

Really, IMHO, this concern is just deeply flawed. This is the code.

rtc_i2c_read_byte(0xF,StatusByte);                                                                                                                                                  

  if(StatusByte & 0x3)                                                                                                                                                                
  {                                                                                                                                                                                   
    // Clear the alarm                                                                                                                                                                
    rtc_i2c_write_byte(0xF,StatusByte & ~0x3);                                                                                                                                        
  }                                                                                                                                                                                   

You are only clearing the flag when it is set. So how can you "miss it"?

• If you clear it, it was set, so by definition you have not missed it?
• If it's not set when you do the read, then you won't clear it, and you will not detect (!) the alarm on this call of checkAlarm. But you will catch it on the next call? (this presumes polling, which is what this method is for, rather than interrupt).

Sorry if I have misunderstood something, but it seems to me that you are "treating a symptom that doesn't exist (missing alarms) with a placebo (writing to the wrong register)."

The only situation where your concern might still be valid, is if when using 2 alarms simultaneously. Ie 1 is set on read and 2 is not set, then 2 becomes set before we write and clear it. But even that is flawed if we (reasonably) assume that the DS3231 uses the same time in seconds to set both flags or none. So again you are not going to miss anything ...?

IMHO, the PauseClock param should be removed (complete with code writing to incorrect register). And same for the comments outlining the concern. The concern is not valid.

[sleemanj]

Been too many years since I wrote this to remember, but

There are 2 alarms, eahch has it's own bit in the status register (which 0x3 covers both for the bitwise comparison) so the race condition (because checkAlarms doesn't care which alarm fired, one the other or both) possible is...

1. Alarm 1 Fires and bit 0 is set in the status register
2. Read status register (and save to variable) and see that one of bit 0 or bit 1 was set so alarm was fired
3. Alarm 2 Fires and bit 1 is set in the status register
4. Clear Status, both bit 0 and 1 are cleared
5. Return the status we read, which was Alarm 1 fired, bit 0 set, but not bit 1, Alarm 2 is lost.

A quick skim (again long long time since I wrote this) says that 0X0E (page 13) is the control register bit 7 is what we WANT the clock to be enabled or disabled, while 0X0F (page 14) is the status register which tells us if it is enabled/disabled (which it may be disabled even if we wanted it enabled, for various reasons).

I think that the code is correct to write to the control register (E) bit 7, although, does rely on the clock running exclusively from battery (page 13, bit 7 details). It's not clear to me from the datasheet if it's ok to write 1 to the status register (F).

[oschonrock]

Re 2 alarms. I think, I updated my above report concurrently with your answer. But again briefly here:

I do not believe a race condition is possible even with 2 alarms, because the alarms bits inside the DS3231 are triggered by matching the current time (minutes, seconds etc) against the alarm time via a mask . This is not spelled out in the Datasheet, but as a reasonable "assumption" (dangerous word, I know) the DS3231 will change both alarm bits at the same time, ie when the seconds match the alarm time - on that very internal clock pulse the 2 flip-flips on the silicon will change. The A1F and A2F flasg can only change once per second, by definition. If the Alarms are both set to same time, then will either both or neither become set on the same internal DS3231 clock.

So, no, I do not believe that it is possible for you to get "in between 2 internal DS3231 clock pulses" with your I2C request.

Regarding the Oscillator enable: You're right regarding Register 0x0F. I misread that on skimming. It's an indicator, not a control bit.

However, as you said 0x0E is for VBAT only:

Control Register (0Eh)
Bit 7: Enable Oscillator (EOSC). When set to logic 0,
the oscillator is started. When set to logic 1, the oscillator
is stopped when the DS3231 switches to VBAT. This bit
is clear (logic 0) when power is first applied. When the
DS3231 is powered by VCC, the oscillator is always on
regardless of the status of the EOSC bit. When EOSC is
disabled, all register data is static.

However, checkAlarms() is for polling, ie we have chosen that we do not want to use a hardware interrupt. Under what circumstances might we be polling the DS3231 when it is on VBAT? What are we polling it with? An MCU which is also on battery? And the DS3231 consumes so much power that we shut it down, but not the arduino MCU?

So I think my original conclusion is probably still correct:

1. There is no race condition - (Also, No other DS3231 lib that I am aware of attempts to deal with it...)

2. Setting 0x0E.7 to 1 will not stop the clock, because the DS3231 will not be on VBAT, because we are talking to it from an ardudino which clearly has Vcc and so will the clock - It would be a very very strange application, where that is not true.

So - symptom doesn't exist, and the treatment is ineffective.

[sleemanj]

1. Alarm 1 fires and bit set at Second N
2. At Second N+(999999 microseconds) we read status
3. At Second N+1 Alarm 2 fires and bit set
4. At Second N+1+(1 microsecond) we clear status

You are free to fork as you disagree, I however and closing wontfix.

[oschonrock]

1. Alarm 1 fires and bit set at Second N

2. At Second N+(999999 microseconds) we read status

3. At Second N+1 Alarm 2 fires and bit set

4. At Second N+1+(1 microsecond) we clear status

Ok, granted if you set your alarms 1 second apart, that is perhaps the ONLY situation. Given you can't set the alarm2 seconds (just not supported), that would only apply if alarm1 was on 59s or 01s.

And then if the user of your lib reads your very kind comment, they may choose to "disable the oscillator" to ensure that doesn't happen, (making their time inaccurate!)..and then that won't work, because they are actually supplying power to their DS3231 when they are talking to it from a powered up MCU.

That is the real risk here. That the user will read the comment in the .h and pass PauseClock=true. Making their time inaccurate for a case which will basically never occur. No luck involved, but only only if alarm1 = HH:MM:59 and alarm2=HH:MM+1:00 and then there is a 1/32768 window. So at once per day, that's ~ 1/100years.... crickey the Sun might spontaenously extinguish first.

I almost did it, before looking at this in detail, I nearly passed PauseClock=true, because I have done lots of C/C++ with race conditions, and I care. And what would I have got. No gain (because I am not on VBAT) just less accurate time, which is the whole point of that chip. The least you should do is change the comment. To explain that passing PauseClock=true could only possibly help if your DS3231 is powered down and on VBAT, while you are polling it over I2C.

You are free to fork as you disagree, I however and closing wontfix.

Actually I already stopped using the lib because it did other debatable things like wipe stored alarms on startup, meaning I had to restore them from eeprom. Also, I switched to using a pic, not an arduino, and while writing the DS3231 comms for that it occurred to me that none of this crazy stuff was needed and that it wouldn't work anyway. So I tried to help improve your very good lib by reporting it.

I actually thought your lib was one of the better arduino ones out there for the DS3231. Shame you don't want to improve/fix it.

Obviously it's very hard to admit when you're wrong isn't it? I have already twice conceded, yet you seem unable to see that what you have is, at best, highly misleading and ridiculous, and probably just wrong.

[sleemanj]

if alarm1 = HH:MM:59 and alarm2=HH:MM+1:00

The race condition will occur when the second alarm (be it alarm 1 or alarm 2) fires after the poll but before the state is updated. The first alarm to fire could be ages before you poll. The alarms do not need to fire 1 second apart to cause the race.

Yes it is a very small window of time, microseconds usually but because checkAlarms does not disable interrupts, potentially variable and not insignificant.

Conversation ends.