* [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal
@ 2017-01-18 21:50 John Brooks
2017-01-19 11:25 ` Harald Geyer
0 siblings, 1 reply; 5+ messages in thread
From: John Brooks @ 2017-01-18 21:50 UTC (permalink / raw)
To: linux-iio, Harald Geyer; +Cc: Jonathan Cameron
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal
2017-01-18 21:50 [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal John Brooks
@ 2017-01-19 11:25 ` Harald Geyer
2017-01-22 13:36 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Harald Geyer @ 2017-01-19 11:25 UTC (permalink / raw)
To: John Brooks; +Cc: linux-iio, Jonathan Cameron
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal
2017-01-19 11:25 ` Harald Geyer
@ 2017-01-22 13:36 ` Jonathan Cameron
2017-01-27 1:33 ` John Brooks
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2017-01-22 13:36 UTC (permalink / raw)
To: Harald Geyer, John Brooks; +Cc: linux-iio
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
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal
2017-01-22 13:36 ` Jonathan Cameron
@ 2017-01-27 1:33 ` John Brooks
2017-01-28 10:30 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: John Brooks @ 2017-01-27 1:33 UTC (permalink / raw)
To: Jonathan Cameron, Harald Geyer; +Cc: linux-iio
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal
2017-01-27 1:33 ` John Brooks
@ 2017-01-28 10:30 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2017-01-28 10:30 UTC (permalink / raw)
To: John Brooks, Harald Geyer; +Cc: linux-iio
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-28 10:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-18 21:50 [PATCH] iio: dht11: Use usleep_range instead of msleep for start signal John Brooks
2017-01-19 11:25 ` Harald Geyer
2017-01-22 13:36 ` Jonathan Cameron
2017-01-27 1:33 ` John Brooks
2017-01-28 10:30 ` Jonathan Cameron
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).