Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752644AbdDHVey (ORCPT ); Sat, 8 Apr 2017 17:34:54 -0400 Received: from mail-vk0-f47.google.com ([209.85.213.47]:33826 "EHLO mail-vk0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752002AbdDHVeu (ORCPT ); Sat, 8 Apr 2017 17:34:50 -0400 MIME-Version: 1.0 In-Reply-To: <20170407192727.GA4205@project> References: <20170327100646.GA8128@project> <2bec8c26-f33c-d331-aa97-047871b4cc57@kernel.org> <20170402172949.GA1479@project> <20170407192727.GA4205@project> From: Matt Ranostay Date: Sat, 8 Apr 2017 14:34:48 -0700 Message-ID: Subject: Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation To: Andreas Klinger Cc: Linus Walleij , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , Vlad Dogaru , Akinobu Mita , Wei Yongjun , Aniroop Mathur , "linux-iio@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5236 Lines: 160 On Fri, Apr 7, 2017 at 12:27 PM, Andreas Klinger 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 > #include > > 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 schrieb am Thu, 06. Apr 23:13: >> On Mon, Apr 3, 2017 at 11:16 PM, Matt Ranostay >> wrote: >> > On Sun, Apr 2, 2017 at 10:29 AM, Andreas Klinger wrote: >> >> Linus Walleij schrieb am Sun, 02. Apr 16:56: >> >>> On Sun, Apr 2, 2017 at 11:32 AM, Jonathan Cameron 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 >> >>> > >> >>> > 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 >> >>> >> >>> 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 > > --