Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation

[Matt Ranostay]

On Fri, Apr 7, 2017 at 12:27 PM, Andreas Klinger <ak@xxxxxxxxxxxxx> wrote:
> 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


> 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 <matt.ranostay@xxxxxxxxxxxx> schrieb am Thu, 06. Apr 23:13:
>> On Mon, Apr 3, 2017 at 11:16 PM, Matt Ranostay
>> <matt.ranostay@xxxxxxxxxxxx> wrote:
>> > On Sun, Apr 2, 2017 at 10:29 AM, Andreas Klinger <ak@xxxxxxxxxxxxx> wrote:
>> >> Linus Walleij <linus.walleij@xxxxxxxxxx> schrieb am Sun, 02. Apr 16:56:
>> >>> On Sun, Apr 2, 2017 at 11:32 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> >>> > On 27/03/17 11:06, Andreas Klinger wrote:
>> >>> >> 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 <ak@xxxxxxxxxxxxx>
>> >>> >
>> >>> > 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 <linus.walleij@xxxxxxxxxx>
>> >>>
>> >>> 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...
>>
>> Hmm the datatypes you are casting to s32 are already signed
>> integers...  Interested in what you found on what the actual bug is.
>>
>> >
>> > Thanks,
>> >
>> > Matt
>> >
>> >> Andreas
>> >>
>> >>>
>> >>> Yours,
>> >>> Linus Walleij
>> >>
>> >> --
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --