JChristensen / DS3232RTC

Arduino Library for Maxim Integrated DS3232 and DS3231 Real-Time Clocks
GNU General Public License v3.0
394 stars 137 forks source link

Jan 28 updates to oscStopped() broke backward compability #9

Closed bperrybap closed 6 years ago

bperrybap commented 9 years ago

When oscStopped() was updated to include an optional flag to clear the stop status in the RTC, the flag is optional and defaults to true. I believe that defaulting the flag to true is a bad choice since it breaks backward compatibility with the existing API. i.e. oscStopped() didn't clear the OSF bit in the past. Now it does. To me this is a fairly significant and non desirable change since if you have older code that is trying to test the validity of the time in the RTC, the new library will now also mark the time in the RTC as valid.

The issue is that because the flag defaults to true, older sketch code can break with the newer library and any newer sketch code that calls the function like oscStopped(false) to prevent the clearing the OSF bit, will fail to compile on the older library.

Prior to this, sketch code could check the RTC stopped status (or the validity of the time) by using:

if(oscStopped())

Now if code does this same operation, it will also clear the state which marks the time in the RTC as being valid since the new API overloads this function with oscStopped(true).

I believe that this is bad because it now creates a significant unexpected behavior that is different from the previous version of the library.

Now that OSF gets cleared with setting the time, I'm not sure what value clearing OSF with oscStopped() has, but assuming it is still desirable to have, then I think it should be done in a way that doesn't break backward compatibility with code that used the previous oscStopped() API.

For example, Suppose you have code that checks for the RTC time being invalid/corrupt by calling oscStopped() with no argument (before this optional flag existed) and then fell into some code to set the time. If there is a reset or power condition issue that causes a restart or a reset, then the next time the processor starts up it will see the time in the RTC as being valid rather than as still being invalid even though the time was never set.

In an arduino environment, where things like the OS and the IDE often open the serial ports and do probing, there can be resets do to the autoreset hardware on the arduino boards.

This new behavior burned me on my clock project. Took me a little while to figure out why. In my code if the time in the RTC is invalid I fall into a clock setting routine. The code was checking for RTC time validity using oscStopped() with no argument. Because of the all serial port probes, the board gets reset a few times when plugged into the PC. Because of the new behavior the clock essentially got set to a garbage time because oscStopped() clears the OSF bit which marks the time in the RTC as valid.

The worst case scenario is right after you replace the battery. On powerup the OSF bit will be set to indicate that the time is bad, but if plugged into a PC that does port probing, the garbage time can end up being marked as being valid if the code calls oscStopped() with no argument.

To me, it would make much more sense if the default flag value were false when omitted to ensure backward compatibility with the older API which simply read the status and didn't mess with it.

i.e. if you want to clear the OSF bit, you have to explicitly ask for it to be done by setting the flag to true.