The read_press/read_humid functions need the updated t_fine value
in order to calculate the current pressure/humidity. Temperature
reads should be removed from the read_press/read_humid functions
and should be placed in the oneshot captures before the pressure
and humidity reads. This makes the code more intuitive.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6d7734f867bc..377e90d9e5a2 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
s32 adc_press;
int ret;
- /* Read and compensate temperature so we get a reading of t_fine. */
- ret = bmp280_read_temp(data);
- if (ret < 0)
- return ret;
-
ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
data->buf, sizeof(data->buf));
if (ret < 0) {
@@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
s32 adc_humidity;
int ret;
- /* Read and compensate temperature so we get a reading of t_fine. */
- ret = bmp280_read_temp(data);
- if (ret < 0)
- return ret;
-
ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
&data->be16, sizeof(data->be16));
if (ret < 0) {
@@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_HUMIDITYRELATIVE:
+ /* Read temperature to update the t_fine value */
+ data->chip_info->read_temp(data);
ret = data->chip_info->read_humid(data);
*val = data->chip_info->humid_coeffs[0] * ret;
*val2 = data->chip_info->humid_coeffs[1];
ret = IIO_VAL_FRACTIONAL;
break;
case IIO_PRESSURE:
+ /*
+ * Read temperature to update the t_fine value.
+ * BMP5xx devices do this in hardware, so skip it.
+ */
+ if (strcmp(indio_dev->name, "bmp580"))
+ data->chip_info->read_temp(data);
+
ret = data->chip_info->read_press(data);
*val = data->chip_info->press_coeffs[0] * ret;
*val2 = data->chip_info->press_coeffs[1];
@@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_RAW:
switch (chan->type) {
case IIO_HUMIDITYRELATIVE:
+ /* Read temperature to update the t_fine value */
+ data->chip_info->read_temp(data);
*val = data->chip_info->read_humid(data);
ret = IIO_VAL_INT;
break;
case IIO_PRESSURE:
+ /*
+ * Read temperature to update the t_fine value.
+ * BMP5xx devices do this in hardware, so skip it.
+ */
+ if (strcmp(indio_dev->name, "bmp580"))
+ data->chip_info->read_temp(data);
+
*val = data->chip_info->read_press(data);
ret = IIO_VAL_INT;
break;
@@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
s32 adc_press;
int ret;
- /* Read and compensate for temperature so we get a reading of t_fine */
- ret = bmp380_read_temp(data);
- if (ret)
- return ret;
-
ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
data->buf, sizeof(data->buf));
if (ret) {
@@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
s32 adc_press;
int ret;
- /* Read and compensate temperature so we get a reading of t_fine. */
- ret = bmp180_read_temp(data);
- if (ret)
- return ret;
-
ret = bmp180_read_adc_press(data, &adc_press);
if (ret)
return ret;
--
2.25.1
On Wed, Mar 13, 2024 at 06:40:05PM +0100, Vasileios Amoiridis wrote:
> The read_press/read_humid functions need the updated t_fine value
read_press()/read_humid()
> in order to calculate the current pressure/humidity. Temperature
> reads should be removed from the read_press/read_humid functions
read_press()/read_humid()
> and should be placed in the oneshot captures before the pressure
> and humidity reads. This makes the code more intuitive.
..
> + if (strcmp(indio_dev->name, "bmp580"))
> + data->chip_info->read_temp(data);
> +
> + if (strcmp(indio_dev->name, "bmp580"))
> + data->chip_info->read_temp(data);
Yeah, not a fan of the strcmp().
--
With Best Regards,
Andy Shevchenko
On Wed, 13 Mar 2024 21:04:46 +0200
Andy Shevchenko <[email protected]> wrote:
> On Wed, Mar 13, 2024 at 06:40:05PM +0100, Vasileios Amoiridis wrote:
> > The read_press/read_humid functions need the updated t_fine value
>
> read_press()/read_humid()
>
>
> > in order to calculate the current pressure/humidity. Temperature
> > reads should be removed from the read_press/read_humid functions
>
> read_press()/read_humid()
>
> > and should be placed in the oneshot captures before the pressure
> > and humidity reads. This makes the code more intuitive.
>
> ...
>
> > + if (strcmp(indio_dev->name, "bmp580"))
> > + data->chip_info->read_temp(data);
> > +
>
> > + if (strcmp(indio_dev->name, "bmp580"))
> > + data->chip_info->read_temp(data);
>
> Yeah, not a fan of the strcmp().
>
Yes - that's a non starter. Add a flag to say it is necessary to
chip_info that doesn't rely on name matching. If you do it the way
you have here, you have to add another strcmp for each new device
supported that needs this code to run. Add a flag and you just
set that in the chip_info structure. Much more flexible and extensible.
Usual description of this is "when you can do things with data rather than
code, do it with data".
Jonathan
On Wed, 13 Mar 2024 18:40:05 +0100
Vasileios Amoiridis <[email protected]> wrote:
> The read_press/read_humid functions need the updated t_fine value
> in order to calculate the current pressure/humidity. Temperature
> reads should be removed from the read_press/read_humid functions
> and should be placed in the oneshot captures before the pressure
> and humidity reads. This makes the code more intuitive.
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
To me this makes the use of these calls less obvious than they were
previously. The calls are made close to where t_fine is used and
don't have to go via the indirection of chip_info.
So I disagree. I think this change makes the code a lot less
clear.
The only improvement I can readily see would be to move the
temperature read into the compensation functions themselves, possibly
removing t_fine from data and having a function that reads everything
relevant to computing it directly but doesn't do the maths to get
a temperature reading. That can be reused in bmp280_compensate_temp()
Something along lines of.
static s32 bmp280_calc_tfine(struct bmp280_calib *calib, s32 adc_temp)
{
s32 var1, var2;
var1 = (((adc_temp >> 3) - ((s32)calib->T1 << 1)) *
((s32)calib->T2)) >> 11;
var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) *
((adc_temp >> 4) - ((s32)calib->T1))) >> 12) *
((s32)calib->T3)) >> 14;
return var1 + var2;
}
static int bmp280_read_temp_raw(struct bmp280_data *data,
s32 *raw)
{
s32 adc_temp;
int ret;
ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
data->buf, sizeof(data->buf));
if (ret < 0) {
dev_err(data->dev, "failed to read temperature\n");
return ret;
}
adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf));
if (adc_temp == BMP280_TEMP_SKIPPED) {
/* reading was skipped */
dev_err(data->dev, "reading temperature skipped\n");
return -EIO;
}
*raw = adc_temp;
return 0;
}
static int bmp280_get_t_fine(.., s32 *t_fine)
{
s32 adc_temp, comp_temp;
s32 t_fine;
int ret;
ret = bmp280_read_temp_raw(data, &adc_temp;
if (ret)
return ret;
*t_fine = bmp280_calc_tfine(&data->calib.bmp280, adc_temp);
return 0;
}
static int bmp280_read_temp(struct bmp280_data *data, s32 *temp)
{
int ret;
s32 t_fine;
ret = bmp280_get_t_fine(data, &t_fine);
if (ret)
return ret;
*temp = (t_fine * 5 + 128) / 256;
//division rather than shift as then it's obvious what the 128 is there for
return 0;
}
Now you have a nice function to get you t_fine which is all you want in some
of these paths. Call it directly where it is needed instead of as
a side effect of a temperature read.
> ---
> drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 6d7734f867bc..377e90d9e5a2 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
> s32 adc_press;
> int ret;
>
> - /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp280_read_temp(data);
> - if (ret < 0)
> - return ret;
> -
> ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> data->buf, sizeof(data->buf));
> if (ret < 0) {
> @@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
> s32 adc_humidity;
> int ret;
>
> - /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp280_read_temp(data);
> - if (ret < 0)
> - return ret;
> -
> ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> &data->be16, sizeof(data->be16));
> if (ret < 0) {
> @@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
> + /* Read temperature to update the t_fine value */
> + data->chip_info->read_temp(data);
> ret = data->chip_info->read_humid(data);
> *val = data->chip_info->humid_coeffs[0] * ret;
> *val2 = data->chip_info->humid_coeffs[1];
> ret = IIO_VAL_FRACTIONAL;
> break;
> case IIO_PRESSURE:
> + /*
> + * Read temperature to update the t_fine value.
> + * BMP5xx devices do this in hardware, so skip it.
> + */
> + if (strcmp(indio_dev->name, "bmp580"))
> + data->chip_info->read_temp(data);
> +
> ret = data->chip_info->read_press(data);
> *val = data->chip_info->press_coeffs[0] * ret;
> *val2 = data->chip_info->press_coeffs[1];
> @@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_RAW:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
> + /* Read temperature to update the t_fine value */
> + data->chip_info->read_temp(data);
> *val = data->chip_info->read_humid(data);
> ret = IIO_VAL_INT;
> break;
> case IIO_PRESSURE:
> + /*
> + * Read temperature to update the t_fine value.
> + * BMP5xx devices do this in hardware, so skip it.
> + */
> + if (strcmp(indio_dev->name, "bmp580"))
> + data->chip_info->read_temp(data);
> +
> *val = data->chip_info->read_press(data);
> ret = IIO_VAL_INT;
> break;
> @@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
> s32 adc_press;
> int ret;
>
> - /* Read and compensate for temperature so we get a reading of t_fine */
> - ret = bmp380_read_temp(data);
> - if (ret)
> - return ret;
> -
> ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> data->buf, sizeof(data->buf));
> if (ret) {
> @@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
> s32 adc_press;
> int ret;
>
> - /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp180_read_temp(data);
> - if (ret)
> - return ret;
> -
> ret = bmp180_read_adc_press(data, &adc_press);
> if (ret)
> return ret;
On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 18:40:05 +0100
> Vasileios Amoiridis <[email protected]> wrote:
>
> > The read_press/read_humid functions need the updated t_fine value
> > in order to calculate the current pressure/humidity. Temperature
> > reads should be removed from the read_press/read_humid functions
> > and should be placed in the oneshot captures before the pressure
> > and humidity reads. This makes the code more intuitive.
> >
> > Signed-off-by: Vasileios Amoiridis <[email protected]>
>
> To me this makes the use of these calls less obvious than they were
> previously. The calls are made close to where t_fine is used and
> don't have to go via the indirection of chip_info.
>
> So I disagree. I think this change makes the code a lot less
> clear.
>
This was mainly driven by the fact that I wanted to avoid reading
the temperature 3 times in case temp, press and humid are enabled
and there are consecutive buffer readings. But thank you for the
proposal I really appreciate it!
Best regards,
Vasilis
> The only improvement I can readily see would be to move the
> temperature read into the compensation functions themselves, possibly
> removing t_fine from data and having a function that reads everything
> relevant to computing it directly but doesn't do the maths to get
> a temperature reading. That can be reused in bmp280_compensate_temp()
>
> Something along lines of.
>
> static s32 bmp280_calc_tfine(struct bmp280_calib *calib, s32 adc_temp)
> {
> s32 var1, var2;
>
> var1 = (((adc_temp >> 3) - ((s32)calib->T1 << 1)) *
> ((s32)calib->T2)) >> 11;
> var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) *
> ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) *
> ((s32)calib->T3)) >> 14;
> return var1 + var2;
> }
>
> static int bmp280_read_temp_raw(struct bmp280_data *data,
> s32 *raw)
> {
> s32 adc_temp;
> int ret;
>
> ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> data->buf, sizeof(data->buf));
> if (ret < 0) {
> dev_err(data->dev, "failed to read temperature\n");
> return ret;
> }
>
> adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf));
> if (adc_temp == BMP280_TEMP_SKIPPED) {
> /* reading was skipped */
> dev_err(data->dev, "reading temperature skipped\n");
> return -EIO;
> }
> *raw = adc_temp;
>
> return 0;
> }
> static int bmp280_get_t_fine(.., s32 *t_fine)
> {
> s32 adc_temp, comp_temp;
> s32 t_fine;
> int ret;
>
> ret = bmp280_read_temp_raw(data, &adc_temp;
> if (ret)
> return ret;
>
> *t_fine = bmp280_calc_tfine(&data->calib.bmp280, adc_temp);
> return 0;
> }
>
> static int bmp280_read_temp(struct bmp280_data *data, s32 *temp)
> {
> int ret;
> s32 t_fine;
>
> ret = bmp280_get_t_fine(data, &t_fine);
> if (ret)
> return ret;
>
> *temp = (t_fine * 5 + 128) / 256;
> //division rather than shift as then it's obvious what the 128 is there for
> return 0;
> }
>
> Now you have a nice function to get you t_fine which is all you want in some
> of these paths. Call it directly where it is needed instead of as
> a side effect of a temperature read.
>
>
>
> > ---
> > drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
> > 1 file changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 6d7734f867bc..377e90d9e5a2 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
> > s32 adc_press;
> > int ret;
> >
> > - /* Read and compensate temperature so we get a reading of t_fine. */
> > - ret = bmp280_read_temp(data);
> > - if (ret < 0)
> > - return ret;
> > -
> > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> > data->buf, sizeof(data->buf));
> > if (ret < 0) {
> > @@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
> > s32 adc_humidity;
> > int ret;
> >
> > - /* Read and compensate temperature so we get a reading of t_fine. */
> > - ret = bmp280_read_temp(data);
> > - if (ret < 0)
> > - return ret;
> > -
> > ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> > &data->be16, sizeof(data->be16));
> > if (ret < 0) {
> > @@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_PROCESSED:
> > switch (chan->type) {
> > case IIO_HUMIDITYRELATIVE:
> > + /* Read temperature to update the t_fine value */
> > + data->chip_info->read_temp(data);
> > ret = data->chip_info->read_humid(data);
> > *val = data->chip_info->humid_coeffs[0] * ret;
> > *val2 = data->chip_info->humid_coeffs[1];
> > ret = IIO_VAL_FRACTIONAL;
> > break;
> > case IIO_PRESSURE:
> > + /*
> > + * Read temperature to update the t_fine value.
> > + * BMP5xx devices do this in hardware, so skip it.
> > + */
> > + if (strcmp(indio_dev->name, "bmp580"))
> > + data->chip_info->read_temp(data);
> > +
> > ret = data->chip_info->read_press(data);
> > *val = data->chip_info->press_coeffs[0] * ret;
> > *val2 = data->chip_info->press_coeffs[1];
> > @@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_RAW:
> > switch (chan->type) {
> > case IIO_HUMIDITYRELATIVE:
> > + /* Read temperature to update the t_fine value */
> > + data->chip_info->read_temp(data);
> > *val = data->chip_info->read_humid(data);
> > ret = IIO_VAL_INT;
> > break;
> > case IIO_PRESSURE:
> > + /*
> > + * Read temperature to update the t_fine value.
> > + * BMP5xx devices do this in hardware, so skip it.
> > + */
> > + if (strcmp(indio_dev->name, "bmp580"))
> > + data->chip_info->read_temp(data);
> > +
> > *val = data->chip_info->read_press(data);
> > ret = IIO_VAL_INT;
> > break;
> > @@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
> > s32 adc_press;
> > int ret;
> >
> > - /* Read and compensate for temperature so we get a reading of t_fine */
> > - ret = bmp380_read_temp(data);
> > - if (ret)
> > - return ret;
> > -
> > ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> > data->buf, sizeof(data->buf));
> > if (ret) {
> > @@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
> > s32 adc_press;
> > int ret;
> >
> > - /* Read and compensate temperature so we get a reading of t_fine. */
> > - ret = bmp180_read_temp(data);
> > - if (ret)
> > - return ret;
> > -
> > ret = bmp180_read_adc_press(data, &adc_press);
> > if (ret)
> > return ret;
>
On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > On Wed, 13 Mar 2024 18:40:05 +0100
> > Vasileios Amoiridis <[email protected]> wrote:
> >
> > > The read_press/read_humid functions need the updated t_fine value
> > > in order to calculate the current pressure/humidity. Temperature
> > > reads should be removed from the read_press/read_humid functions
> > > and should be placed in the oneshot captures before the pressure
> > > and humidity reads. This makes the code more intuitive.
> > >
> > > Signed-off-by: Vasileios Amoiridis <[email protected]>
> >
> > To me this makes the use of these calls less obvious than they were
> > previously. The calls are made close to where t_fine is used and
> > don't have to go via the indirection of chip_info.
> >
> > So I disagree. I think this change makes the code a lot less
> > clear.
> >
>
> This was mainly driven by the fact that I wanted to avoid reading
> the temperature 3 times in case temp, press and humid are enabled
> and there are consecutive buffer readings. But thank you for the
> proposal I really appreciate it!
>
Hi, just a side note reflecting on this. Depending on your sampling frequency
and registers data shadowing, to avoid compensating with different samples
between readings, we should be doing burst readings to get a bundle of the
temperature+pressure and/or humidity.
On the bmp/bme280 and bmp380 this can be done as registers are contiguous on the
memory. On the bmp580 this is not a problem as the values are already
compensated, you`ll get always the latest reading.
Kind regard,
Angel
On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:
> On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > Vasileios Amoiridis <[email protected]> wrote:
> > >
> > > > The read_press/read_humid functions need the updated t_fine value
> > > > in order to calculate the current pressure/humidity. Temperature
> > > > reads should be removed from the read_press/read_humid functions
> > > > and should be placed in the oneshot captures before the pressure
> > > > and humidity reads. This makes the code more intuitive.
> > > >
> > > > Signed-off-by: Vasileios Amoiridis <[email protected]>
> > >
> > > To me this makes the use of these calls less obvious than they were
> > > previously. The calls are made close to where t_fine is used and
> > > don't have to go via the indirection of chip_info.
> > >
> > > So I disagree. I think this change makes the code a lot less
> > > clear.
> > >
> >
> > This was mainly driven by the fact that I wanted to avoid reading
> > the temperature 3 times in case temp, press and humid are enabled
> > and there are consecutive buffer readings. But thank you for the
> > proposal I really appreciate it!
> >
>
> Hi, just a side note reflecting on this. Depending on your sampling frequency
> and registers data shadowing, to avoid compensating with different samples
> between readings, we should be doing burst readings to get a bundle of the
> temperature+pressure and/or humidity.
> On the bmp/bme280 and bmp380 this can be done as registers are contiguous on the
> memory. On the bmp580 this is not a problem as the values are already
> compensated, you`ll get always the latest reading.
>
> Kind regard,
> Angel
Hi Angel,
Thank you for pointing this out! Indeed that's true but I noticed that this is not
the case for the BMP{085/180} devices. I just feel that some changes might make
data acquisition/processing faster for a device (like the one you proposed) but
it might make the code much more unreadable and unmaintanable. I will try and
see if something could be done in that sense but I feel that keeping it simple will
be good for everyone!
Cheers,
Vasilis
On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote:
> On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:
> > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> > > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > > Vasileios Amoiridis <[email protected]> wrote:
> > > >
> > > > > The read_press/read_humid functions need the updated t_fine value
> > > > > in order to calculate the current pressure/humidity. Temperature
> > > > > reads should be removed from the read_press/read_humid functions
> > > > > and should be placed in the oneshot captures before the pressure
> > > > > and humidity reads. This makes the code more intuitive.
> > > > >
> > > > > Signed-off-by: Vasileios Amoiridis <[email protected]>
> > > >
> > > > To me this makes the use of these calls less obvious than they were
> > > > previously. The calls are made close to where t_fine is used and
> > > > don't have to go via the indirection of chip_info.
> > > >
> > > > So I disagree. I think this change makes the code a lot less
> > > > clear.
> > > >
> > >
> > > This was mainly driven by the fact that I wanted to avoid reading
> > > the temperature 3 times in case temp, press and humid are enabled
> > > and there are consecutive buffer readings. But thank you for the
> > > proposal I really appreciate it!
> > >
> >
> > Hi, just a side note reflecting on this. Depending on your sampling
> > frequency
> > and registers data shadowing, to avoid compensating with different samples
> > between readings, we should be doing burst readings to get a bundle of the
> > temperature+pressure and/or humidity.
> > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on
> > the
> > memory. On the bmp580 this is not a problem as the values are already
> > compensated, you`ll get always the latest reading.
> >
> > Kind regard,
> > Angel
>
> Hi Angel,
>
> Thank you for pointing this out! Indeed that's true but I noticed that this is
> not
> the case for the BMP{085/180} devices. I just feel that some changes might
> make
> data acquisition/processing faster for a device (like the one you proposed)
> but
> it might make the code much more unreadable and unmaintanable. I will try and
> see if something could be done in that sense but I feel that keeping it simple
> will
> be good for everyone!
>
> Cheers,
> Vasilis
Yeah, data adquisition on bmp085/180 is already different as they don't support
continuous mode as the newer models and you have to warm-up the sensor and do
one-shot readings. There's already a different code path in place for that
models. I guess that is the price to pay to support that wide range of
sensors...
Anyway, this patches are already big and you're doing quite a lot of heavy-
lifting right now, so don't pay much attention to my ramblings! Regardless,
happy to help with tasks polishing and updating this driver :)
Kind regards,
Angel
On Fri, 15 Mar 2024 14:28:30 +0100
Angel Iglesias <[email protected]> wrote:
> On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote:
> > On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:
> > > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> > > > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > > > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > > > Vasileios Amoiridis <[email protected]> wrote:
> > > > >
> > > > > > The read_press/read_humid functions need the updated t_fine value
> > > > > > in order to calculate the current pressure/humidity. Temperature
> > > > > > reads should be removed from the read_press/read_humid functions
> > > > > > and should be placed in the oneshot captures before the pressure
> > > > > > and humidity reads. This makes the code more intuitive.
> > > > > >
> > > > > > Signed-off-by: Vasileios Amoiridis <[email protected]>
> > > > >
> > > > > To me this makes the use of these calls less obvious than they were
> > > > > previously. The calls are made close to where t_fine is used and
> > > > > don't have to go via the indirection of chip_info.
> > > > >
> > > > > So I disagree. I think this change makes the code a lot less
> > > > > clear.
> > > > >
> > > >
> > > > This was mainly driven by the fact that I wanted to avoid reading
> > > > the temperature 3 times in case temp, press and humid are enabled
> > > > and there are consecutive buffer readings. But thank you for the
> > > > proposal I really appreciate it!
> > > >
> > >
> > > Hi, just a side note reflecting on this. Depending on your sampling
> > > frequency
> > > and registers data shadowing, to avoid compensating with different samples
> > > between readings, we should be doing burst readings to get a bundle of the
> > > temperature+pressure and/or humidity.
> > > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on
> > > the
> > > memory. On the bmp580 this is not a problem as the values are already
> > > compensated, you`ll get always the latest reading.
> > >
> > > Kind regard,
> > > Angel
> >
> > Hi Angel,
> >
> > Thank you for pointing this out! Indeed that's true but I noticed that this is
> > not
> > the case for the BMP{085/180} devices. I just feel that some changes might
> > make
> > data acquisition/processing faster for a device (like the one you proposed)
> > but
> > it might make the code much more unreadable and unmaintanable. I will try and
> > see if something could be done in that sense but I feel that keeping it simple
> > will
> > be good for everyone!
> >
> > Cheers,
> > Vasilis
>
> Yeah, data adquisition on bmp085/180 is already different as they don't support
> continuous mode as the newer models and you have to warm-up the sensor and do
> one-shot readings. There's already a different code path in place for that
> models. I guess that is the price to pay to support that wide range of
> sensors...
> Anyway, this patches are already big and you're doing quite a lot of heavy-
> lifting right now, so don't pay much attention to my ramblings! Regardless,
> happy to help with tasks polishing and updating this driver :)
If burst readings do make sense: We can reasonably assume anyone who is using
this sensor and is using buffered mode probably wants to 'mostly' grab all the
channels, then a specific function that always grabs them all, plus use of
available_scan_masks = { ALL BITS, 0 }; will let the IIO core deal with any
cases where only some channels are requested. This is something we
do in drivers where there is some interaction between the channels (like here)
or where burst reads are much more efficient than single channels (possibly
also true here) and complexity is significant for switching between burst
and single channels reads.
That covers a lot of devices and is part of why we have the core code
do channel de-multiplexing rather than leaving it for the drivers. The other
reason that drove that complexity was unrelated to this driver (SoC ADCs with
some channels used for touchscreens, and others for unrelated purposes).
You may just want to reduce how much code you are reusing from
the oneshot single channel sysfs reads so that you can just do a single set
of readings and use them as needed for compensation etc.
Jonathan
>
> Kind regards,
> Angel
>