RTC_DS3231.cpp getTemperature() doesn't provide for negative temps

[Fishbone69]

I believe as written, the DS3231 method for getting the temperature does not look at the MSB of the high byte to determine if the temperature is positive or negative. I would recommend something like this for the update:

float RTC_DS3231::getTemperature() { uint8_t buffer[2] = {DS3231_TEMPERATUREREG, 0}; float temperatureDegC; i2c_dev->write_then_read(buffer, 1, buffer, 2); temperatureDegC = (float)buffer[0] + (float)(buffer[1] >> 6) * 0.25f; if (buffer[0] & 0x80) { temperatureDegC *= -1.0f; } return temperatureDegC; }

[edgar-bonet]

the DS3231 method for getting the temperature does not look at the MSB of the high byte to determine if the temperature is positive or negative.

Indeed you are right. This method will fail to correctly read a negative temperature.

if (buffer[0] & 0x80) {
  temperatureDegC *= -1.0f;
}

This won't work either. It would, if the temperature was encoded using the sign–magnitude representation. However, according to the datasheet of the DS3231, “The temperature is encoded in two’s complement format.” (link mine).

I would instead try this:

float RTC_DS3231::getTemperature() {
  uint8_t buffer[2] = {DS3231_TEMPERATUREREG, 0};
  i2c_dev->write_then_read(buffer, 1, buffer, 2);
  int16_t temp = uint16_t(buffer[0]) << 8 | buffer[1];
  return temp * (1 / 256.0);
}

Some notes:

• Casting buffer[0] to an unsigned type is meant to avoid signed integer overflow, which is undefined behavior in C++.
• Assigning the result to a signed integer makes numbers with the most significant bit set be interpreted as negative. There is no extra work to do, as all current processors use two’s complement representation internally.
• Multiplying by 1 / 256.0 (rather than 1 / 4.0) obviates the need for the 6-bit shift. Also, 1 / 256.0 being a compile-time constant, there is no division performed at run-time (which would be expensive).

Could you give a try to this implementation?

[Fishbone69]

I'm not an experienced coder so I am glad there are folks like you to take me to school. I did stupidly assume sign-magnitude representation. I did some reseach and your suggestion not only appears correct but is far more elegant. I'll try it when I get home. Thank you!!

[i440bx]

float RTC_DS3231::getTemperature() {
  uint8_t buffer[2] = {DS3231_TEMPERATUREREG, 0};
  i2c_dev->write_then_read(buffer, 1, buffer, 2);
  int16_t temp = uint16_t(buffer[0]) << 8 | buffer[1];
  return temp * (1 / 256.0);
}

Could you give a try to this implementation?

I have copied the code into my RTC_DS3231.cpp and it runs flawless :) It shows negative temperatures low to -27.00°C.

Lower my medical ice spray cant cool down.

[edgar-bonet]

@i440bx: Thanks for testing this! I just submitted this change as pull request #303.