[PATCH 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits

[PATCH 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits

[Harald Geyer]

Instead of guessing where the data starts, we now just try to decode from
every possible start position. This causes no additional overhead if we
properly received the full preamble and only costs a few extra CPU cycles
in the case where the preamble is corrupted. This is much more efficient
than to return an error to userspace and start over again.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Jonathan Bell:
Can you test if this fixes the problems you reported on RPi?

(It does fix problems I noticed when I slowed down my test HW with
enabling the debug logging introduced later in this series, so including
this patch is a good thing even if it doesn't fix Jonathan's problems.)

 drivers/iio/humidity/dht11.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 1165b1c..1ca284a 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -161,7 +161,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			int *val, int *val2, long m)
 {
 	struct dht11 *dht11 = iio_priv(iio_dev);
-	int ret, timeres;
+	int ret, timeres, offset;
 
 	mutex_lock(&dht11->lock);
 	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
@@ -208,11 +208,14 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		if (ret < 0)
 			goto err;
 
-		ret = dht11_decode(dht11,
-				   dht11->num_edges == DHT11_EDGES_PER_READ ?
-					DHT11_EDGES_PREAMBLE :
-					DHT11_EDGES_PREAMBLE - 2,
-				timeres);
+		offset = DHT11_EDGES_PREAMBLE +
+				dht11->num_edges - DHT11_EDGES_PER_READ;
+		for (; offset >= 0; --offset) {
+			ret = dht11_decode(dht11, offset, timeres);
+			if (!ret)
+				break;
+		}
+
 		if (ret)
 			goto err;
 	}
-- 
2.1.4

[PATCH 2/3] iio: dht11: Simplify decoding algorithm

[Harald Geyer]

The new algorithm uses a 'one size fits em all' threshold, which should
be easier to understand and debug. I believe there are no regressions
compared to the old adaptive threshold algorithm. I don't remember why
I chose the old algorithm when I initially wrote the driver.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/iio/humidity/dht11.c | 64 +++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 1ca284a..cd1477d 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -50,12 +50,32 @@
 #define DHT11_EDGES_PER_READ (2 * DHT11_BITS_PER_READ + \
 			      DHT11_EDGES_PREAMBLE + 1)
 
-/* Data transmission timing (nano seconds) */
+/*
+ * Data transmission timing:
+ * Data bits are encoded as pulse length (high time) on the data line.
+ * 0-bit: 22-30uS -- typically 26uS (AM2302)
+ * 1-bit: 68-75uS -- typically 70uS (AM2302)
+ * The acutal timings also depend on the properties of the cable, with
+ * longer cables typically making pulses shorter.
+ *
+ * Our decoding depends on the time resolution of the system:
+ * timeres > 34uS ... don't know what a 1-tick pulse is
+ * 34uS > timeres > 30uS ... no problem (30kHz and 32kHz clocks)
+ * 30uS > timeres > 23uS ... don't know what a 2-tick pulse is
+ * timeres < 23uS ... no problem
+ *
+ * Luckily clocks in the 33-44kHz range are quite uncommon, so we can
+ * support most systems if the threshold for decoding a pulse as 1-bit
+ * is chosen carefully. If somebody really wants to support clocks around
+ * 40kHz, where this driver is most unreliable, there are two options.
+ * 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_SENSOR_RESPONSE	80000
-#define DHT11_START_BIT		50000
-#define DHT11_DATA_BIT_LOW	27000
-#define DHT11_DATA_BIT_HIGH	70000
+#define DHT11_MIN_TIMERES	34000  /* ns */
+#define DHT11_THRESHOLD		49000  /* ns */
+#define DHT11_AMBIG_LOW		23000  /* ns */
+#define DHT11_AMBIG_HIGH	30000  /* ns */
 
 struct dht11 {
 	struct device			*dev;
@@ -76,43 +96,39 @@ struct dht11 {
 	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
 };
 
-static unsigned char dht11_decode_byte(int *timing, int threshold)
+static unsigned char dht11_decode_byte(char *bits)
 {
 	unsigned char ret = 0;
 	int i;
 
 	for (i = 0; i < 8; ++i) {
 		ret <<= 1;
-		if (timing[i] >= threshold)
+		if (bits[i])
 			++ret;
 	}
 
 	return ret;
 }
 
-static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
+static int dht11_decode(struct dht11 *dht11, int offset)
 {
-	int i, t, timing[DHT11_BITS_PER_READ], threshold;
+	int i, t;
+	char bits[DHT11_BITS_PER_READ];
 	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
-	threshold = DHT11_DATA_BIT_HIGH / timeres;
-	if (DHT11_DATA_BIT_LOW / timeres + 1 >= threshold)
-		pr_err("dht11: WARNING: decoding ambiguous\n");
-
-	/* scale down with timeres and check validity */
 	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
 		t = dht11->edges[offset + 2 * i + 2].ts -
 			dht11->edges[offset + 2 * i + 1].ts;
 		if (!dht11->edges[offset + 2 * i + 1].value)
 			return -EIO;  /* lost synchronisation */
-		timing[i] = t / timeres;
+		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
 	}
 
-	hum_int = dht11_decode_byte(timing, threshold);
-	hum_dec = dht11_decode_byte(&timing[8], threshold);
-	temp_int = dht11_decode_byte(&timing[16], threshold);
-	temp_dec = dht11_decode_byte(&timing[24], threshold);
-	checksum = dht11_decode_byte(&timing[32], threshold);
+	hum_int = dht11_decode_byte(bits);
+	hum_dec = dht11_decode_byte(&bits[8]);
+	temp_int = dht11_decode_byte(&bits[16]);
+	temp_dec = dht11_decode_byte(&bits[24]);
+	checksum = dht11_decode_byte(&bits[32]);
 
 	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
 		return -EIO;
@@ -166,7 +182,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 	mutex_lock(&dht11->lock);
 	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
 		timeres = ktime_get_resolution_ns();
-		if (DHT11_DATA_BIT_HIGH < 2 * timeres) {
+		if (timeres > DHT11_MIN_TIMERES) {
 			dev_err(dht11->dev, "timeresolution %dns too low\n",
 				timeres);
 			/* In theory a better clock could become available
@@ -176,6 +192,10 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			ret = -EAGAIN;
 			goto err;
 		}
+		if (timeres > DHT11_AMBIG_LOW && timeres < DHT11_AMBIG_HIGH)
+			dev_warn(dht11->dev,
+				 "timeresolution: %dns - decoding ambiguous\n",
+				 timeres);
 
 		reinit_completion(&dht11->completion);
 
@@ -211,7 +231,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		offset = DHT11_EDGES_PREAMBLE +
 				dht11->num_edges - DHT11_EDGES_PER_READ;
 		for (; offset >= 0; --offset) {
-			ret = dht11_decode(dht11, offset, timeres);
+			ret = dht11_decode(dht11, offset);
 			if (!ret)
 				break;
 		}
-- 
2.1.4

[PATCH 3/3] iio: dht11: Improve logging

[Harald Geyer]

* Unify log messages
* Add more DEBUG messages

Apparently this driver is working unreliably on some platforms that I can't
test. Therefore I want an easy way for bug reporters to provide useful
information without making the driver too chatty by default.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/iio/humidity/dht11.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index cd1477d..f0cb28c 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -96,6 +96,24 @@ struct dht11 {
 	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
 };
 
+#ifdef CONFIG_DYNAMIC_DEBUG
+/*
+ * dht11_edges_print: show the data as actually received by the
+ *                    driver.
+ */
+static void dht11_edges_print(struct dht11 *dht11)
+{
+	int i;
+
+	dev_dbg(dht11->dev, "%d edges detected:\n", dht11->num_edges);
+	for (i = 1; i < dht11->num_edges; ++i) {
+		dev_dbg(dht11->dev, "%d: %lld ns %s\n", i,
+			dht11->edges[i].ts - dht11->edges[i - 1].ts,
+			dht11->edges[i - 1].value ? "high" : "low");
+	}
+}
+#endif /* CONFIG_DYNAMIC_DEBUG */
+
 static unsigned char dht11_decode_byte(char *bits)
 {
 	unsigned char ret = 0;
@@ -119,8 +137,12 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
 		t = dht11->edges[offset + 2 * i + 2].ts -
 			dht11->edges[offset + 2 * i + 1].ts;
-		if (!dht11->edges[offset + 2 * i + 1].value)
-			return -EIO;  /* lost synchronisation */
+		if (!dht11->edges[offset + 2 * i + 1].value) {
+			dev_dbg(dht11->dev,
+				"lost synchronisation at edge %d\n",
+				offset + 2 * i + 1);
+			return -EIO;
+		}
 		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
 	}
 
@@ -130,8 +152,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	temp_dec = dht11_decode_byte(&bits[24]);
 	checksum = dht11_decode_byte(&bits[32]);
 
-	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
+		dev_dbg(dht11->dev, "invalid checksum\n");
 		return -EIO;
+	}
 
 	dht11->timestamp = ktime_get_real_ns();
 	if (hum_int < 20) {  /* DHT22 */
@@ -182,6 +206,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 	mutex_lock(&dht11->lock);
 	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
 		timeres = ktime_get_resolution_ns();
+		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
 		if (timeres > DHT11_MIN_TIMERES) {
 			dev_err(dht11->dev, "timeresolution %dns too low\n",
 				timeres);
@@ -219,10 +244,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 
 		free_irq(dht11->irq, iio_dev);
 
+#ifdef CONFIG_DYNAMIC_DEBUG
+		dht11_edges_print(dht11);
+#endif
+
 		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
-			dev_err(&iio_dev->dev,
-				"Only %d signal edges detected\n",
-					dht11->num_edges);
+			dev_err(dht11->dev, "Only %d signal edges detected\n",
+				dht11->num_edges);
 			ret = -ETIMEDOUT;
 		}
 		if (ret < 0)
-- 
2.1.4

Re: [PATCH 2/3] iio: dht11: Simplify decoding algorithm

[Jonathan Cameron]

On 30/12/15 14:26, Harald Geyer wrote:
> The new algorithm uses a 'one size fits em all' threshold, which should
> be easier to understand and debug. I believe there are no regressions
> compared to the old adaptive threshold algorithm. I don't remember why
> I chose the old algorithm when I initially wrote the driver.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Hmm. Couple of trivial bits inline.

I'd like some tested-by's on this one if possible...

Such a 'fun' device ;)

Jonathan
> ---
>  drivers/iio/humidity/dht11.c | 64 +++++++++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 1ca284a..cd1477d 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -50,12 +50,32 @@
>  #define DHT11_EDGES_PER_READ (2 * DHT11_BITS_PER_READ + \
>  			      DHT11_EDGES_PREAMBLE + 1)
>  
> -/* Data transmission timing (nano seconds) */
> +/*
> + * Data transmission timing:
> + * Data bits are encoded as pulse length (high time) on the data line.
> + * 0-bit: 22-30uS -- typically 26uS (AM2302)
> + * 1-bit: 68-75uS -- typically 70uS (AM2302)
> + * The acutal timings also depend on the properties of the cable, with
actual
> + * longer cables typically making pulses shorter.
> + *
> + * Our decoding depends on the time resolution of the system:
> + * timeres > 34uS ... don't know what a 1-tick pulse is
> + * 34uS > timeres > 30uS ... no problem (30kHz and 32kHz clocks)
> + * 30uS > timeres > 23uS ... don't know what a 2-tick pulse is
> + * timeres < 23uS ... no problem
> + *
> + * Luckily clocks in the 33-44kHz range are quite uncommon, so we can
> + * support most systems if the threshold for decoding a pulse as 1-bit
> + * is chosen carefully. If somebody really wants to support clocks around
> + * 40kHz, where this driver is most unreliable, there are two options.
> + * 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_SENSOR_RESPONSE	80000
> -#define DHT11_START_BIT		50000
> -#define DHT11_DATA_BIT_LOW	27000
> -#define DHT11_DATA_BIT_HIGH	70000
> +#define DHT11_MIN_TIMERES	34000  /* ns */
> +#define DHT11_THRESHOLD		49000  /* ns */
> +#define DHT11_AMBIG_LOW		23000  /* ns */
> +#define DHT11_AMBIG_HIGH	30000  /* ns */
>  
>  struct dht11 {
>  	struct device			*dev;
> @@ -76,43 +96,39 @@ struct dht11 {
>  	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>  };
>  
> -static unsigned char dht11_decode_byte(int *timing, int threshold)
> +static unsigned char dht11_decode_byte(char *bits)
>  {
>  	unsigned char ret = 0;
>  	int i;
>  
>  	for (i = 0; i < 8; ++i) {
>  		ret <<= 1;
> -		if (timing[i] >= threshold)
> +		if (bits[i])
>  			++ret;
>  	}
>  
>  	return ret;
>  }
>  
> -static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
> +static int dht11_decode(struct dht11 *dht11, int offset)
>  {
> -	int i, t, timing[DHT11_BITS_PER_READ], threshold;
> +	int i, t;
> +	char bits[DHT11_BITS_PER_READ];
>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>  
> -	threshold = DHT11_DATA_BIT_HIGH / timeres;
> -	if (DHT11_DATA_BIT_LOW / timeres + 1 >= threshold)
> -		pr_err("dht11: WARNING: decoding ambiguous\n");
> -
> -	/* scale down with timeres and check validity */
>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>  		t = dht11->edges[offset + 2 * i + 2].ts -
>  			dht11->edges[offset + 2 * i + 1].ts;
>  		if (!dht11->edges[offset + 2 * i + 1].value)
>  			return -EIO;  /* lost synchronisation */
> -		timing[i] = t / timeres;
> +		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
t > DHT11_THRESHOLD is already going to give 0 or 1...
>  	}
>  
> -	hum_int = dht11_decode_byte(timing, threshold);
> -	hum_dec = dht11_decode_byte(&timing[8], threshold);
> -	temp_int = dht11_decode_byte(&timing[16], threshold);
> -	temp_dec = dht11_decode_byte(&timing[24], threshold);
> -	checksum = dht11_decode_byte(&timing[32], threshold);
> +	hum_int = dht11_decode_byte(bits);
> +	hum_dec = dht11_decode_byte(&bits[8]);
> +	temp_int = dht11_decode_byte(&bits[16]);
> +	temp_dec = dht11_decode_byte(&bits[24]);
> +	checksum = dht11_decode_byte(&bits[32]);
>  
>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>  		return -EIO;
> @@ -166,7 +182,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  	mutex_lock(&dht11->lock);
>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
>  		timeres = ktime_get_resolution_ns();
> -		if (DHT11_DATA_BIT_HIGH < 2 * timeres) {
> +		if (timeres > DHT11_MIN_TIMERES) {
>  			dev_err(dht11->dev, "timeresolution %dns too low\n",
>  				timeres);
>  			/* In theory a better clock could become available
> @@ -176,6 +192,10 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			ret = -EAGAIN;
>  			goto err;
>  		}
> +		if (timeres > DHT11_AMBIG_LOW && timeres < DHT11_AMBIG_HIGH)
> +			dev_warn(dht11->dev,
> +				 "timeresolution: %dns - decoding ambiguous\n",
> +				 timeres);
>  
>  		reinit_completion(&dht11->completion);
>  
> @@ -211,7 +231,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		offset = DHT11_EDGES_PREAMBLE +
>  				dht11->num_edges - DHT11_EDGES_PER_READ;
>  		for (; offset >= 0; --offset) {
> -			ret = dht11_decode(dht11, offset, timeres);
> +			ret = dht11_decode(dht11, offset);
>  			if (!ret)
>  				break;
>  		}
> 

Re: [PATCH 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits

[Jonathan Cameron]

On 30/12/15 14:26, Harald Geyer wrote:
> Instead of guessing where the data starts, we now just try to decode from
> every possible start position. This causes no additional overhead if we
> properly received the full preamble and only costs a few extra CPU cycles
> in the case where the preamble is corrupted. This is much more efficient
> than to return an error to userspace and start over again.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Seems sensible, but ideally I'd like a tested by!  Sounds like
Jonathan Bell might be able to provide one.

Changes to this driver always worry me given how fiddly the hardware
interface is.

Jonathan
> ---
> Jonathan Bell:
> Can you test if this fixes the problems you reported on RPi?
> 
> (It does fix problems I noticed when I slowed down my test HW with
> enabling the debug logging introduced later in this series, so including
> this patch is a good thing even if it doesn't fix Jonathan's problems.)
> 
>  drivers/iio/humidity/dht11.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 1165b1c..1ca284a 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -161,7 +161,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			int *val, int *val2, long m)
>  {
>  	struct dht11 *dht11 = iio_priv(iio_dev);
> -	int ret, timeres;
> +	int ret, timeres, offset;
>  
>  	mutex_lock(&dht11->lock);
>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
> @@ -208,11 +208,14 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		if (ret < 0)
>  			goto err;
>  
> -		ret = dht11_decode(dht11,
> -				   dht11->num_edges == DHT11_EDGES_PER_READ ?
> -					DHT11_EDGES_PREAMBLE :
> -					DHT11_EDGES_PREAMBLE - 2,
> -				timeres);
> +		offset = DHT11_EDGES_PREAMBLE +
> +				dht11->num_edges - DHT11_EDGES_PER_READ;
> +		for (; offset >= 0; --offset) {
> +			ret = dht11_decode(dht11, offset, timeres);
> +			if (!ret)
> +				break;
> +		}
> +
>  		if (ret)
>  			goto err;
>  	}
> 

Re: [PATCH 3/3] iio: dht11: Improve logging

[Jonathan Cameron]

On 30/12/15 14:26, Harald Geyer wrote:
> * Unify log messages
> * Add more DEBUG messages
> 
> Apparently this driver is working unreliably on some platforms that I can't
> test. Therefore I want an easy way for bug reporters to provide useful
> information without making the driver too chatty by default.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Basically fine, though I'd loose the #ifdefs.

Jonathan
> ---
>  drivers/iio/humidity/dht11.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index cd1477d..f0cb28c 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -96,6 +96,24 @@ struct dht11 {
>  	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>  };
>  
> +#ifdef CONFIG_DYNAMIC_DEBUG
> +/*
> + * dht11_edges_print: show the data as actually received by the
> + *                    driver.
> + */
> +static void dht11_edges_print(struct dht11 *dht11)
> +{
> +	int i;
> +
> +	dev_dbg(dht11->dev, "%d edges detected:\n", dht11->num_edges);
> +	for (i = 1; i < dht11->num_edges; ++i) {
> +		dev_dbg(dht11->dev, "%d: %lld ns %s\n", i,
> +			dht11->edges[i].ts - dht11->edges[i - 1].ts,
> +			dht11->edges[i - 1].value ? "high" : "low");
> +	}
> +}
> +#endif /* CONFIG_DYNAMIC_DEBUG */
Hmm. I guess the ifdef is to avoid having the overhead in all cases.
I'd be more included to leave it out as the overhead isn't huge...

Also it's more than possible people will want to use traditional
non dynamic debug for this...

> +
>  static unsigned char dht11_decode_byte(char *bits)
>  {
>  	unsigned char ret = 0;
> @@ -119,8 +137,12 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>  		t = dht11->edges[offset + 2 * i + 2].ts -
>  			dht11->edges[offset + 2 * i + 1].ts;
> -		if (!dht11->edges[offset + 2 * i + 1].value)
> -			return -EIO;  /* lost synchronisation */
> +		if (!dht11->edges[offset + 2 * i + 1].value) {
> +			dev_dbg(dht11->dev,
> +				"lost synchronisation at edge %d\n",
> +				offset + 2 * i + 1);
> +			return -EIO;
> +		}
>  		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
>  	}
>  
> @@ -130,8 +152,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	temp_dec = dht11_decode_byte(&bits[24]);
>  	checksum = dht11_decode_byte(&bits[32]);
>  
> -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
> +		dev_dbg(dht11->dev, "invalid checksum\n");
>  		return -EIO;
> +	}
>  
>  	dht11->timestamp = ktime_get_real_ns();
>  	if (hum_int < 20) {  /* DHT22 */
> @@ -182,6 +206,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  	mutex_lock(&dht11->lock);
>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
>  		timeres = ktime_get_resolution_ns();
> +		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
>  		if (timeres > DHT11_MIN_TIMERES) {
>  			dev_err(dht11->dev, "timeresolution %dns too low\n",
>  				timeres);
> @@ -219,10 +244,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  
>  		free_irq(dht11->irq, iio_dev);
>  
> +#ifdef CONFIG_DYNAMIC_DEBUG
> +		dht11_edges_print(dht11);
> +#endif
> +
>  		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> -			dev_err(&iio_dev->dev,
> -				"Only %d signal edges detected\n",
> -					dht11->num_edges);
> +			dev_err(dht11->dev, "Only %d signal edges detected\n",
> +				dht11->num_edges);
>  			ret = -ETIMEDOUT;
>  		}
>  		if (ret < 0)
> 

Re: [PATCH 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits

[Harald Geyer]

Jonathan Cameron writes:
> On 30/12/15 14:26, Harald Geyer wrote:
> > Instead of guessing where the data starts, we now just try to decode from
> > every possible start position. This causes no additional overhead if we
> > properly received the full preamble and only costs a few extra CPU cycles
> > in the case where the preamble is corrupted. This is much more efficient
> > than to return an error to userspace and start over again.
> > 
> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> Seems sensible, but ideally I'd like a tested by!  Sounds like
> Jonathan Bell might be able to provide one.

Probably this should have been part of the commit message:
Tested on imx233-olinuxino with both DHT11 and DHT22 the following
patch combinations: 2+3 and 1+2+3.

Of course tests by people who already had problems with the driver in
the past are highly appreciated.

> Changes to this driver always worry me given how fiddly the hardware
> interface is.

Yeah, looks like I already got it wrong in various ways in the past.

Thanks,
Harald

> Jonathan
> > ---
> > Jonathan Bell:
> > Can you test if this fixes the problems you reported on RPi?
> > 
> > (It does fix problems I noticed when I slowed down my test HW with
> > enabling the debug logging introduced later in this series, so including
> > this patch is a good thing even if it doesn't fix Jonathan's problems.)
> > 
> >  drivers/iio/humidity/dht11.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> > index 1165b1c..1ca284a 100644
> > --- a/drivers/iio/humidity/dht11.c
> > +++ b/drivers/iio/humidity/dht11.c
> > @@ -161,7 +161,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >  			int *val, int *val2, long m)
> >  {
> >  	struct dht11 *dht11 = iio_priv(iio_dev);
> > -	int ret, timeres;
> > +	int ret, timeres, offset;
> >  
> >  	mutex_lock(&dht11->lock);
> >  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
> > @@ -208,11 +208,14 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >  		if (ret < 0)
> >  			goto err;
> >  
> > -		ret = dht11_decode(dht11,
> > -				   dht11->num_edges == DHT11_EDGES_PER_READ ?
> > -					DHT11_EDGES_PREAMBLE :
> > -					DHT11_EDGES_PREAMBLE - 2,
> > -				timeres);
> > +		offset = DHT11_EDGES_PREAMBLE +
> > +				dht11->num_edges - DHT11_EDGES_PER_READ;
> > +		for (; offset >= 0; --offset) {
> > +			ret = dht11_decode(dht11, offset, timeres);
> > +			if (!ret)
> > +				break;
> > +		}
> > +
> >  		if (ret)
> >  			goto err;
> >  	}
> > 
> 

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

Re: [PATCH 2/3] iio: dht11: Simplify decoding algorithm

[Harald Geyer]

Jonathan Cameron writes:
> On 30/12/15 14:26, Harald Geyer wrote:
> > The new algorithm uses a 'one size fits em all' threshold, which should
> > be easier to understand and debug. I believe there are no regressions
> > compared to the old adaptive threshold algorithm. I don't remember why
> > I chose the old algorithm when I initially wrote the driver.
> > 
> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> Hmm. Couple of trivial bits inline.
> 
> I'd like some tested-by's on this one if possible...
> 
> Such a 'fun' device ;)

On the up-side its working on poor and long wiring almost as well as
on good and short wiring ... ;)
 
> > -		timing[i] = t / timeres;
> > +		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
> t > DHT11_THRESHOLD is already going to give 0 or 1...

Thanks. I'll send a v2 after everybody has had enough time to test and
comment on the whole series.

Harald

Re: [PATCH 3/3] iio: dht11: Improve logging

[Harald Geyer]

Jonathan Cameron writes:
> On 30/12/15 14:26, Harald Geyer wrote:
> > * Unify log messages
> > * Add more DEBUG messages
> > 
> > Apparently this driver is working unreliably on some platforms that I can't
> > test. Therefore I want an easy way for bug reporters to provide useful
> > information without making the driver too chatty by default.
> > 
> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> Basically fine, though I'd loose the #ifdefs.
> 
> Jonathan
> > ---
> >  drivers/iio/humidity/dht11.c | 40 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 34 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> > index cd1477d..f0cb28c 100644
> > --- a/drivers/iio/humidity/dht11.c
> > +++ b/drivers/iio/humidity/dht11.c
> > @@ -96,6 +96,24 @@ struct dht11 {
> >  	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
> >  };
> >  
> > +#ifdef CONFIG_DYNAMIC_DEBUG
> > +/*
> > + * dht11_edges_print: show the data as actually received by the
> > + *                    driver.
> > + */
> > +static void dht11_edges_print(struct dht11 *dht11)
> > +{
> > +	int i;
> > +
> > +	dev_dbg(dht11->dev, "%d edges detected:\n", dht11->num_edges);
> > +	for (i = 1; i < dht11->num_edges; ++i) {
> > +		dev_dbg(dht11->dev, "%d: %lld ns %s\n", i,
> > +			dht11->edges[i].ts - dht11->edges[i - 1].ts,
> > +			dht11->edges[i - 1].value ? "high" : "low");
> > +	}
> > +}
> > +#endif /* CONFIG_DYNAMIC_DEBUG */
> Hmm. I guess the ifdef is to avoid having the overhead in all cases.
> I'd be more included to leave it out as the overhead isn't huge...

Actually my reason was that this particular function is very chatty
writing more than 80 lines of output. This can easily push more important
lines out of the ring buffer. So I thought an easy way to turn it off
without recompiling the kernel is appropriate.
 
> Also it's more than possible people will want to use traditional
> non dynamic debug for this...

I don't expect so, but I entirely rely on your judgement in this case.
What do you want to have there instead?

Thanks,
Harald

> > +
> >  static unsigned char dht11_decode_byte(char *bits)
> >  {
> >  	unsigned char ret = 0;
> > @@ -119,8 +137,12 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
> >  		t = dht11->edges[offset + 2 * i + 2].ts -
> >  			dht11->edges[offset + 2 * i + 1].ts;
> > -		if (!dht11->edges[offset + 2 * i + 1].value)
> > -			return -EIO;  /* lost synchronisation */
> > +		if (!dht11->edges[offset + 2 * i + 1].value) {
> > +			dev_dbg(dht11->dev,
> > +				"lost synchronisation at edge %d\n",
> > +				offset + 2 * i + 1);
> > +			return -EIO;
> > +		}
> >  		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
> >  	}
> >  
> > @@ -130,8 +152,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >  	temp_dec = dht11_decode_byte(&bits[24]);
> >  	checksum = dht11_decode_byte(&bits[32]);
> >  
> > -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> > +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
> > +		dev_dbg(dht11->dev, "invalid checksum\n");
> >  		return -EIO;
> > +	}
> >  
> >  	dht11->timestamp = ktime_get_real_ns();
> >  	if (hum_int < 20) {  /* DHT22 */
> > @@ -182,6 +206,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >  	mutex_lock(&dht11->lock);
> >  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
> >  		timeres = ktime_get_resolution_ns();
> > +		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
> >  		if (timeres > DHT11_MIN_TIMERES) {
> >  			dev_err(dht11->dev, "timeresolution %dns too low\n",
> >  				timeres);
> > @@ -219,10 +244,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >  
> >  		free_irq(dht11->irq, iio_dev);
> >  
> > +#ifdef CONFIG_DYNAMIC_DEBUG
> > +		dht11_edges_print(dht11);
> > +#endif
> > +
> >  		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> > -			dev_err(&iio_dev->dev,
> > -				"Only %d signal edges detected\n",
> > -					dht11->num_edges);
> > +			dev_err(dht11->dev, "Only %d signal edges detected\n",
> > +				dht11->num_edges);
> >  			ret = -ETIMEDOUT;
> >  		}
> >  		if (ret < 0)
> > 
> 

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

Re: [PATCH 3/3] iio: dht11: Improve logging

[Jonathan Cameron]

On 04/01/16 14:49, Harald Geyer wrote:
> Jonathan Cameron writes:
>> On 30/12/15 14:26, Harald Geyer wrote:
>>> * Unify log messages
>>> * Add more DEBUG messages
>>>
>>> Apparently this driver is working unreliably on some platforms that I can't
>>> test. Therefore I want an easy way for bug reporters to provide useful
>>> information without making the driver too chatty by default.
>>>
>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>> Basically fine, though I'd loose the #ifdefs.
>>
>> Jonathan
>>> ---
>>>  drivers/iio/humidity/dht11.c | 40 ++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 34 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>>> index cd1477d..f0cb28c 100644
>>> --- a/drivers/iio/humidity/dht11.c
>>> +++ b/drivers/iio/humidity/dht11.c
>>> @@ -96,6 +96,24 @@ struct dht11 {
>>>  	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>>>  };
>>>  
>>> +#ifdef CONFIG_DYNAMIC_DEBUG
>>> +/*
>>> + * dht11_edges_print: show the data as actually received by the
>>> + *                    driver.
>>> + */
>>> +static void dht11_edges_print(struct dht11 *dht11)
>>> +{
>>> +	int i;
>>> +
>>> +	dev_dbg(dht11->dev, "%d edges detected:\n", dht11->num_edges);
>>> +	for (i = 1; i < dht11->num_edges; ++i) {
>>> +		dev_dbg(dht11->dev, "%d: %lld ns %s\n", i,
>>> +			dht11->edges[i].ts - dht11->edges[i - 1].ts,
>>> +			dht11->edges[i - 1].value ? "high" : "low");
>>> +	}
>>> +}
>>> +#endif /* CONFIG_DYNAMIC_DEBUG */
>> Hmm. I guess the ifdef is to avoid having the overhead in all cases.
>> I'd be more included to leave it out as the overhead isn't huge...
> 
> Actually my reason was that this particular function is very chatty
> writing more than 80 lines of output. This can easily push more important
> lines out of the ring buffer. So I thought an easy way to turn it off
> without recompiling the kernel is appropriate.
>  
>> Also it's more than possible people will want to use traditional
>> non dynamic debug for this...
> 
> I don't expect so, but I entirely rely on your judgement in this case.
> What do you want to have there instead?
Not sure there is a good solution to this.  The issues is accidental enabling
of the function by setting DEBUG as well as the dynamic config option as that
will make all the dynamic stuff default to on.  You can turn it off explicitly
but it seems unlikely that will be done by users...

I guess perhaps your solution is the best one though it's not exactly
a tidy solution!

Jonathan
> 
> Thanks,
> Harald
> 
>>> +
>>>  static unsigned char dht11_decode_byte(char *bits)
>>>  {
>>>  	unsigned char ret = 0;
>>> @@ -119,8 +137,12 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>>>  		t = dht11->edges[offset + 2 * i + 2].ts -
>>>  			dht11->edges[offset + 2 * i + 1].ts;
>>> -		if (!dht11->edges[offset + 2 * i + 1].value)
>>> -			return -EIO;  /* lost synchronisation */
>>> +		if (!dht11->edges[offset + 2 * i + 1].value) {
>>> +			dev_dbg(dht11->dev,
>>> +				"lost synchronisation at edge %d\n",
>>> +				offset + 2 * i + 1);
>>> +			return -EIO;
>>> +		}
>>>  		bits[i] = t > DHT11_THRESHOLD ? 1 : 0;
>>>  	}
>>>  
>>> @@ -130,8 +152,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>>  	temp_dec = dht11_decode_byte(&bits[24]);
>>>  	checksum = dht11_decode_byte(&bits[32]);
>>>  
>>> -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>>> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
>>> +		dev_dbg(dht11->dev, "invalid checksum\n");
>>>  		return -EIO;
>>> +	}
>>>  
>>>  	dht11->timestamp = ktime_get_real_ns();
>>>  	if (hum_int < 20) {  /* DHT22 */
>>> @@ -182,6 +206,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>>  	mutex_lock(&dht11->lock);
>>>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
>>>  		timeres = ktime_get_resolution_ns();
>>> +		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
>>>  		if (timeres > DHT11_MIN_TIMERES) {
>>>  			dev_err(dht11->dev, "timeresolution %dns too low\n",
>>>  				timeres);
>>> @@ -219,10 +244,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>>  
>>>  		free_irq(dht11->irq, iio_dev);
>>>  
>>> +#ifdef CONFIG_DYNAMIC_DEBUG
>>> +		dht11_edges_print(dht11);
>>> +#endif
>>> +
>>>  		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
>>> -			dev_err(&iio_dev->dev,
>>> -				"Only %d signal edges detected\n",
>>> -					dht11->num_edges);
>>> +			dev_err(dht11->dev, "Only %d signal edges detected\n",
>>> +				dht11->num_edges);
>>>  			ret = -ETIMEDOUT;
>>>  		}
>>>  		if (ret < 0)
>>>
>>
>