Solved! Long Longs are (timb was) Wrong Wrong?

The übergeek in me is mildly disappointed this got answered before I saw it. :stuck_out_tongue_winking_eye:

Awesome question and awesome answers.

Such bits. Wow. So overflow.

3 Likes

This is a new revelation for me... and it explains quite a bit, actually! Now I know why I was getting SOS in that code last week... o_O

No, things aren't quite working. What I meant is that where they have <<16 and >>7, the datasheet says it should be << 17 (2^17) and >> 6 (2^6). I'm assuming they were going off an older datasheet or something?

Good to know!

Has the web ide been updated to tell us how much space we’re using? I’d forgotten that we have so little memory.

Now I feel like a really stupid idiot…

while(Wire.available()) {
    rawT = Wire.read();
    rawT = rawP << 8 | Wire.read();
    rawT = rawP << 8 | Wire.read();
}

Durrrrrrrrrrrrrr. I copied and pasted the the section that reads the temperature register to use for the pressure section, and forgot the change the first portion of variables! So that’s something.

Now that I’ve got that fixed I’m still not getting the right readings, but at least some of these fixes will actually do something.

So, doing the math by hand—per the data sheet—with the raw data from the Core looks like this:

C1 = 44527
C2 = 40588
C3 = 27507
C4 = 25360
dT = 66678
D1 = 6193396

OFF = C2 * 2^17 + (C4 * dT) / 2^6 => 5,346,371,493.5
SENS = C1 * 2^16 + (C3 * dT) / 2^7 => 2,932,450,470.0156
P = ((((D1 * SENS) / 2^21) - OFF) / 2^15) => 101,131.0573

1011.31mb

It comes out with what the pressure should be! Looking at those results, it’s clear that OFF is going to overflow. However, I still can’t seem to figure out the best way to work the math. Maybe I’m just getting tired and should sleep on it… Here’s the current code if anyone wants to have a look:

#define MS5607_ADDR 0x76
#define RESET_REG 0x1E
#define PROM_BASE_REG 0xA0


uint16_t calData[6] = {0, 0, 0, 0, 0, 0};

uint32_t rawT;
uint32_t rawP;

int32_t deltaT;
int32_t Tc;
int32_t Tf;

int64_t offP;
int64_t sensP;
int64_t P;

float Temp;

void setup() {
    Wire.begin();
    Serial1.begin(115200);
    Serial1.println("MS5607 Library by @TimothyBrown");
    Serial1.println("Version: 20130323 - Build: 0xD0");
    for(int sensLoop = 0; sensLoop < 10; sensLoop++) {
        sensorStart();
        getData();
        Serial1.println(deltaT);
        Serial1.println(rawP);
        Serial1.print("Temperature: ");
        Serial1.println(Temp);
        Serial1.print("Temp C: ");
        Serial1.println(Tc);
        Serial1.print("Temp F: ");
        Serial1.println(Tf);
        Serial1.print("Pressure: ");
        char buf[100];
        sprintf(buf, "%lld", P);
        Serial1.println(buf);
        Serial1.print("Pressure: 0x");
        Serial1.print((int32_t)((P>>32)&0xFFFFFFFF),HEX);
        Serial1.println((int32_t)(P&0xFFFFFFFF),HEX);
        delay(1000);
    }
}

void loop() {

}

void sensorStart() {
    Wire.beginTransmission(MS5607_ADDR);
    Wire.write(RESET_REG);
    Wire.endTransmission();
    delay(5);
    uint8_t calReg = PROM_BASE_REG;
    for(uint8_t i = 0; i < 6; i++) {
        calReg = calReg + 0x02;
        Wire.beginTransmission(MS5607_ADDR);
        Wire.write(calReg);
        Wire.endTransmission();
        delay(1);
        Wire.requestFrom(MS5607_ADDR, 2);
        while(Wire.available()) {
            calData[i] = Wire.read();
            calData[i] = calData[i] << 8 | Wire.read();
            
        }
    Serial1.println(calData[i]);
    }
}

void getData() {
    
    /* Read Raw Temperature Data */
    Wire.beginTransmission(MS5607_ADDR);
    Wire.write(0x58);
    Wire.endTransmission();
    delay(10);
    Wire.beginTransmission(MS5607_ADDR);
    Wire.write(0x00);
    Wire.endTransmission();
    delay(1);
    Wire.requestFrom(MS5607_ADDR, 3);
    while(Wire.available()) {
        rawT = Wire.read();
        rawT = rawT << 8 | Wire.read();
        rawT = rawT << 8 | Wire.read();
    }
    
    /* Read Raw Pressure Data */
    Wire.beginTransmission(MS5607_ADDR);
    Wire.write(0x48);
    Wire.endTransmission();
    delay(10);
    Wire.beginTransmission(MS5607_ADDR);
    Wire.write(0x00);
    Wire.endTransmission();
    delay(1);
    Wire.requestFrom(MS5607_ADDR, 3);
    while(Wire.available()) {
        rawP = Wire.read();
        rawP = rawP << 8 | Wire.read();
        rawP = rawP << 8 | Wire.read();
    }
    
    /* Process Temperature */
    deltaT = rawT - ((uint32_t)calData[4] << 8);
    Tc = 2000 + ((deltaT * calData[5]) >> 23);
    Tf = Tc * 1.8 + 3200;
    
    
    /* Process Pressure */
    offP = ((uint32_t)calData[1] << 17) + ((calData[3] * deltaT) >> 6);
    sensP = ((uint32_t)calData[0] << 16) + ((calData[2] * deltaT) >> 7);
    P = ((((rawP * sensP) >> 21) - offP) >> 15);

    
}

I’m not sure if the current discussion is still the same as when the topic was started, but regarding trouble with long long, or 64 bit values; I’ve seen that gcc seems very eager to truncate values to 32 bits if it has the chance. The only way I could solve this was explicitly casting every value to 64 bits when doing arithmetic, like so:

i = (signed long long)((((signed long long)timestamp_us+(signed long long)delta))*((signed long long)ticks_per_s));

I’ve also seen some cross compilers messing up the divide when using long long, causing me to revert to calling a 64 bit division function explicitly:
uint64_t div64_64(uint64_t dividend, uint64_t divisor) or something similar.

Good luck.

Robin

If you make C1-C4, dT, OFF, and SENS all long long, these will work:

OFF = C2 * 2^17 + (C4 * dT) / 2^6 => 5,346,371,493.5
SENS = C1 * 2^16 + (C3 * dT) / 2^7 => 2,932,450,470.0156

You won't get any fractional parts, but you don't care about that. Alternatively, you can coerce each subterm, but that looks ugly.

The likely issue is that the C/C++ promotion rules aren't what people expect, although there could be bugs. For the given example, this would be sufficient (assuming no bugs):

// Assuming that i is long long:
i = (((long long)timestamp_us+delta))*ticks_per_s;

timb's equation is more complex, and would have to use something like (assuming that the variables retain their current 16- and 32-bit types):

OFF = (long long)C2 * 2^17 + ((long long)C4 * dT) / 2^6
2 Likes

I agree with @Raldus here but one more point: I have been burned by signed versus unsigned division by constants. You need to watch those unsigned divides closely.

With complicated expressions, this is also one of those times where I feel like writing out all the terms explicitly with temporary vars can be a help. You aren’t using much (any?) more stack typically since the cast has to happen anyway and you get clarity of the types of each term.

int64_t off_term1 = (int64_t)c2 * 131072LL;
int64_t off_term2 = (int64_t)c4 * dT / 64LL;
int64_t off = off_term1 + off_term2;

You can add a cast to dT as well or let the compiler do it for you–your choice.

1 Like

For cases like this, you only need one cast. Adding more doesn't hurt, but it can make the code hard-to-read. For example, all of these work the same:

int64_t off_term1 = c2 * 131072LL;    // easiest to read
int64_t off_term1 = (int64_t)c2 * 131072;
int64_t off_term1 = (int64_t)c2 * 131072LL;

EDIT: general nit: the above are all the same, since c2 is an unsigned 16-bit integer. If c2 happened to be an unsigned long long, they wouldn't be the same.

The reason is that, when the multiplication is done, both sides are promoted, if necessary, to be the larger datatype of the two. For example:

  • If one is 16-bit and the other is 32-bit, the result is 32-bit. (This is the case that is causing @timb problems.)

  • If both are 32-bit, the result is 32-bit.

  • If one is 16-bit and the other is 64-bit, the result is 64-bit.

  • If one is 32-bit and the other is 64-bit, the result is 64-bit.

Note that assigning the result into a 64-bit variable does not affect the above. This is the key point that trips up a lot of people; by the time the assignment is done, the computed value could already be incorrect.

Mixing signed and unsigned datatypes adds another messy dimension to the above. If you have a mix, everything gets converted to unsigned. This is a big problem if the signed datatype is negative (you end up with a positive number that is utterly not what you want).

Further complicating this, is the fact that datatype promotion occurs separately at each operation in an expression. In @timb's case:

calData[1] * 131072 + (calData[3] * deltaT) / 64;

When the above is evaluated, the compiler does:

calData[1] * 131072

Then:

calData[3] * deltaT

Then (using the result from the above):

(calData[3] * deltaT) / 64;

Then finally (using all of the above results):

calData[1] * 131072 + (calData[3] * deltaT) / 64;

You have to be careful about possible integer overflows in all of the above. You can't just do the following, because the second term ("calData[3] * deltaT") is still 32-bit.

(long long)calData[1] * 131072 + (calData[3] * deltaT) / 64;

You have to add another cast:

calData[1] * 131072LL + ((long long)calData[3] * deltaT) / 64;

Above, I moved the cast in the first term, to emphasize that the cast can be on either side. However, do not do this:

calData[1] * 131072LL + (calData[3] * deltaT) / 64LL;

That's because calData[3] * deltaT is still computed using 32-bits. By placing the cast with calData[3] * deltaT, you force that to be 64-bit, which cascades into another 64-bit result when you divide by 64.

1 Like

All fully understood! I think you are making my point for me that being very explicit and intentional at each step is better than depending on implicit rules, which even if you understand the rules perfectly, the next person to work on your code might not.

1 Like

I have mixed feelings about that. While you have a very valid and good point, adding lots of explicit casts can sometimes result in very weird and unexpected behavior, especially when you have a mix of signed and unsigned datatypes.

And characters can cause all sorts of weirdness, because they're a signed datatype:

// This is C -- spark can't handle this
#include <stdio.h>

int main()
{
  char c = 255;
  unsigned int i;
  int j;

  i = (unsigned int)c - 1;
  printf("i = %u\n", i);
  j = (int)c - 1;
  printf("j = %d\n", j);
  return 0;
}

If you compile and run the above, you get:

$ ./a.out
i = 4294967294
j = -2
1 Like

I agree with you that lots of explicit casts can be ugly too–there is no one size fits all solution. Where things will overflow if you don’t do the casts right, I prefer more explicit casting but I recognize that is just my preference.

@timb sounds like you’re still digging into this, but just wanted to throw out - please ping @zachary and/or me if you do discover there are any issues with the firmware that need to be fixed!

Bingo! The funny thing is, this was the first thing I tried when I was having issues, but it didn't work…because I wasn't updating the raw pressure variable (rawP) in the I2C read code…

Pro Tip: When you copy and paste blocks of code, make sure you double check that you changed all the variable names!

So yeah, this looks good. Thanks everyone!

1 Like

Hell yeah!

MS5607 Library by @TimothyBrown
Version: 20130328 - Build: 0xA0
Temperature: 72.75 F
Pressure: 30.32 Hg

According to the local weather map, the barometric pressure is 30.32 In.Hg; so this thing is dead on!

float MS5607::getPressure() {
  
    if(rawTemperature && rawPressure != 0) {
        int64_t dT = rawTemperature - (calData[4] << 8);
        int64_t OFF  = (calData[1] << 17) + ((dT * calData[3]) >> 6);
        int64_t SENS = (calData[0] << 16) + ((dT * calData[2]) >> 7);
        return ((((((rawPressure * SENS) >> 21) - OFF) >> 15) / 100.0) + 20) * 0.02953;
    }
    else {
        return -1;
    }
}

float MS5607::getTemperature() {

    if(rawTemperature && rawPressure != 0) {
        int64_t dT = rawTemperature - (calData[4] << 8);
        return (((2000 + ((dT * calData[5]) >> 23)) / 100.0) * 1.8) + 32;
    }
    else {
        return NULL;
    }
}

(The + 20 * 0.02953 portion of the getPressure() function adjusts for sea level (+20) and converts from millibar to inches of mercury (* 0.02953).

So yeah, I’m really pleased with this thing! It’s super accurate (the temperature matches my professionally calibrated thermal probe exactly); I guess the factory calibration and 24-bit resolution helps with that. Totally worth pulling my hair out over the calibration algorithms.

(These are Parallax breakout boards for the MS5607 Altimeter modules that I picked up last fall for $10 on clearance at Radio Shack.)

1 Like

Just to add, having precision like this is awesome!

Temperature: 72.43 F
Pressure: 30.30 Hg

Temperature: 72.45 F
Pressure: 30.30 Hg

Temperature: 72.46 F
Pressure: 30.30 Hg

Temperature: 72.50 F
Pressure: 30.30 Hg

Temperature: 72.52 F
Pressure: 30.30 Hg

This covers 25 seconds and starts when the gas fireplace (about 12ft away from me) kicked on. You can just see it getting progressively warmer and warmer.

I’ve now got a Spark.varible() for the pressure and temperature setup and have Spark Tools graphing it (5 minute update rate). It’ll be interesting to see a chart over a several day period, which will be amazing when I put this into my thermostat!

2 Likes

Sparkfun has breakout boards for the EPCOS T5400 barometric pressure/temperature sensors, for ~$15. While it's not the same as the MS5607, the use model is similar (e.g., download calibration coefficients and use in equations). Interestingly enough, the equations are vaguely similar, although they appear to have been reformulated to avoid 32-bit overflows:

(It's been tempting to play with one of these, too. I like the idea of a NetAtmo, but it's a bit pricey, and doing a DIY with one of these and a spark or raspberry pi is tempting. Using an air quality sensor would be fun, too, assuming it's accurate.)

1 Like

Let me in on your time machine secrets! :stuck_out_tongue: (2013)