[PATCH] iio: dht11: Use usleep_range instead of msleep for start signal

[PATCH] iio: dht11: Use usleep_range instead of msleep for start signal

[John Brooks]

The DHT22 (AM2302) datasheet specifies that the LOW start pulse should not
exceed 20ms. However, observations with an oscilloscope of an RPi Model 2B
(rev 1.1) communicating with a DHT22 sensor showed that the driver was
consistently sending start pulses longer than 20ms:

Kernel 4.7.10-v7+ (n=132):
    Minimum pulse length: 20.20ms
    Maximum:              29.84ms
    Mean:                 24.96ms
    StDev:                2.82ms
    Sensor response rate: 100%
    Read success rate:    76%

On kernel 4.8, the start pulse was so long that the sensor would not even
respond 97% of the time:

Kernel 4.8.16-v7+ (n=100):
    Minimum pulse length: 30.4ms
    Maximum:              74.4ms
    Mean:                 39.3ms
    StDev:                10.2ms
    Sensor response rate: 3%
    Read success rate:    3%

The driver would return ETIMEDOUT and write log messages like this:

[   51.430987] dht11 dht11@0: Only 1 signal edges detected
[   66.311019] dht11 dht11@0: Only 0 signal edges detected

Replacing msleep(18) with usleep_range(18000, 20000) made the pulse length
sane again and restored responsiveness:

Kernel 4.8.16-v7+ with usleep_range (n=123):
    Minimum pulse length: 18.16ms
    Maximum:              20.20ms
    Mean:                 19.85ms
    StDev:                0.51ms
    Sensor response rate: 100%
    Read success rate:    84%

Cc: stable@vger.kernel.org
Signed-off-by: John Brooks <john@fastquake.com>
---
 drivers/iio/humidity/dht11.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 9c47bc9..2a22ad9 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -71,7 +71,8 @@
  * a) select an implementation using busy loop polling on those systems
  * b) use the checksum to do some probabilistic decoding
  */
-#define DHT11_START_TRANSMISSION	18  /* ms */
+#define DHT11_START_TRANSMISSION_MIN	18000  /* us */
+#define DHT11_START_TRANSMISSION_MAX	20000  /* us */
 #define DHT11_MIN_TIMERES	34000  /* ns */
 #define DHT11_THRESHOLD		49000  /* ns */
 #define DHT11_AMBIG_LOW		23000  /* ns */
@@ -228,7 +229,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		ret = gpio_direction_output(dht11->gpio, 0);
 		if (ret)
 			goto err;
-		msleep(DHT11_START_TRANSMISSION);
+		usleep_range(DHT11_START_TRANSMISSION_MIN,
+			     DHT11_START_TRANSMISSION_MAX);
 		ret = gpio_direction_input(dht11->gpio);
 		if (ret)
 			goto err;
-- 
2.7.4

Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal

[Harald Geyer]

John Brooks writes:
> The DHT22 (AM2302) datasheet specifies that the LOW start pulse should not
> exceed 20ms. However, observations with an oscilloscope of an RPi Model 2B
> (rev 1.1) communicating with a DHT22 sensor showed that the driver was
> consistently sending start pulses longer than 20ms:
> 
> Kernel 4.7.10-v7+ (n=132):
>     Minimum pulse length: 20.20ms
>     Maximum:              29.84ms
>     Mean:                 24.96ms
>     StDev:                2.82ms
>     Sensor response rate: 100%
>     Read success rate:    76%
> 
> On kernel 4.8, the start pulse was so long that the sensor would not even
> respond 97% of the time:
> 
> Kernel 4.8.16-v7+ (n=100):
>     Minimum pulse length: 30.4ms
>     Maximum:              74.4ms
>     Mean:                 39.3ms
>     StDev:                10.2ms
>     Sensor response rate: 3%
>     Read success rate:    3%
> 
> The driver would return ETIMEDOUT and write log messages like this:
> 
> [   51.430987] dht11 dht11@0: Only 1 signal edges detected
> [   66.311019] dht11 dht11@0: Only 0 signal edges detected
> 
> Replacing msleep(18) with usleep_range(18000, 20000) made the pulse length
> sane again and restored responsiveness:
> 
> Kernel 4.8.16-v7+ with usleep_range (n=123):
>     Minimum pulse length: 18.16ms
>     Maximum:              20.20ms
>     Mean:                 19.85ms
>     StDev:                0.51ms
>     Sensor response rate: 100%
>     Read success rate:    84%
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: John Brooks <john@fastquake.com>
> ---
>  drivers/iio/humidity/dht11.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 9c47bc9..2a22ad9 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -71,7 +71,8 @@
>   * a) select an implementation using busy loop polling on those systems
>   * b) use the checksum to do some probabilistic decoding
>   */
> -#define DHT11_START_TRANSMISSION	18  /* ms */
> +#define DHT11_START_TRANSMISSION_MIN	18000  /* us */
> +#define DHT11_START_TRANSMISSION_MAX	20000  /* us */

If you want to stay strictly within the 20ms range, you might want to
make this a bit lower like 19750. However, I don't care much either way.
I think this is a reasonable change.

Reviewed-by: Harald Geyer <harald@ccbib.org>

I do wonder why there is a difference between 4.7 and 4.8. Is this a
performance regression on the RPi or have there been changes to the
scheduler, that make this actually expected behaviour. (Ie is this a
bug or a feature?)

Thanks for your patch,
Harald

>  #define DHT11_MIN_TIMERES	34000  /* ns */
>  #define DHT11_THRESHOLD		49000  /* ns */
>  #define DHT11_AMBIG_LOW		23000  /* ns */
> @@ -228,7 +229,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		ret = gpio_direction_output(dht11->gpio, 0);
>  		if (ret)
>  			goto err;
> -		msleep(DHT11_START_TRANSMISSION);
> +		usleep_range(DHT11_START_TRANSMISSION_MIN,
> +			     DHT11_START_TRANSMISSION_MAX);
>  		ret = gpio_direction_input(dht11->gpio);
>  		if (ret)
>  			goto err;
> -- 
> 2.7.4
> 

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal

[Jonathan Cameron]

On 19/01/17 11:25, Harald Geyer wrote:
> John Brooks writes:
>> The DHT22 (AM2302) datasheet specifies that the LOW start pulse should not
>> exceed 20ms. However, observations with an oscilloscope of an RPi Model 2B
>> (rev 1.1) communicating with a DHT22 sensor showed that the driver was
>> consistently sending start pulses longer than 20ms:
>>
>> Kernel 4.7.10-v7+ (n=132):
>>     Minimum pulse length: 20.20ms
>>     Maximum:              29.84ms
>>     Mean:                 24.96ms
>>     StDev:                2.82ms
>>     Sensor response rate: 100%
>>     Read success rate:    76%
>>
>> On kernel 4.8, the start pulse was so long that the sensor would not even
>> respond 97% of the time:
>>
>> Kernel 4.8.16-v7+ (n=100):
>>     Minimum pulse length: 30.4ms
>>     Maximum:              74.4ms
>>     Mean:                 39.3ms
>>     StDev:                10.2ms
>>     Sensor response rate: 3%
>>     Read success rate:    3%
>>
>> The driver would return ETIMEDOUT and write log messages like this:
>>
>> [   51.430987] dht11 dht11@0: Only 1 signal edges detected
>> [   66.311019] dht11 dht11@0: Only 0 signal edges detected
>>
>> Replacing msleep(18) with usleep_range(18000, 20000) made the pulse length
>> sane again and restored responsiveness:
>>
>> Kernel 4.8.16-v7+ with usleep_range (n=123):
>>     Minimum pulse length: 18.16ms
>>     Maximum:              20.20ms
>>     Mean:                 19.85ms
>>     StDev:                0.51ms
>>     Sensor response rate: 100%
>>     Read success rate:    84%
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: John Brooks <john@fastquake.com>
>> ---
>>  drivers/iio/humidity/dht11.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index 9c47bc9..2a22ad9 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
>> @@ -71,7 +71,8 @@
>>   * a) select an implementation using busy loop polling on those systems
>>   * b) use the checksum to do some probabilistic decoding
>>   */
>> -#define DHT11_START_TRANSMISSION	18  /* ms */
>> +#define DHT11_START_TRANSMISSION_MIN	18000  /* us */
>> +#define DHT11_START_TRANSMISSION_MAX	20000  /* us */
> 
> If you want to stay strictly within the 20ms range, you might want to
> make this a bit lower like 19750. However, I don't care much either way.
> I think this is a reasonable change.
> 
> Reviewed-by: Harald Geyer <harald@ccbib.org>
> 
> I do wonder why there is a difference between 4.7 and 4.8. Is this a
> performance regression on the RPi or have there been changes to the
> scheduler, that make this actually expected behaviour. (Ie is this a
> bug or a feature?)
Agreed.  Any chance you can do a bisection on this John?

Either way - defence in depth!
Applied to the fixes-togreg branch of iio.git.

thanks,

Jonathan
> 
> Thanks for your patch,
> Harald
> 
>>  #define DHT11_MIN_TIMERES	34000  /* ns */
>>  #define DHT11_THRESHOLD		49000  /* ns */
>>  #define DHT11_AMBIG_LOW		23000  /* ns */
>> @@ -228,7 +229,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>  		ret = gpio_direction_output(dht11->gpio, 0);
>>  		if (ret)
>>  			goto err;
>> -		msleep(DHT11_START_TRANSMISSION);
>> +		usleep_range(DHT11_START_TRANSMISSION_MIN,
>> +			     DHT11_START_TRANSMISSION_MAX);
>>  		ret = gpio_direction_input(dht11->gpio);
>>  		if (ret)
>>  			goto err;
>> -- 
>> 2.7.4
>>
> 

Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal

[John Brooks]

On Sun, Jan 22, 2017 at 01:36:47PM +0000, Jonathan Cameron wrote:
> Agreed.  Any chance you can do a bisection on this John?
> 
> Either way - defence in depth!
> Applied to the fixes-togreg branch of iio.git.
> 
> thanks,
> 
> Jonathan

It's my first bug fixing kernel patch; I'm very happy to see it applied :)

I would like to bisect this. It's a bit of a pain because I'll need to branch
and apply all the RPi kernel patches every step of the way. If I find the time,
I will do it.

Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal

[Jonathan Cameron]

On 27/01/17 01:33, John Brooks wrote:
> On Sun, Jan 22, 2017 at 01:36:47PM +0000, Jonathan Cameron wrote:
>> Agreed.  Any chance you can do a bisection on this John?
>>
>> Either way - defence in depth!
>> Applied to the fixes-togreg branch of iio.git.
>>
>> thanks,
>>
>> Jonathan
> 
> It's my first bug fixing kernel patch; I'm very happy to see it applied :)
> 
> I would like to bisect this. It's a bit of a pain because I'll need to branch
> and apply all the RPi kernel patches every step of the way. If I find the time,
> I will do it.
Cool and good luck!
> --
> 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
>