search

Zanduino / MCP7940

Arduino Library to access the MCP7940M, MCP7940N and MCP7940x Real-Time chips
GNU General Public License v3.0
35 stars 22 forks source link

Observation about calibrate... #42

Closed davidlehrian closed 5 years ago

davidlehrian commented 5 years ago

I'm looking at the two calibrate methods

int8_t MCP7940_Class::calibrate(const DateTime& dt) and int8_t MCP7940_Class::calibrate(const int8_t newTrim)

and am concerned that while the trim value gets set, the actual time is never adjusted. _SetUnixTime is updated to be the current unit time and the comment says "Store time of last change" but all that changed was the trim value. So if the clock was off, it will still be off and the next time calibrate is called the same trim value (assuming the clock was 100% accurate after the current trim) will be calculated and added to the trim throwing the trim off. I'm thinking that the time should be adjusted and _SetUnixTime set as soon as the trim value is calculated. What do you think?

SV-Zanshin commented 5 years ago

That sounds correct in that multiple calibrations would lead to progressively incorrect trim values. I will look into this later in the week.

SV-Zanshin commented 5 years ago

I've thought about the overloaded "calibrate(DateTime)" call and that certainly should include a call to change the RTC's time, since the call itself is unambiguous regarding the correct time. On the other hand, the call to "calibrate(newTrim)" could be called after the correct time has already been adjusted, so that might introduce an error if the actual time were to be changed.

I've made the one change as that makes perfect sense.

davidlehrian commented 5 years ago

I agree that what you have done looks correct. My only additional comment is that the time should probably be set after SecDeviation is calculated and before ExpectedSec is calculated otherwise ExpectedSec is being calculated with the old incorrect time. Or ExpectedSec could be calculated with dt instead of now() since ExpectedSec is the difference between the last time the system was calibrated and the current actual time. The former is probably better since it sets the clock as quickly as possibly in the method without introducing any additional microsecond lag from the time it takes to run the method. Agreed it probably makes little difference, but it is more accurate.