ppm out of range

[davidlehrian]

Another thought I was having about calibrate is with regard to the PPM being adjusted to be within the range -130 to 130. I know this is done to keep the trim from overflowing -127/127, but if the PPM is out of range then the time has gotten WAY off. For example ~34 min in 6 months or ~1.5 min a week will give a value of 130 ppm. If the clock is performing that poorly it is at the edge of being able to be corrected and if the value is significantly outside of that there is probably a problem with the chip/crystal/design. What might make more sense in that case would be to assume that the clock was simply wrong when the calibrate method was called. So I'm proposing a method that is calibrateOrAdjust (which I have already implemented) that would either calibrate the chip if the PPM is within range, or adjust the clock to be the correct value (I haven't contemplated if the trim should be zeroed out or not when the clock is adjusted, I'm thinking not, but maybe that is an optional flag passed in to the method).

This came up in my application as I was thinking about daylight savings time. I get the time passed in periodically and I use the passed in value to initially set the time if the oscillator isn't running or to calibrate the chip if the oscillator is running. But twice a year, because I haven't coded to adjust the clock for daylight savings time, the time passed in will be an hour off. This will create a PPM error of approx. 230 or -230 which will cause maximum trim values to be ADDED to or SUBTRACTED from the existing trim value which, as I continue looking at the code, will possibly cause the following line in calibrate to overflow if the previous trim value was not 0. There should probably be a check on the line

trim += ppm * 32768 * 60 / 2000000; // compute the new trim value

to prevent this.

And as I noted in my other post, the time will still be wrong since the time doesn't appear to be adjusted when the timer is calibrated.

[SV-Zanshin]

I've "clamped" the values of the line you quoted to be in range, plus modified the current Date/Time when calibrating with a new DateTime, but otherwise not made changes in the calibration.

If you have a new function to add to the library as you mentioned above, you could post that to an issue or do GitHub pull request so that the change gets integrated.

-Arnd.

[davidlehrian]

You are clamping the value to -130 and 130 and it needs to be clamped to -127 and 127. In addition to that, the overflow would have already occurred on the previous line since the trim value is declared to be int8_t. It should be declared int16_t to prevent overflow on the line I quoted then you can clamp it to -127 and 127.