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

signed integer handling in calibration methods #31

Closed HannesJo0139 closed 6 years ago

HannesJo0139 commented 6 years ago

I tested your methods to calibrate by time deviation. As long as trim value is positive, everything is fine.

When I setup a deviation of -1 second and an ExpectedSec of 1 day (86400 secs), the first trim value is -10 what should be correct. I started a new calibration with the same deviation and ExpectedSec then, expecting a new trim value of -20 (Using positive values that is exacty what happened 10, 20, 30, ...).

Unfortunately the method calculated a new trim of 108, which is completely wrong obviously, followed by 98, 88, 78,...

While searching the reason I noticed that you simply write negative trim values to RTC what is invalid regarding to MCP7940 datasheet p. 29. As it is described there, the absolute value of trim must be saved in register bits [0-6]. Setting SIGN bit [7] to 1 will then result in adding this amount of clock cycles. (subtracting for 0 vice versa).

Therefore I suggest changing

int8_t MCP7940_Class::calibrate(const int8_t newTrim) {                     
int8_t trim = newTrim;                                    // Make a local copy                        //
if (trim < 0) {                                                   // if the trim is less than 0, then      //
    trim = 0x80 | (trim * -1);                             // set non-excess 128 negative val  //
}

to

int8_t MCP7940_Class::calibrate(const int8_t newTrim) {                 
    uint8_t trim = abs(newTrim);                       // Get abs                                        //
    if (newTrim < 0)                                           // if the trim is less than 0, then     //
    {                                                             
        trim = 0x80 | trim;                                   // SET SIGN Bit to add clock cycles //
    }

And

int8_t MCP7940_Class::calibrate(const DateTime& dt) {
  ...
  if (trim >> 7) {                                                 // use negative value if necessary  //
      trim = trim * -1;                                               
  }
  ...

to

int8_t MCP7940_Class::calibrate(const DateTime& dt) {
  ...
  if (trim >> 7)                                                    // if SIGN Bit is set               //
  {              
      trim = (~0x80 & trim) * -1;                          // Clear SIGN Bit and make negative //
  }
  ...

These changings seem to lead in the expeced behaviour.

SV-Zanshin commented 6 years ago

Good catch, I missed that completely and have implemented your fixes.