Date: Mon, 10 Apr 2017 18:47:50 +0200
From: Andreas Klinger <ak@it-klinger.de>
To: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
        Jonathan Cameron <jic23@kernel.org>, Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald <pmeerw@pmeerw.net>, Vlad Dogaru <ddvlad@gmail.com>,
        Akinobu Mita <akinobu.mita@gmail.com>,
        Wei Yongjun <yongjun_wei@trendmicro.com.cn>,
        Aniroop Mathur <a.mathur@samsung.com>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation
Message-ID: <20170410164750.GA1784@project>
References: <20170327100646.GA8128@project>
 <2bec8c26-f33c-d331-aa97-047871b4cc57@kernel.org>
 <CACRpkdb06PcUwC_zRThLXcw=haiDzuqP-gsVjchNnSWkootT0Q@mail.gmail.com>
 <20170402172949.GA1479@project>
 <CAJCx=g=E-4i=S+a0euYa8zJd=A2+zH8DgvngQPCF5vydzs+Cww@mail.gmail.com>
 <CAJCx=gnBZ3QbqftmuMOKuPKJ5+QBUjq41MireL4_8E0PM3LXiA@mail.gmail.com>
 <20170407192727.GA4205@project>
 <CAJCx=gkFU+PK+gROVRKGiNuXdjsBDFmBva0zqoHdQQ=YEy2xRw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJCx=gkFU+PK+gROVRKGiNuXdjsBDFmBva0zqoHdQQ=YEy2xRw@mail.gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5947
Lines: 176

> On Fri, Apr 7, 2017 at 12:27 PM, Andreas Klinger <ak@it-klinger.de> 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.
> 

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

> 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@konsulko.com> schrieb am Thu, 06. Apr 23:13:
> >> On Mon, Apr 3, 2017 at 11:16 PM, Matt Ranostay
> >> <matt.ranostay@konsulko.com> wrote:
> >> > On Sun, Apr 2, 2017 at 10:29 AM, Andreas Klinger <ak@it-klinger.de> wrote:
> >> >> Linus Walleij <linus.walleij@linaro.org> schrieb am Sun, 02. Apr 16:56:
> >> >>> On Sun, Apr 2, 2017 at 11:32 AM, Jonathan Cameron <jic23@kernel.org> 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@it-klinger.de>
> >> >>> >
> >> >>> > 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@linaro.org>
> >> >>>
> >> >>> 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@vger.kernel.org
> >> >> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Andreas Klinger
Grabenreith 27
84508 Burgkirchen
+49 8623 919966
ak@it-klinger.de
www.it-klinger.de
www.grabenreith.de