Correct library for DS3231

[smz]

See: https://github.com/Makuna/Rtc/issues/49

Thanks @mrwgx3

[smz]

@mrwgx3, I'm answering here as I don't want to interfere with the the thread regarding the @makuna's library.

Contrary to what @makuna does in his library, here I just have an GetTemperature() method that simply returns the floating point temperature.

If I'm not mistaken, to correct my code according to your findings, I should modify my code in GetTemperature() from:

float degrees = (float) _wire.read();
// fraction is just the upper bits
// representing 1/4 of a degree
uint8_t fract = (_wire.read() >> 6) * 25;

degrees += (float) fract / ((degrees < 0) ? -100.0f : 100.0f) ;
return degrees;

to:

uint8_t mstemp = _wire.read();
uint8_t lstemp = _wire.read();
int16_t scaledtemp = mstemp << 8 | lstemp;  // This should be the same as (mstemp << 8) + lstemp
return ((float) scaledtemp  / 256.0f );

Is the above correct?

Unfortunately I don't have a DS3231 at hand right now (mine died sometime ago), so I will be really grateful if you could give it a try...

And of course yes, a PR will be appreciated too!

[smz]

... and to save a couple of precious bytes and a couple of instructions, it could probably be something like:

union ds3231temp_t {
  int16_t v;
  struct {
    uint8_t hi;
    uint8_t lo;
  } b;
} ds3231temp;

ds3231temp.b.hi = _wire.read();
ds3231temp.b.lo = _wire.read();

return (float) ds3231temp.v / 256.0f;

[mrwgx3]

Per your request, here are some test results...

float RtcDS3231::GetTemperature ( void )
{
/*
// Code used in my RTC library does not use the Wire library,
// hence I do my own buffering

// Read scaled 16-bit 2's complement integer
// Result returned in bufRtc[0 through 1]
   readDev( RTC_REG_TEMPERATURE, RTC_SIZE_TEMPERATURE );

// Unscaled float temperature
   return(  (int16_t)( (bufRtc[ 0 ] << 8) + bufRtc[ 1 ] ) / 256.0f  );
*/

// Code equivalent for WIRE library
// Read scaled 16-bit 2's complement integer
   Wire.beginTransmission( RTC_I2C_ADDR  );  // For DS3231,  0x68
   Wire.write( RTC_REG_TEMPERATURE );        // ..........., 0x11
   Wire.endTransmission();
   Wire.requestFrom(RTC_I2C_ADDR, 2 );

/* TEST RESULTS

// This works fine
   uint8_t mstemp = Wire.read();
   uint8_t lstemp = Wire.read();
   int16_t scaledtemp = mstemp << 8 | lstemp;  // This should be the same as (mstemp << 8) + lstemp
   return ((float) scaledtemp  / 256.0f );

// --------------------------------

// Doesn't work for the ESP8266, as its byte order is LITTLE ENDIAN
   union ds3231temp_t {
     int16_t v;
     struct {
       uint8_t hi;     // BIG ENDIAN
       uint8_t lo;
     } b;
   } ds3231temp;

   ds3231temp.b.hi = Wire.read();
   ds3231temp.b.lo = Wire.read();
   return (float) ds3231temp.v / 256.0f;

// --------------------------------

// This does work for the ESP8266
   union ds3231temp_t {
     int16_t v;
     struct {
       uint8_t lo;      // LITTLE ENDIAN
       uint8_t hi;
     } b;
   } ds3231temp;

   ds3231temp.b.hi = Wire.read();
   ds3231temp.b.lo = Wire.read();
   return (float) ds3231temp.v / 256.0f;

// --------------------------------

// As for saving bytes, one would thinks this would be close to optimal.
// It does not work, howver, as Wire.read() sequence order is evaluated
// incorrectly
   return(  (int16_t)( (Wire.read() << 8) | (uint8_t)Wire.read() ) / 256.0f  );
*/

// This works because Wire.read() sequence order is enforced
// If you don't need the raw temperature elsewhere, I'd go with this endian independent method
   uint8_t mstemp = Wire.read();
   return(  (int16_t)( (mstemp << 8) | (uint8_t)Wire.read() ) / 256.0f  );

} //GetTemperature

[mrwgx3]

Have generated pull-request #5

[smz]

@mrwgx3: thank you, thank you, thank you!

Gosh... I didn't thought about different processors having a different endianess: thanks for that too!

I'm going to merge your PR and in a few days (after having reviewed if there anything else I can fix/upgrade) I'll release a new version incorporating your fixes.

Best regards,

Sergio

[smz]

Closing this issue as we have a valid PR (https://github.com/smz/Arduino-RTCtime/pull/5)

[smz]

Conversation locked: please open a new issue if you need...