* [PATCH] IIO: bmp280-core.c: fix error in humidity calculation @ 2017-03-27 10:06 Andreas Klinger 2017-04-02 9:32 ` Jonathan Cameron 0 siblings, 1 reply; 10+ messages in thread From: Andreas Klinger @ 2017-03-27 10:06 UTC (permalink / raw) To: jic23, knaack.h, lars, pmeerw, linus.walleij, ddvlad, akinobu.mita, yongjun_wei, a.mathur Cc: linux-iio, linux-kernel 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> --- drivers/iio/pressure/bmp280-core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 4d18826ac63c..73308f0ea260 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -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; }; -- 2.1.4 -- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-03-27 10:06 [PATCH] IIO: bmp280-core.c: fix error in humidity calculation Andreas Klinger @ 2017-04-02 9:32 ` Jonathan Cameron 2017-04-02 14:56 ` Linus Walleij 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Cameron @ 2017-04-02 9:32 UTC (permalink / raw) To: Andreas Klinger, knaack.h, lars, pmeerw, linus.walleij, ddvlad, akinobu.mita, yongjun_wei, a.mathur Cc: linux-iio, linux-kernel 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. Linus? J > --- > drivers/iio/pressure/bmp280-core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index 4d18826ac63c..73308f0ea260 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -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; > }; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-02 9:32 ` Jonathan Cameron @ 2017-04-02 14:56 ` Linus Walleij 2017-04-02 17:29 ` Andreas Klinger 0 siblings, 1 reply; 10+ messages in thread From: Linus Walleij @ 2017-04-02 14:56 UTC (permalink / raw) To: Jonathan Cameron, Matt Ranostay Cc: Andreas Klinger, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, ddvlad, Akinobu Mita, Wei Yongjun, Aniroop Mathur, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org 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 :) Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-02 14:56 ` Linus Walleij @ 2017-04-02 17:29 ` Andreas Klinger 2017-04-02 17:46 ` Jonathan Cameron 2017-04-04 6:16 ` Matt Ranostay 0 siblings, 2 replies; 10+ messages in thread From: Andreas Klinger @ 2017-04-02 17:29 UTC (permalink / raw) To: Linus Walleij Cc: Jonathan Cameron, Matt Ranostay, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, ddvlad, Akinobu Mita, Wei Yongjun, Aniroop Mathur, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org 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. Andreas > > Yours, > Linus Walleij -- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-02 17:29 ` Andreas Klinger @ 2017-04-02 17:46 ` Jonathan Cameron 2017-04-04 6:16 ` Matt Ranostay 1 sibling, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2017-04-02 17:46 UTC (permalink / raw) To: Andreas Klinger, Linus Walleij Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, ddvlad, Akinobu Mita, Wei Yongjun, Aniroop Mathur, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, matt.ranostay On 2 April 2017 18:29:49 BST, 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 :) He's now at Konsulko. Ccd > >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 > >> >> Yours, >> Linus Walleij -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-02 17:29 ` Andreas Klinger 2017-04-02 17:46 ` Jonathan Cameron @ 2017-04-04 6:16 ` Matt Ranostay 2017-04-07 6:13 ` Matt Ranostay 1 sibling, 1 reply; 10+ messages in thread From: Matt Ranostay @ 2017-04-04 6:16 UTC (permalink / raw) To: Andreas Klinger Cc: Linus Walleij, Jonathan Cameron, Matt Ranostay, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, ddvlad, Akinobu Mita, Wei Yongjun, Aniroop Mathur, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org 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... 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-04 6:16 ` Matt Ranostay @ 2017-04-07 6:13 ` Matt Ranostay 2017-04-07 19:27 ` Andreas Klinger 0 siblings, 1 reply; 10+ messages in thread From: Matt Ranostay @ 2017-04-07 6:13 UTC (permalink / raw) 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 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-07 6:13 ` Matt Ranostay @ 2017-04-07 19:27 ` Andreas Klinger 2017-04-08 21:34 ` Matt Ranostay 0 siblings, 1 reply; 10+ messages in thread From: Andreas Klinger @ 2017-04-07 19:27 UTC (permalink / raw) To: Matt Ranostay 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 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 <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 -- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-07 19:27 ` Andreas Klinger @ 2017-04-08 21:34 ` Matt Ranostay 2017-04-10 16:47 ` Andreas Klinger 0 siblings, 1 reply; 10+ messages in thread From: Matt Ranostay @ 2017-04-08 21:34 UTC (permalink / raw) 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 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. 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 > > -- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation 2017-04-08 21:34 ` Matt Ranostay @ 2017-04-10 16:47 ` Andreas Klinger 0 siblings, 0 replies; 10+ messages in thread From: Andreas Klinger @ 2017-04-10 16:47 UTC (permalink / raw) To: Matt Ranostay 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 > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-10 16:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-27 10:06 [PATCH] IIO: bmp280-core.c: fix error in humidity calculation Andreas Klinger 2017-04-02 9:32 ` Jonathan Cameron 2017-04-02 14:56 ` Linus Walleij 2017-04-02 17:29 ` Andreas Klinger 2017-04-02 17:46 ` Jonathan Cameron 2017-04-04 6:16 ` Matt Ranostay 2017-04-07 6:13 ` Matt Ranostay 2017-04-07 19:27 ` Andreas Klinger 2017-04-08 21:34 ` Matt Ranostay 2017-04-10 16:47 ` Andreas Klinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).