| Message ID | Zin2udkXRD0+GrML@adam-asahi.lan (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: pressure: bmp280: Fix BMP580 temperature reading | expand |
On Thu, 25 Apr 2024 01:22:49 -0500 Adam Rizkalla <ajarizzo@gmail.com> wrote: > Fix overflow issue when storing BMP580 temperature reading and > properly preserve sign of 24-bit data. > > Signed-off-by: Adam Rizkalla <ajarizzo@gmail.com> Hi Adam, Looks like a correct fix to me, but leaving on list a little longer for Angel to take a look. Thanks, Jonathan > --- > drivers/iio/pressure/bmp280-core.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index fe8734468ed3..e79c9715bb28 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -1393,12 +1393,12 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) > > /* > * Temperature is returned in Celsius degrees in fractional > - * form down 2^16. We rescale by x1000 to return milli Celsius > - * to respect IIO ABI. > + * form down 2^16. We rescale by x1000 to return millidegrees > + * Celsius to respect IIO ABI. > */ > - *val = raw_temp * 1000; > - *val2 = 16; > - return IIO_VAL_FRACTIONAL_LOG2; > + raw_temp = sign_extend32(raw_temp, 23); > + *val = ((s64)raw_temp * 1000) / (1 << 16); > + return IIO_VAL_INT; > } > > static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
Hi guys!
I am sending this e-mail as a reply to [1]
I know I am not included in the mailing list, but since I am working on that
driver currently [2] and I am also reading daily the iio mailing list I found
your patch quite interesting and I wanted to test it.
I managed to generate a negative temperature for the sensor and as it looks like
you are right. Before your patch, the return value was overflowing for negative
temperatures. So, good job!
I have one question though:
1) The comment that includes the "milli Celsius" is also similar in the
bmp380_read_temp() function so maybe you could change both. But since it is
a fixes patch, is that necessary to be done in this commit?
[1]: https://lore.kernel.org/linux-iio/Zin2udkXRD0+GrML@adam-asahi.lan/
[2]: https://lore.kernel.org/linux-iio/20240429190046.24252-1-vassilisamir@gmail.com/
P.S: I don't know how tags work, but if for the tested tag the testing that I
did is enough, then here is my tag:
Tested-By: Vasileios Amoiridis <vassilisamir@gmail.com>
On Thu, 2024-04-25 at 01:22 -0500, Adam Rizkalla wrote: > Fix overflow issue when storing BMP580 temperature reading and > properly preserve sign of 24-bit data. > > Signed-off-by: Adam Rizkalla <ajarizzo@gmail.com> > --- > drivers/iio/pressure/bmp280-core.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280- > core.c > index fe8734468ed3..e79c9715bb28 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -1393,12 +1393,12 @@ static int bmp580_read_temp(struct bmp280_data *data, > int *val, int *val2) > > /* > * Temperature is returned in Celsius degrees in fractional > - * form down 2^16. We rescale by x1000 to return milli Celsius > - * to respect IIO ABI. > + * form down 2^16. We rescale by x1000 to return millidegrees > + * Celsius to respect IIO ABI. > */ > - *val = raw_temp * 1000; > - *val2 = 16; > - return IIO_VAL_FRACTIONAL_LOG2; > + raw_temp = sign_extend32(raw_temp, 23); > + *val = ((s64)raw_temp * 1000) / (1 << 16); > + return IIO_VAL_INT; > } > > static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2) Hi Adam, Great catch! Reading back the device's datasheet, it also says the raw pressure value it's a 24 bit signed integer, so we have the same problem on bmp580_read_press. Could you make a v2 version of this series including the pressure fix? Kind regards, Angel
Hi Angel! Indeed the datasheet says that the pressure is a signed value. But this comes in contrast with 2 things: 1) The BMP58x sensor does not have a compensation function so the value from the sensor is a pressure value just in different unit from the one reported by IIO. And the sensor is able to report in between 30-125kPa according to the datasheet which are both positive values so it makes more sense to be an unsigned value. 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is declared as an unsigned value. So, what should we trust? [1]: https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895
On Thu, May 02, 2024 at 07:16:16PM +0200, Vasileios Amoiridis wrote: > Hi Angel! > > Indeed the datasheet says that the pressure is a signed value. But this comes > in contrast with 2 things: > > 1) The BMP58x sensor does not have a compensation function so the value from the > sensor is a pressure value just in different unit from the one reported by IIO. > And the sensor is able to report in between 30-125kPa according to the > datasheet which are both positive values so it makes more sense to be an > unsigned value. > > 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is > declared as an unsigned value. > > So, what should we trust? > > [1]: https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895 The pressure sensor reading cannot be negative, as the pressure range of the sensor is 300 - 1250 hPa,so this change does not need to be applied for bmp580_read_press(). Also, the overflow issue does not happen with the pressure reading since the value read back from the device is scaled up only by 2^6 for pressure vs 2^16 for temperature, so multiplying by 1000 even for the maximum value would still fit in a 32-bit signed integer. Temperature ranges above ~32.767C, however, will overflow a 32-bit signed integer when multiplied by 2^16 * 1000 which is why this change is necessary only for temperature readings. Hope this helps clarify. Best, Adam
On Thu, 2024-05-02 at 13:15 -0500, Adam Rizkalla wrote: > On Thu, May 02, 2024 at 07:16:16PM +0200, Vasileios Amoiridis wrote: > > Hi Angel! > > > > Indeed the datasheet says that the pressure is a signed value. But this > > comes > > in contrast with 2 things: > > > > 1) The BMP58x sensor does not have a compensation function so the value from > > the > > sensor is a pressure value just in different unit from the one reported by > > IIO. > > And the sensor is able to report in between 30-125kPa according to the > > datasheet which are both positive values so it makes more sense to be an > > unsigned value. > > > > 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is > > declared as an unsigned value. > > > > So, what should we trust? > > > > [1]: > > https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895 > > The pressure sensor reading cannot be negative, as the pressure range of the > sensor > is 300 - 1250 hPa,so this change does not need to be applied for > bmp580_read_press(). > Also, the overflow issue does not happen with the pressure reading since the > value > read back from the device is scaled up only by 2^6 for pressure vs 2^16 for > temperature, > so multiplying by 1000 even for the maximum value would still fit in a 32-bit > signed > integer. Temperature ranges above ~32.767C, however, will overflow a 32-bit > signed > integer when multiplied by 2^16 * 1000 which is why this change is necessary > only for > temperature readings. > > Hope this helps clarify. > > Best, > Adam Crystal clear. Thanks to both of you for the clarifications :) Acked-by: Angel Iglesias <ang.iglesiasg@gmail.com> Kind regards, Angel
On Fri, 03 May 2024 09:13:33 +0200 Angel Iglesias <ang.iglesiasg@gmail.com> wrote: > On Thu, 2024-05-02 at 13:15 -0500, Adam Rizkalla wrote: > > On Thu, May 02, 2024 at 07:16:16PM +0200, Vasileios Amoiridis wrote: > > > Hi Angel! > > > > > > Indeed the datasheet says that the pressure is a signed value. But this > > > comes > > > in contrast with 2 things: > > > > > > 1) The BMP58x sensor does not have a compensation function so the value from > > > the > > > sensor is a pressure value just in different unit from the one reported by > > > IIO. > > > And the sensor is able to report in between 30-125kPa according to the > > > datasheet which are both positive values so it makes more sense to be an > > > unsigned value. > > > > > > 2) According to the BMP5 sensor API [1] provided by Bosch, the pressure is > > > declared as an unsigned value. > > > > > > So, what should we trust? > > > > > > [1]: > > > https://github.com/boschsensortec/BMP5_SensorAPI/blob/master/bmp5_defs.h#L895 > > > > The pressure sensor reading cannot be negative, as the pressure range of the > > sensor > > is 300 - 1250 hPa,so this change does not need to be applied for > > bmp580_read_press(). > > Also, the overflow issue does not happen with the pressure reading since the > > value > > read back from the device is scaled up only by 2^6 for pressure vs 2^16 for > > temperature, > > so multiplying by 1000 even for the maximum value would still fit in a 32-bit > > signed > > integer. Temperature ranges above ~32.767C, however, will overflow a 32-bit > > signed > > integer when multiplied by 2^16 * 1000 which is why this change is necessary > > only for > > temperature readings. > > > > Hope this helps clarify. > > > > Best, > > Adam > > Crystal clear. Thanks to both of you for the clarifications :) > > Acked-by: Angel Iglesias <ang.iglesiasg@gmail.com> Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > > Kind regards, > Angel
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index fe8734468ed3..e79c9715bb28 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -1393,12 +1393,12 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2) /* * Temperature is returned in Celsius degrees in fractional - * form down 2^16. We rescale by x1000 to return milli Celsius - * to respect IIO ABI. + * form down 2^16. We rescale by x1000 to return millidegrees + * Celsius to respect IIO ABI. */ - *val = raw_temp * 1000; - *val2 = 16; - return IIO_VAL_FRACTIONAL_LOG2; + raw_temp = sign_extend32(raw_temp, 23); + *val = ((s64)raw_temp * 1000) / (1 << 16); + return IIO_VAL_INT; } static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
Fix overflow issue when storing BMP580 temperature reading and properly preserve sign of 24-bit data. Signed-off-by: Adam Rizkalla <ajarizzo@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)