Re: [PATCH v9 1/4] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[ASSI <Stromeko@nexgo.de>]

David Lechner writes:
> It looks like the wait time calculations in bmp280_wait_conv() don't
> match up with the datasheet [1] that I found.
>
> [1]: https://cdn-shop.adafruit.com/datasheets/BST-BMP280-DS001-11.pdf

The canonical location for these is
https://www.bosch-sensortec.com/products/downloads/ and there is a later
version available (no changes w.r.t. the information discussed here,
though).

> There, table 13 says that if pressure and temperature oversampling
> are bot set to x1, then it could take up to 6.4 ms to complete the
> measurement.

The BME280 has a slightly different specification that matches up with
the numbers in the header file…

> However, in the code, we have:
>
> ```
> #define BMP280_MEAS_OFFSET		1250
> #define BMP280_MEAS_DUR			2300
> #define BMP280_PRESS_HUMID_MEAS_OFFSET	575
> ```

… with the proviso that BMP280_MEAS_OFFSET is applied to each
measurement independent of any other settings and
BMP280_PRESS_HUMID_MEAS_OFFSET to the humidity and pressure measurement
if these are activated.

> and 
>
> ```
> 	/* Check if we are using a BME280 device */
> 	if (data->oversampling_humid)
> 		meas_time_us = BMP280_PRESS_HUMID_MEAS_OFFSET +
> 				BIT(data->oversampling_humid) * BMP280_MEAS_DUR;
>
> 	else
> 		meas_time_us = 0;
>
> 	/* Pressure measurement time */
> 	meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
> 			BIT(data->oversampling_press) * BMP280_MEAS_DUR;
>
> 	/* Temperature measurement time */
> 	meas_time_us += BIT(data->oversampling_temp) * BMP280_MEAS_DUR;
>
> 	/* Waiting time according to the BM(P/E)2 Sensor API */
> 	fsleep(meas_time_us);
> ```
>
> Assuming BIT(*oversampling*) is 1 for x1 oversampling, we get
>
> meas_time_us = (0) + (575 + 1 * 2300) + (1 * 2300) = 5175
>                ^     ^                  ^
>                |     |                  |
>                |     |                  temperature
>                |     pressure
>                humidity
>
> 5175 microseconds is less than the 6.4 milliseconds max time, so
> that would explain the error that is seen since the error is
> triggered by the status bit that says the chip is still busy taking
> the measurement.
>
> BMP280_MEAS_OFFSET is unused in the code. But 6400 - 5175 is 1225
> which is very close to 1250. So I think the intention was to include
> that it the calculation like this:

Yes, I think that's where the trouble comes from.

> ---
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 74505c9ec1a0..50c869e3d5c9 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -1042,14 +1042,13 @@ static int bmp280_wait_conv(struct bmp280_data *data)
>  	unsigned int reg, meas_time_us;
>  	int ret;
>  
> +	meas_time_us = BMP280_MEAS_OFFSET;
> +
>  	/* Check if we are using a BME280 device */
>  	if (data->oversampling_humid)
> -		meas_time_us = BMP280_PRESS_HUMID_MEAS_OFFSET +
> +		meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
>  				BIT(data->oversampling_humid) * BMP280_MEAS_DUR;
>  
> -	else
> -		meas_time_us = 0;
> -
>  	/* Pressure measurement time */
>  	meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
>  			BIT(data->oversampling_press) * BMP280_MEAS_DUR;
> ---

That looks correct to me. So in my case (all three measurements
performed) the measurement should take 8ms nominal / 9.3ms maximum, but
the current code waits just 8.05ms (which is still slightly above
nominal, but apparently not sufficiently long for my sensor).

Another area of concern is that now that each sysfs read triggers
another forced measurement instead of just looking at the values from
the last one that was produced automatically, should the oversampling
ratio of the unused measurements (as specified by chan->type) for that
read not be set to zero temporarily so that the measurements whose
values you don't get the result of are not performed?  That would still
be at least 2.5ms or 12.5ms slower (2×1.25ms + 2×5ms entering sleep mode
and 2.3ms more for each increment of OSR settings) than before depending
on whether the device enters sleep mode inbetween readings (I haven't
figured that one out from the code, but my timing results suggest it
does) in normal mode when reading all three values in succession (which
suggests that there should probably be a separate sysfs interface for
doing a complete read), but not quite as much as currently seen.

Also, instead of waiting the maximum time (typical +15%) unconditionally
it might be preferrable to initially wait just the typical time followed
by a wait of (meas_time_us>>3) and then (meas_time_us>>5) if the busy
flag is still on before returning EBUSY.  That of course depends on how
large the overhead is of that wait plus the register read, but it
probably still is a net win even if the actual sensor always requires
two reads.

> Hopefully you can compile your own kernel and test this. If that fixes it
> we can turn it into a proper patch.

I used to compile the driver myself, but Tumbleweed since switched to a
signed kernel, so I have to figure out how to load an unsigned driver and
then get up to speed again on how to compile and install it.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+