IIO: bmp280-core.c: fix error in humidity calculation

PATCH Superseded Andreas Klinger 5 participants 10 messages Mar 27, 2017 – Apr 10, 2017 IIO

Share & tools

Andreas Klinger

author

2017-03-27 10:08 UTC #0 lore raw

PATCH 1/1 IIO: bmp280-core.c: fix error in humidity calculation

While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.

Signed-off-by Andreas Klinger <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iio/pressure/bmp280-core.c +3 -3

index 4d18826ac63c..73308f0ea260 100644
@@ -177,9 +177,9 @@ static u32 bmp280_compensate_humidity(struct bmp280_data *data,
 
 	var = ((s32)data->t_fine) - 76800;
 	var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var)) + 16384) >> 15)
-		* (((((((var * H6) >> 10) * (((var * H3) >> 11) + 32768)) >> 10)
-		+ 2097152) * H2 + 8192) >> 14);
-	var -= ((((var >> 15) * (var >> 15)) >> 7) * H1) >> 4;
+		* (((((((var * H6) >> 10) * (((var * (s32)H3) >> 11) + 32768))
+		>> 10) + 2097152) * H2 + 8192) >> 14);
+	var -= ((((var >> 15) * (var >> 15)) >> 7) * (s32)H1) >> 4;
 
 	return var >> 12;
 };

Signature · 4 lines

2.1.4


--

Jonathan Cameron

maintainer

2017-04-02 09:32 UTC #1 lore raw

Quoted 6 lines · Andreas Klinger

While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>

Looks superficially right to me, but would like an Ack from Linus Walleij.

Linus?

Quoted 22 lines

---
 drivers/iio/pressure/bmp280-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iio/pressure/bmp280-core.c +3 -3

index 4d18826ac63c..73308f0ea260 100644
@@ -177,9 +177,9 @@ static u32 bmp280_compensate_humidity(struct bmp280_data *data,
 
 	var = ((s32)data->t_fine) - 76800;
 	var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var)) + 16384) >> 15)
-		* (((((((var * H6) >> 10) * (((var * H3) >> 11) + 32768)) >> 10)
-		+ 2097152) * H2 + 8192) >> 14);
-	var -= ((((var >> 15) * (var >> 15)) >> 7) * H1) >> 4;
+		* (((((((var * H6) >> 10) * (((var * (s32)H3) >> 11) + 32768))
+		>> 10) + 2097152) * H2 + 8192) >> 14);
+	var -= ((((var >> 15) * (var >> 15)) >> 7) * (s32)H1) >> 4;
 
 	return var >> 12;
 };

Linus Walleij

Acked-by

2017-04-02 14:56 UTC #2 lore raw

Quoted 9 lines · Jonathan Cameron

Quoted 6 lines · Andreas Klinger
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Looks superficially right to me, but would like an Ack from Linus Walleij.

I didn't work on these calculations, only infrastructure for the driver but FWIW:

Acked-by Linus Walleij <[email protected]>

Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)

Yours,
Linus Walleij

Andreas Klinger

author

2017-04-02 17:30 UTC #3 lore raw

Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:

Quoted 9 lines · Jonathan Cameron

Quoted 8 lines · Andreas Klinger
Quoted 6 lines
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>

Quoted 8 lines

Quoted 1 line
Looks superficially right to me, but would like an Ack from Linus Walleij.
I didn't work on these calculations, only infrastructure for the driver but FWIW:
Acked-by Linus Walleij <[email protected]>
Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)

It just turned out that there must be another error in the calculation. I'll fix and test it first.

When i'm done i'll send a new version of the patch.

Andreas

Quoted 3 lines

Yours,
Linus Walleij

Jonathan Cameron

2017-04-02 17:46 UTC #4 lore raw

Quoted 16 lines · Andreas Klinger

Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:
Quoted 1 line
On Sun, Apr 2, 2017 at 11:32 AM, Jonathan Cameron <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines · Andreas Klinger
Quoted 1 line
While calculating the compensation of the humidity there are
negative
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the
documentation
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Looks superficially right to me, but would like an Ack from Linus
Walleij.
Quoted 1 line

Quoted 8 lines

Quoted 1 line
I didn't work on these calculations, only infrastructure for the
driver
Quoted 4 lines
but FWIW:
Acked-by Linus Walleij <[email protected]>
Matt Ranostay @Intel is the person you probably want to ACK it
though,
Quoted 1 line
he added the humidity calculations :)

He's now at Konsulko.
Ccd

Quoted 12 lines

It just turned out that there must be another error in the calculation. I'll fix
and test it first.
When i'm done i'll send a new version of the patch.
Andreas
Quoted 3 lines
Yours,
Linus Walleij

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Matt Ranostay

2017-04-04 06:16 UTC #5 lore raw

In reply to Andreas Klinger

Quoted 16 lines · Andreas Klinger

Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:
Quoted 15 lines · Jonathan Cameron
Quoted 9 lines · Andreas Klinger
Quoted 6 lines
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Looks superficially right to me, but would like an Ack from Linus Walleij.
I didn't work on these calculations, only infrastructure for the driver but FWIW:
Acked-by Linus Walleij <[email protected]>

Quoted 8 lines

Quoted 2 lines
Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)
It just turned out that there must be another error in the calculation. I'll fix and test it first.
When i'm done i'll send a new version of the patch.

Ok will hold off till review till the next patchset... BTW this datasheet's calculations made my eyes bleed so it is quiet likely it has bugs...

Thanks,

Matt

Quoted 11 lines

Andreas
Quoted 3 lines
Yours,
Linus Walleij
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Matt Ranostay

2017-04-07 06:13 UTC #6 lore raw

Quoted 23 lines · Matt Ranostay

Quoted 22 lines · Andreas Klinger
Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:
Quoted 9 lines · Jonathan Cameron
Quoted 8 lines · Andreas Klinger
Quoted 6 lines
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Quoted 8 lines
Quoted 1 line
Looks superficially right to me, but would like an Ack from Linus Walleij.
I didn't work on these calculations, only infrastructure for the driver but FWIW:
Acked-by Linus Walleij <[email protected]>
Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)
It just turned out that there must be another error in the calculation. I'll fix and test it first.

Quoted 6 lines

Quoted 2 lines
When i'm done i'll send a new version of the patch.
Ok will hold off till review till the next patchset... BTW this datasheet's calculations made my eyes bleed so it is quiet likely it has bugs...

Hmm the datatypes you are casting to s32 are already signed integers... Interested in what you found on what the actual bug is.

Quoted 16 lines

Thanks,
Matt
Quoted 11 lines
Andreas
Quoted 3 lines
Yours,
Linus Walleij
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Klinger

author

2017-04-07 19:27 UTC #7 lore raw

Hi Matt,

i've extracted the error condition in a small userspace application for demonstration. Just compile and see that in this case the variable H3 is zero but the whole term evaluates differently if treated as unsigned or signed.

The whole example including the values of the variables are taken from the real driver.

Andreas

Now the example:

---

/*
* humidity.c
* this test program is just for demonstrating the difference in the
* calculation of humidity compensation with BME280 sensor
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
*/

#include <stdio.h>
#include <stdlib.h>

int main(int argn, char* argv[])

{
	int adc_humidity = 28275;
	int t_fine = 50623;
	unsigned int H1 = 75, H3 = 0;
	int H2 = 360, H4 = 324, H5 = 0, H6 = 30;
	int var;

	var = -26177;
	/* extracted errornous term with cast */
	printf("with cast:    %d\n", (((var * H6) >> 10) * (((var * (int)H3) >> 11) 
							+ (int)32768)) >> 10);
	/* extracted errornous term now without a cast */
	printf("without cast: %d\n", (((var * H6) >> 10) * (((var * H3) >> 11) 
							+ 32768)) >> 10);

	printf("\n");
	printf("t_fine: %d; humidity: %d\nH: %d; %d; %d; %d; %d; %d\n", 
			t_fine, adc_humidity, H1, H2, H3, H4, H5, H6);
	printf("\n");

	/* the whole example taken from the driver */
	/* with the cast as proposed by the documentation */
	var = ((int)t_fine) - (int)76800;
	var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
		+ (int)16384) >> 15) * (((((((var * H6) >> 10)
		* (((var * (int)H3) >> 11) + (int)32768)) >> 10)
		+ (int)2097152) * H2 + 8192) >> 14);
	var -= ((((var >> 15) * (var >> 15)) >> 7) * (int)H1) >> 4;

	printf("with cast:    var: %d\n", var>>12);
	printf("\n");

	/* now the same calculation without the cast */
	var = (t_fine) - 76800;
	var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
		+ 16384) >> 15) * (((((((var * H6) >> 10)
		* (((var * H3) >> 11) + 32768)) >> 10)
		+ 2097152) * H2 + 8192) >> 14);
	var -= ((((var >> 15) * (var >> 15)) >> 7) * H1) >> 4;

	printf("without cast: var: %d\n", var>>12);

	return 0;
}

---

Output:

with cast:    -24544
without cast: 4169760

t_fine: 50623; humidity: 28275
H: 75; 360; 0; 324; 0; 30

with cast: var: 41671

without cast: var: 124497

---

Matt Ranostay <[email protected]> schrieb am Thu, 06. Apr 23:13:

Quoted 51 lines

Quoted 23 lines · Matt Ranostay
Quoted 22 lines · Andreas Klinger
Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:
Quoted 9 lines · Jonathan Cameron
Quoted 8 lines · Andreas Klinger
Quoted 6 lines
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Quoted 8 lines
Quoted 1 line
Looks superficially right to me, but would like an Ack from Linus Walleij.
I didn't work on these calculations, only infrastructure for the driver but FWIW:
Acked-by Linus Walleij <[email protected]>
Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)
It just turned out that there must be another error in the calculation. I'll fix and test it first.
Quoted 6 lines
Quoted 2 lines
When i'm done i'll send a new version of the patch.
Ok will hold off till review till the next patchset... BTW this datasheet's calculations made my eyes bleed so it is quiet likely it has bugs...
Hmm the datatypes you are casting to s32 are already signed integers... Interested in what you found on what the actual bug is.
Quoted 16 lines
Thanks,
Matt
Quoted 11 lines
Andreas
Quoted 3 lines
Yours,
Linus Walleij
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Matt Ranostay

2017-04-08 21:34 UTC #8 lore raw

Quoted 9 lines · Andreas Klinger

Hi Matt,
i've extracted the error condition in a small userspace application for demonstration. Just compile and see that in this case the variable H3 is zero but the whole term evaluates differently if treated as unsigned or signed.
The whole example including the values of the variables are taken from the real driver.

Ok now I see what you are referencing from the datasheet, and can confirm the values are incorrect.

Just a suggestion rather than have all those casts it would be easier to just change H1 and H3 types
to signed integers.

Thanks,

Matt

Quoted 137 lines

Andreas
Now the example:
---
/*
* humidity.c
* this test program is just for demonstrating the difference in the
* calculation of humidity compensation with BME280 sensor
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
*/
#include <stdio.h>
#include <stdlib.h>
int main(int argn, char* argv[])
{
        int adc_humidity = 28275;
        int t_fine = 50623;
        unsigned int H1 = 75, H3 = 0;
        int H2 = 360, H4 = 324, H5 = 0, H6 = 30;
        int var;

        var = -26177;
        /* extracted errornous term with cast */
        printf("with cast:    %d\n", (((var * H6) >> 10) * (((var * (int)H3) >> 11)
                                                        + (int)32768)) >> 10);
        /* extracted errornous term now without a cast */
        printf("without cast: %d\n", (((var * H6) >> 10) * (((var * H3) >> 11)
                                                        + 32768)) >> 10);

        printf("\n");
        printf("t_fine: %d; humidity: %d\nH: %d; %d; %d; %d; %d; %d\n",
                        t_fine, adc_humidity, H1, H2, H3, H4, H5, H6);
        printf("\n");

        /* the whole example taken from the driver */
        /* with the cast as proposed by the documentation */
        var = ((int)t_fine) - (int)76800;
        var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
                + (int)16384) >> 15) * (((((((var * H6) >> 10)
                * (((var * (int)H3) >> 11) + (int)32768)) >> 10)
                + (int)2097152) * H2 + 8192) >> 14);
        var -= ((((var >> 15) * (var >> 15)) >> 7) * (int)H1) >> 4;

        printf("with cast:    var: %d\n", var>>12);
        printf("\n");

        /* now the same calculation without the cast */
        var = (t_fine) - 76800;
        var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
                + 16384) >> 15) * (((((((var * H6) >> 10)
                * (((var * H3) >> 11) + 32768)) >> 10)
                + 2097152) * H2 + 8192) >> 14);
        var -= ((((var >> 15) * (var >> 15)) >> 7) * H1) >> 4;

        printf("without cast: var: %d\n", var>>12);

        return 0;
}
---
Output:
with cast:    -24544
without cast: 4169760
t_fine: 50623; humidity: 28275
H: 75; 360; 0; 324; 0; 30
with cast: var: 41671
without cast: var: 124497
---
Matt Ranostay <[email protected]> schrieb am Thu, 06. Apr 23:13:
Quoted 28 lines
Quoted 26 lines · Matt Ranostay
Quoted 24 lines · Andreas Klinger
Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:
Quoted 9 lines · Jonathan Cameron
Quoted 8 lines · Andreas Klinger
Quoted 6 lines
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Quoted 8 lines
Quoted 1 line
Looks superficially right to me, but would like an Ack from Linus Walleij.
I didn't work on these calculations, only infrastructure for the driver but FWIW:
Acked-by Linus Walleij <[email protected]>
Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)
It just turned out that there must be another error in the calculation. I'll fix and test it first.
When i'm done i'll send a new version of the patch.
Quoted 23 lines
Quoted 3 lines
Ok will hold off till review till the next patchset... BTW this datasheet's calculations made my eyes bleed so it is quiet likely it has bugs...
Hmm the datatypes you are casting to s32 are already signed integers... Interested in what you found on what the actual bug is.
Quoted 16 lines
Thanks,
Matt
Quoted 11 lines
Andreas
Quoted 3 lines
Yours,
Linus Walleij
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--

Andreas Klinger

author

2017-04-10 16:49 UTC #9 lore raw

Quoted 11 lines · Andreas Klinger

Quoted 9 lines
Hi Matt,
i've extracted the error condition in a small userspace application for demonstration. Just compile and see that in this case the variable H3 is zero but the whole term evaluates differently if treated as unsigned or signed.
The whole example including the values of the variables are taken from the real driver.

Quoted 7 lines

Ok now I see what you are referencing from the datasheet, and can confirm the values are incorrect.
Just a suggestion rather than have all those casts it would be easier to just change H1 and H3 types
to signed integers.

That's right. But i wanted to keep close to the documentation.

Quoted 146 lines

Thanks,
Matt
Quoted 130 lines
Andreas
Now the example:
---
/*
* humidity.c
* this test program is just for demonstrating the difference in the
* calculation of humidity compensation with BME280 sensor
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
*/
#include <stdio.h>
#include <stdlib.h>
int main(int argn, char* argv[])
{
        int adc_humidity = 28275;
        int t_fine = 50623;
        unsigned int H1 = 75, H3 = 0;
        int H2 = 360, H4 = 324, H5 = 0, H6 = 30;
        int var;

        var = -26177;
        /* extracted errornous term with cast */
        printf("with cast:    %d\n", (((var * H6) >> 10) * (((var * (int)H3) >> 11)
                                                        + (int)32768)) >> 10);
        /* extracted errornous term now without a cast */
        printf("without cast: %d\n", (((var * H6) >> 10) * (((var * H3) >> 11)
                                                        + 32768)) >> 10);

        printf("\n");
        printf("t_fine: %d; humidity: %d\nH: %d; %d; %d; %d; %d; %d\n",
                        t_fine, adc_humidity, H1, H2, H3, H4, H5, H6);
        printf("\n");

        /* the whole example taken from the driver */
        /* with the cast as proposed by the documentation */
        var = ((int)t_fine) - (int)76800;
        var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
                + (int)16384) >> 15) * (((((((var * H6) >> 10)
                * (((var * (int)H3) >> 11) + (int)32768)) >> 10)
                + (int)2097152) * H2 + 8192) >> 14);
        var -= ((((var >> 15) * (var >> 15)) >> 7) * (int)H1) >> 4;

        printf("with cast:    var: %d\n", var>>12);
        printf("\n");

        /* now the same calculation without the cast */
        var = (t_fine) - 76800;
        var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
                + 16384) >> 15) * (((((((var * H6) >> 10)
                * (((var * H3) >> 11) + 32768)) >> 10)
                + 2097152) * H2 + 8192) >> 14);
        var -= ((((var >> 15) * (var >> 15)) >> 7) * H1) >> 4;

        printf("without cast: var: %d\n", var>>12);

        return 0;
}
---
Output:
with cast:    -24544
without cast: 4169760
t_fine: 50623; humidity: 28275
H: 75; 360; 0; 324; 0; 30
with cast: var: 41671
without cast: var: 124497
---
Matt Ranostay <[email protected]> schrieb am Thu, 06. Apr 23:13:
Quoted 46 lines
Quoted 23 lines · Matt Ranostay
Quoted 22 lines · Andreas Klinger
Linus Walleij <[email protected]> schrieb am Sun, 02. Apr 16:56:
Quoted 9 lines · Jonathan Cameron
Quoted 8 lines · Andreas Klinger
Quoted 6 lines
While calculating the compensation of the humidity there are negative values interpreted as unsigned because of unsigned variables used. These values need to be casted to signed as indicated by the documentation of the sensor.
Signed-off-by Andreas Klinger <[email protected]>
Quoted 8 lines
Quoted 1 line
Looks superficially right to me, but would like an Ack from Linus Walleij.
I didn't work on these calculations, only infrastructure for the driver but FWIW:
Acked-by Linus Walleij <[email protected]>
Matt Ranostay @Intel is the person you probably want to ACK it though, he added the humidity calculations :)
It just turned out that there must be another error in the calculation. I'll fix and test it first.
Quoted 6 lines
Quoted 2 lines
When i'm done i'll send a new version of the patch.
Ok will hold off till review till the next patchset... BTW this datasheet's calculations made my eyes bleed so it is quiet likely it has bugs...
Hmm the datatypes you are casting to s32 are already signed integers... Interested in what you found on what the actual bug is.
Quoted 11 lines
Thanks,
Matt
Quoted 6 lines
Andreas
Quoted 3 lines
Yours,
Linus Walleij
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 7 lines

Andreas Klinger
Grabenreith 27
84508 Burgkirchen
+49 8623 919966
[email protected]
www.it-klinger.de
www.grabenreith.de