iio: humidity: dht11: Rewrite decode algorithm, improve read reliability

This driver has a poor read success rate when any amount of hard-IRQ latency is encountered. On the Raspberry Pi platform for example, the read success rate is ~50% with no hardware activity and practically 0% with any other significant hardware activity.

Rewrite several aspects of the decode mechanism to make it more robust:

- Cater for GPIO chips that can raise interrupts when I/O pins are set as output by expanding the maximum number of edges recorded.
- Start decoding the bitstream right-justified to make the driver agnostic towards whether the start bits generate IRQs.
- Only bother decoding for GPIO "high" bit-times - the high-time is what encodes the useful information.
- Use simple split-the-difference thresholding to decide if a received data bit is a "1" or "0", instead of comparing relative bit periods.
- Refine the start bit timing to use usleep_range() to stop the later sensor models from timing out if the msleep() takes longer than expected.

Also tweak a few error paths to be more informative.

The sensor decode success rate on a Pi after these changes is 99.1% with no other hardware activity and approximately 78% with an Ethernet ping flood happening concurrently.

There's still a few rough corners - several versions of the datasheet seem to suggest that a double-read is required to get an up-to-date value, i.e. the last value converted is the one returned when a readout is requested.

This can't be easily reconciled with the minimum repeat interval that is also specified - a 2 second round-trip time is a long time to block for.

As these sensors have a slow (~10s) response time to environment changes, it's reasonable to assume that a measured value from 2 seconds ago will not be inaccurate.

Signed-off-by Jonathan Bell <[email protected]>

---
I only have access to an AM2302/DHT22 variant of the sensor. This patch could use some testing on the other models in the range.

I am aware of a pending patch that causes the driver to bail out of a reading if the kernel's timestamp resolution is too large. With the change above, the driver won't be adversely affected unless the resolution is very coarse - ~50us per tick.

 drivers/iio/humidity/dht11.c |  157 +++++++++++++++++++++++-------------------
 1 file changed, 86 insertions(+), 71 deletions(-)

drivers/iio/humidity/dht11.c +79 -70

index 7d79a1a..855fb30 100644
@@ -38,22 +38,28 @@
 
 #define DRIVER_NAME    "dht11"
 
-#define DHT11_DATA_VALID_TIME    2000000000  /* 2s in ns */
-
-#define DHT11_EDGES_PREAMBLE 2
+#define DHT11_START_EDGES 2
+#define DHT11_ACK_EDGES 2
 #define DHT11_BITS_PER_READ 40
+#define DHT11_STOP_EDGES 2
+
+#define DHT11_MAX_EDGES (DHT11_START_EDGES + DHT11_ACK_EDGES + \
+            DHT11_BITS_PER_READ * 2 + DHT11_STOP_EDGES)
+
 /*
- * Note that when reading the sensor actually 84 edges are detected, but
- * since the last edge is not significant, we only store 83:
+ * Sensor variants have differing requirements for the start pulse.
+ * DHT11 is minimum 18ms, DHT21 is 1-10ms, DHT22 is 1ms.
+ * If a >20ms start pulse is driven on a DHT22 then the sensor doesn't respond
+ * at all. Stick to the minimum that should work for all variants.
  */
-#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
-
-/* Data transmission timing (nano seconds) */
-#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_START_PULSE_MIN    18000 /* us */
+#define DHT11_START_PULSE_MAX    20000 /* us */
+ /* Determinant for 1 or 0 decode. 27uS high-time for a 0, 70uS for a 1. */
+#define DHT11_BIT_THRESHOLD    51000 /* ns */
+/* Limit repeat interval. Datasheet specifies minimum 2s interval */
+#define DHT11_REPEAT_INTERVAL    2000000000 /* ns */
+/* This is approx. 2x maximum readout time */
+#define DHT11_CONV_TIMEOUT    20 /* ms */
 
 struct dht11 {
     struct device            *dev;

@@ -70,10 +76,10 @@ struct dht11 {
 
     /* num_edges: -1 means "no transmission in progress" */
     int                num_edges;
-    struct {s64 ts; int value; }    edges[DHT11_EDGES_PER_READ];
+    struct {s64 ts; int value; }    edges[DHT11_MAX_EDGES];
 };
 
-static unsigned char dht11_decode_byte(int *timing, int threshold)
+static unsigned char dht11_decode_byte(s64 *timing, int threshold)
 {
     unsigned char ret = 0;
     int i;

@@ -87,46 +93,55 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
     return ret;
 }
 
-static int dht11_decode(struct dht11 *dht11, int offset)
+static int dht11_decode(struct dht11 *dht11)
 {
-    int i, t, timing[DHT11_BITS_PER_READ], threshold,
-        timeres = DHT11_SENSOR_RESPONSE;
+    int i, t, offset;
+    s64 timing[DHT11_BITS_PER_READ];
     unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
-    /* Calculate timestamp resolution */
-    for (i = 1; i < dht11->num_edges; ++i) {
-        t = dht11->edges[i].ts - dht11->edges[i-1].ts;
-        if (t > 0 && t < timeres)
-            timeres = t;
-    }
-    if (2*timeres > DHT11_DATA_BIT_HIGH) {
-        pr_err("dht11: timeresolution %d too bad for decoding\n",
-            timeres);
+    dht11->timestamp = iio_get_time_ns();
+    /*
+     * Start our decode taking into account that some GPIO chips
+     * won't interrupt when we drive the start bits.
+     */
+    offset = dht11->num_edges - DHT11_STOP_EDGES - DHT11_BITS_PER_READ * 2;
+    if (offset < 0) {
+        dev_err(dht11->dev,
+            "Decode failure: only %d edges recorded\n",
+            dht11->num_edges);
         return -EIO;
     }
-    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;
+    /* Check that our pulse train hasn't been corrupted by latency */
+    for (i = 0; i < dht11->num_edges - 1; i++) {
+        if (dht11->edges[i].value == dht11->edges[i + 1].value) {
+            dev_err(dht11->dev,
+                "Decode failure: missed edges (%d total)\n",
+                dht11->num_edges);
+            return -EIO;
+        }
+    }
+    /* Find pairs of edges that correspond to a high interval */
+    for (i = offset, t = 0; i < dht11->num_edges - DHT11_STOP_EDGES; i++) {
+        if (dht11->edges[i].value == 1 &&
+                dht11->edges[i + 1].value == 0) {
+            if (t == DHT11_BITS_PER_READ)
+                break;
+            timing[t++] = dht11->edges[i + 1].ts -
+                    dht11->edges[i].ts;
+        }
     }
 
-    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(timing, DHT11_BIT_THRESHOLD);
+    hum_dec = dht11_decode_byte(&timing[8], DHT11_BIT_THRESHOLD);
+    temp_int = dht11_decode_byte(&timing[16], DHT11_BIT_THRESHOLD);
+    temp_dec = dht11_decode_byte(&timing[24], DHT11_BIT_THRESHOLD);
+    checksum = dht11_decode_byte(&timing[32], DHT11_BIT_THRESHOLD);
 
-    if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+    if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
+        dev_err(dht11->dev, "Decode failure: invalid checksum\n");
         return -EIO;
+    }
 
-    dht11->timestamp = iio_get_time_ns();
     if (hum_int < 20) {  /* DHT22 */
         dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
                     ((temp_int & 0x80) ? -100 : 100);

@@ -153,12 +168,13 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
     struct dht11 *dht11 = iio_priv(iio);
 
     /* TODO: Consider making the handler safe for IRQ sharing */
-    if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
+    if (dht11->num_edges < DHT11_MAX_EDGES && dht11->num_edges >= 0) {
         dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
         dht11->edges[dht11->num_edges++].value =
                         gpio_get_value(dht11->gpio);
-
-        if (dht11->num_edges >= DHT11_EDGES_PER_READ)
+        /* Ensure other cores can see our array update */
+        smp_mb();
+        if (dht11->num_edges >= DHT11_MAX_EDGES)
             complete(&dht11->completion);
     }
 

@@ -173,42 +189,35 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
     int ret;
 
     mutex_lock(&dht11->lock);
-    if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
+    if (dht11->timestamp + DHT11_REPEAT_INTERVAL < iio_get_time_ns()) {
         reinit_completion(&dht11->completion);
 
         dht11->num_edges = 0;
+        /* Ensure the IRQ handler knows we've started a reading */
+        smp_mb();
+
         ret = gpio_direction_output(dht11->gpio, 0);
         if (ret)
             goto err;
-        msleep(DHT11_START_TRANSMISSION);
-        ret = gpio_direction_input(dht11->gpio);
-        if (ret)
-            goto err;
 
-        ret = request_irq(dht11->irq, dht11_handle_irq,
-                  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-                  iio_dev->name, iio_dev);
+        usleep_range(DHT11_START_PULSE_MIN, DHT11_START_PULSE_MAX);
+
+        ret = gpio_direction_input(dht11->gpio);
         if (ret)
             goto err;
 
         ret = wait_for_completion_killable_timeout(&dht11->completion,
-                                 HZ);
-
-        free_irq(dht11->irq, iio_dev);
-
-        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);
-            ret = -ETIMEDOUT;
-        }
+                    msecs_to_jiffies(DHT11_CONV_TIMEOUT));
+        /*
+         * If the completion call times out, it could be the case that
+         * the IRQ handler may have reported data which could contain
+         * sufficient recorded edges to achieve decode.
+         * Test based on the number of edges instead of timeout.
+         */
         if (ret < 0)
             goto err;
 
-        ret = dht11_decode(dht11,
-                dht11->num_edges == DHT11_EDGES_PER_READ ?
-                    DHT11_EDGES_PREAMBLE :
-                    DHT11_EDGES_PREAMBLE - 2);
+        ret = dht11_decode(dht11);
         if (ret)
             goto err;
     }

This driver has a poor read success rate when any amount of hard-IRQ latency is encountered. On the Raspberry Pi platform for example, the read success rate is ~50% with no hardware activity and practically 0% with any other significant hardware activity.

Rewrite several aspects of the decode mechanism to make it more robust:

- Cater for GPIO chips that can raise interrupts when I/O pins are set as output by expanding the maximum number of edges recorded.
- Start decoding the bitstream right-justified to make the driver

towards whether the start bits generate IRQs.

- Only bother decoding for GPIO "high" bit-times - the high-time is what encodes the useful information.
- Use simple split-the-difference thresholding to decide if a received data bit is a "1" or "0", instead of comparing relative bit periods.

- Refine the start bit timing to use usleep_range() to stop the later sensor models from timing out if the msleep() takes longer than expected.

Also tweak a few error paths to be more informative.

The sensor decode success rate on a Pi after these changes is 99.1% with no other hardware activity and approximately 78% with an Ethernet ping flood happening concurrently.

There's still a few rough corners - several versions of the datasheet

to suggest that a double-read is required to get an up-to-date value,

the last value converted is the one returned when a readout is

This can't be easily reconciled with the minimum repeat interval that is also specified - a 2 second round-trip time is a long time to block for.

As these sensors have a slow (~10s) response time to environment

it's reasonable to assume that a measured value from 2 seconds ago will not be inaccurate.

Signed-off-by Jonathan Bell <[email protected]>

---
I only have access to an AM2302/DHT22 variant of the sensor. This patch could use some testing on the other models in the range.

I am aware of a pending patch that causes the driver to bail out of a reading if the kernel's timestamp resolution is too large. With the

above, the driver won't be adversely affected unless the resolution is very coarse - ~50us per tick.

  drivers/iio/humidity/dht11.c |  157  
+++++++++++++++++++++++-------------------
  1 file changed, 86 insertions(+), 71 deletions(-)

drivers/iio/humidity/dht11.c +0 -3

index 7d79a1a..855fb30 100644
@@ -38,22 +38,28 @@
 
  #define DRIVER_NAME	"dht11"
 
-#define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
-
-#define DHT11_EDGES_PREAMBLE 2

drivers/iio/humidity/dht11.c +3 -1

- * since the last edge is not significant, we only store 83:
+ * Sensor variants have differing requirements for the start pulse.
+ * DHT11 is minimum 18ms, DHT21 is 1-10ms, DHT22 is 1ms.
+ * If a >20ms start pulse is driven on a DHT22 then the sensor doesn't 
 

respond
+ * at all. Stick to the minimum that should work for all variants. */
-#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)

*/

drivers/iio/humidity/dht11.c +10 -10

+#define DHT11_BIT_THRESHOLD	51000 /* ns */
+/* Limit repeat interval. Datasheet specifies minimum 2s interval */
+#define DHT11_REPEAT_INTERVAL	2000000000 /* ns */
+/* This is approx. 2x maximum readout time */
+#define DHT11_CONV_TIMEOUT	20 /* ms */
 
  struct dht11 {
  	struct device			*dev;

@@ -70,10 +76,10 @@ struct dht11 {
 
  	/* num_edges: -1 means "no transmission in progress" */
  	int				num_edges;
-	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
+	struct {s64 ts; int value; }	edges[DHT11_MAX_EDGES];
  };
 
-static unsigned char dht11_decode_byte(int *timing, int threshold)
+static unsigned char dht11_decode_byte(s64 *timing, int threshold)
  {
  	unsigned char ret = 0;
  	int i;

@@ -87,46 +93,55 @@ static unsigned char dht11_decode_byte(int *timing, 
 
 int threshold)
  	return ret;
  }
 
-static int dht11_decode(struct dht11 *dht11, int offset)
+static int dht11_decode(struct dht11 *dht11)
  {
-	int i, t, timing[DHT11_BITS_PER_READ], threshold,
-		timeres = DHT11_SENSOR_RESPONSE;
+	int i, t, offset;
+	s64 timing[DHT11_BITS_PER_READ];
  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
-	/* Calculate timestamp resolution */
-	for (i = 1; i < dht11->num_edges; ++i) {
-		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
-		if (t > 0 && t < timeres)
-			timeres = t;

drivers/iio/humidity/dht11.c +5 -11

+	if (offset < 0) {
+		dev_err(dht11->dev,
+			"Decode failure: only %d edges recorded\n",
+			dht11->num_edges);
  		return -EIO;
  	}
-	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;
+	/* Check that our pulse train hasn't been corrupted by latency */

drivers/iio/humidity/dht11.c +15 -7

+		if (dht11->edges[i].value == 1 &&
+				dht11->edges[i + 1].value == 0) {
+			if (t == DHT11_BITS_PER_READ)
+				break;
+			timing[t++] = dht11->edges[i + 1].ts -
+					dht11->edges[i].ts;
+		}
  	}
 
-	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(timing, DHT11_BIT_THRESHOLD);
+	hum_dec = dht11_decode_byte(&timing[8], DHT11_BIT_THRESHOLD);
+	temp_int = dht11_decode_byte(&timing[16], DHT11_BIT_THRESHOLD);
+	temp_dec = dht11_decode_byte(&timing[24], DHT11_BIT_THRESHOLD);
+	checksum = dht11_decode_byte(&timing[32], DHT11_BIT_THRESHOLD);
 
-	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
+		dev_err(dht11->dev, "Decode failure: invalid checksum\n");
  		return -EIO;
+	}
 
-	dht11->timestamp = iio_get_time_ns();
  	if (hum_int < 20) {  /* DHT22 */

  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
  					((temp_int & 0x80) ? -100 : 100);

drivers/iio/humidity/dht11.c +7 -1

  		return -EINVAL;
  	}
 
-	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
+	dht11->timestamp = iio_get_time_ns() - DHT11_REPEAT_INTERVAL - 1;
  	dht11->num_edges = -1;
 
+	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			pdev->name, iio);
+	if (ret)
+		return ret;
+
  	platform_set_drvdata(pdev, iio);
 
  	init_completion(&dht11->completion);

Quoted 3 lines · Jonathan Bell

- Cater for GPIO chips that can raise interrupts when I/O pins are set as output by expanding the maximum number of edges recorded.
- Start decoding the bitstream right-justified to make the driver

agnostic

Quoted 1 line

towards whether the start bits generate IRQs.

That was the original behaviour, but is actively prohibited by some GPIO chip drivers. We asked Linus Walleij about it in
https://lkml.org/lkml/2014/12/1/171 who replied:
"IRQs on pins set to output *does* *not* *make* *sense*." I still disagree,

Quoted 6 lines · On Mon, Jul 6, 2015 at 1:39 AM, <[email protected]> wrote:

Quoted 2 lines · Jonathan Bell

- Cater for GPIO chips that can raise interrupts when I/O pins are set as output by expanding the maximum number of edges

Quoted 5 lines

Quoted 1 line

- Start decoding the bitstream right-justified to make the driver

agnostic

Quoted 1 line

towards whether the start bits generate IRQs.

That was the original behaviour, but is actively prohibited by some

Quoted 4 lines

chip drivers. We asked Linus Walleij about it in
https://lkml.org/lkml/2014/12/1/171 who replied:
"IRQs on pins set to output *does* *not* *make* *sense*." I still disagree,

I just don't understand this usecase. Can you explain to me what you are doing and why exactly so as to soften me up a bit?

Quoted 3 lines

I just don't understand this usecase. Can you explain to me what you are doing and why exactly so as to soften me up a bit?

Sure. The HW setup where this makes most sense is if the GPIO pin has a fixed drive strength (say 4mA) and is connected to some pullup or pulldown
resistor - maybe on the client device.

In this case there are two uses for getting interrupts on our own output edges:
1) We get feedback if we successfully pulled the line up/down.

2) If we get interrupts on output edges, timestamps of all signal edges can
be taken via the same codepath and thus be slightly more accurate. This probably is not important, but it is a nice thing to support.

Also, if we get spurious interrupts before receiving the actual data, there
is less likelyhood of cache misses during reading the data, which might improve reliability.

Quoted 8 lines · On Mon, Jul 6, 2015 at 10:02 AM, <[email protected]> wrote:

Quoted 3 lines

I just don't understand this usecase. Can you explain to me what you are doing and why exactly so as to soften me up a bit?

Sure. The HW setup where this makes most sense is if the GPIO pin has a fixed drive strength (say 4mA) and is connected to some pullup or pulldown
resistor - maybe on the client device.

Pull-up or not doesn't matter I think. That is only there to stabilize the line if the output is set to something like high impedance. You must certainly do not want an IRQ because a pull-up/down is dragging your line past a trigger point, you still know what is going on.

As in my other response, what could possibly make sense is open drain/collector or open emitter/source.
https://en.wikipedia.org/wiki/Open_collector

I am open to patches handling IRQ events on GPIO descriptors flagged as open [drain/collector/emitter/source].

But then make sure this line is flagged as such in the machine descriptor / device tree.

Quoted 1 line

In this case there are two uses for getting interrupts on our own

Quoted 2 lines

edges:
1) We get feedback if we successfully pulled the line up/down.

Seems more apropriate to check by polling the line, possibly using a timer.

IRQ seems highly inapropriate since it is an expected event and it must have a timeout anyways.

Quoted 4 lines

2) If we get interrupts on output edges, timestamps of all signal edges can
be taken via the same codepath and thus be slightly more accurate. This probably is not important, but it is a nice thing to support.

This seems more like a debug aid than something that should be done in drivers.

I would think it is possibly something then that the driver can set up and handle internally using debugfs rather than exporting it to the gpiolib API.

Quoted 4 lines

Also, if we get spurious interrupts before receiving the actual data, there
is less likelyhood of cache misses during reading the data, which might improve reliability.

Isn't this usecase analogous to debouncing?

On Thu, 9 Jul 2015 12:59:16 +0200, Linus Walleij

Quoted 5 lines

Pull-up or not doesn't matter I think. That is only there to stabilize the line if the output is set to something like high impedance. You must certainly do not want an IRQ because a pull-up/down is dragging your line past a trigger point, you still know what is going on.

Okay, I can simplify my point actually:
If our GPIO pin is configured to use a drive strength of say 4mA and is connected to a device with drive strength >4mA we can always get signal edges during output. - Yes, this is not exactly what is going on with DHT11, but it is the most obvious example I can think of right now.

Quoted 9 lines

As in my other response, what could possibly make sense is open drain/collector or open emitter/source.
https://en.wikipedia.org/wiki/Open_collector

I am open to patches handling IRQ events on GPIO descriptors flagged as open [drain/collector/emitter/source].

But then make sure this line is flagged as such in the machine descriptor / device tree.

With the DTH11 the sensor is open collector but not the GPIO.

Quoted 1 line

Quoted 1 line

In this case there are two uses for getting interrupts on our own

output

Quoted 5 lines

Quoted 2 lines

edges:
1) We get feedback if we successfully pulled the line up/down.

Seems more apropriate to check by polling the line, possibly using a timer.

Yes, we could probably work around by polling, but that's an odd argument IMHO. That's a bit like "Don't bother with the FPU, we already know how to do floating point math in software".

Of course polling is ugly: Even if we only have to do it once using a timer, it makes it quite a bit more difficult to get the overall timing of the data transmission right.

Quoted 7 lines

Quoted 4 lines

2) If we get interrupts on output edges, timestamps of all signal edges can
be taken via the same codepath and thus be slightly more accurate. This probably is not important, but it is a nice thing to support.

This seems more like a debug aid than something that should be done in drivers.

As said it's nothing terribly important, but I don't see why accurate timestamps wouldn't be nice to everybody trying to decode a signal. The DHT11 driver needs timestamps on all input edges to work. The timestamps on output edges are mostly useful for debugging in this case. It could be more important with the next driver to be written and it is a nice thing to support anyway.

Quoted 3 lines

I would think it is possibly something then that the driver can set up and handle internally using debugfs rather than exporting it to the gpiolib API.

What do you have in mind? I don't get your point.

Quoted 5 lines

Quoted 4 lines

Also, if we get spurious interrupts before receiving the actual data, there
is less likelyhood of cache misses during reading the data, which might improve reliability.

Quoted 1 line

Isn't this usecase analogous to debouncing?

Ah, sorry. "spurious interrupts" probably has the wrong spin. What I was trying to say was: If we get interrupts on output edges, then the interrupt handler and data structures will already be in cache when we start receiving the important input edges. Thus making the data transmission more reliable. That's not a usecase of it's own. Just a nice sideeffekt.

I hope the above explains what I had in mind when I wrote dht11.c - since Richard changed it mongth ago to comply with your model, I have little urge to convice you. However I do wonder: Did you find any real bugs with this check againt interrupts on output? If yes, then maybe some GPIO_I_KNOW_I_M_DOING_SOMETHING_ODD flag would be the way to go.

Hi!

Thanks for looking into this driver. Actually I'm working actively (even if at a low pace) on cleaning up and improving the code, so I'd appreciate coordination of work. I currently have a patch under review, so you will need to rebase your patch once mine got merged.

Also you should split your patch into logical changes an grouping them into fixes, improvements and cleanups, so that I can ACK them individually.

Quoted 4 lines · Jonathan Bell

This driver has a poor read success rate when any amount of hard-IRQ latency is encountered. On the Raspberry Pi platform for example, the read success rate is ~50% with no hardware activity and practically 0% with any other significant hardware activity.

Actually I developed and tested this driver on a slow platform (imx233-olinuxino) and it worked quite well there, so I believe what really
fixes your problems is the added calls to smp_mb(), which you entirely forgot to mention in your summary below.

I admit I never tested my code on any multicore system and if decoding results are really that bad there, this is a serious bug. Obviously I'm not
competent about multiprocessing and can't review your solution, but if you can confirm, that just adding smp_mb() calls to the existing code fixes your problems, then you can add my Acked-By to such a patch and this should
be applied as fix.

About your other changes:

Quoted 5 lines

Rewrite several aspects of the decode mechanism to make it more robust:

- Cater for GPIO chips that can raise interrupts when I/O pins are set as output by expanding the maximum number of edges recorded.
- Start decoding the bitstream right-justified to make the driver

agnostic

Quoted 1 line

towards whether the start bits generate IRQs.

That was the original behaviour, but is actively prohibited by some GPIO chip drivers. We asked Linus Walleij about it in
https://lkml.org/lkml/2014/12/1/171 who replied:
"IRQs on pins set to output *does* *not* *make* *sense*." I still disagree,
but I don't feel like diving into discussions like this. I'll be happy if you can convince him, because the DHT11 driver lost a nice diagnostic feature with that change, but before that GPIO code is changed, there is no chance this can be added back.

Quoted 5 lines

- Only bother decoding for GPIO "high" bit-times - the high-time is what encodes the useful information.
- Use simple split-the-difference thresholding to decide if a received data bit is a "1" or "0", instead of comparing relative bit periods.

These changes are actually in my queue of cleanup patches. However selecting the right threshold is not quite trivial: There are the timing differences between sensor variants but there are also timing differences depending on cable length. I have DHT22s connected to cables up to 15 meters. You must be careful to select a threshold that really works with different impedances, etc.

Quoted 3 lines

- Refine the start bit timing to use usleep_range() to stop the later sensor models from timing out if the msleep() takes longer than expected.

Do you have any data (or even experiemental results) backing this change? The start bit timing is bothering me for a long time now: I never got any conclusive answer, what is best. However I never saw any problems obviously attributable to start bit time outs either. (Your change of course makes sense independent of the actual timing used.)

Quoted 1 line

Also tweak a few error paths to be more informative.

That's also on my todo list of cleanups. (And already begun with the patch currently under review.)

Quoted 3 lines

The sensor decode success rate on a Pi after these changes is 99.1% with no other hardware activity and approximately 78% with an Ethernet ping flood happening concurrently.

Actually if you used the information from the GPIO "low" timings for error correction, you probably could improve the ping flood case. However I'm not
sure if it is worth the effort and code complexity.

Quoted 1 line

There's still a few rough corners - several versions of the datasheet

seem

Quoted 1 line

to suggest that a double-read is required to get an up-to-date value,

i.e.

Quoted 1 line

the last value converted is the one returned when a readout is

requested.

This also is not clear to me. The datasheets seem to be translations above translations...

I'm not doing an actual review of your code before you split it into a "one patch per change" series. But I hope this feedback helps you anyway.

Quoted 3 lines · On Mon, 06 Jul 2015 00:39:53 +0100, <[email protected]> wrote:

Hi!

Thanks for looking into this driver. Actually I'm working actively

Quoted 3 lines

if at a low pace) on cleaning up and improving the code, so I'd appreciate
coordination of work. I currently have a patch under review, so you

Quoted 5 lines

need to rebase your patch once mine got merged.

Also you should split your patch into logical changes an grouping them into fixes, improvements and cleanups, so that I can ACK them individually.

This is a spare-time project for me.

I was aware that others were working
on it, hence the RFC with my changes so far.

Quoted 18 lines

Quoted 4 lines · Jonathan Bell

This driver has a poor read success rate when any amount of hard-IRQ latency is encountered. On the Raspberry Pi platform for example, the read success rate is ~50% with no hardware activity and practically 0% with any other significant hardware activity.

Actually I developed and tested this driver on a slow platform (imx233-olinuxino) and it worked quite well there, so I believe what really
fixes your problems is the added calls to smp_mb(), which you entirely forgot to mention in your summary below.

I admit I never tested my code on any multicore system and if decoding results are really that bad there, this is a serious bug. Obviously I'm not
competent about multiprocessing and can't review your solution, but if

Quoted 5 lines

you
can confirm, that just adding smp_mb() calls to the existing code fixes your problems, then you can add my Acked-By to such a patch and this should
be applied as fix.

The driver was just as unreliable on a Pi model B+ (single-core, ARM11)

it was on a B2 (quad-core A7).

The smp_mb() is necessary for multicore architectures that do not have a

snoop control unit on their cache controller. As it happens, the A7

unit *does* have one, but I was thinking about making the driver more robust on more platforms.

Quoted 2 lines

About your other changes:

Quoted 1 line

Rewrite several aspects of the decode mechanism to make it more

Quoted 3 lines

Quoted 3 lines

- Cater for GPIO chips that can raise interrupts when I/O pins are set as output by expanding the maximum number of edges

Quoted 5 lines

Quoted 1 line

- Start decoding the bitstream right-justified to make the driver

agnostic

Quoted 1 line

towards whether the start bits generate IRQs.

That was the original behaviour, but is actively prohibited by some

Quoted 5 lines

chip drivers. We asked Linus Walleij about it in
https://lkml.org/lkml/2014/12/1/171 who replied:
"IRQs on pins set to output *does* *not* *make* *sense*." I still disagree,
but I don't feel like diving into discussions like this. I'll be happy

Quoted 2 lines

you can convince him, because the DHT11 driver lost a nice diagnostic feature with that change, but before that GPIO code is changed, there

Quoted 1 line

no chance this can be added back.

For Linus's benefit:

These sensors are prodded by initially driving their data line low for a

long period (some ms) and then reverting the pin to an input to listen

their response. The first bit-edge arrives about 27us after reverting

pin to an input.

I moved the request_irq() back to the probe function originally because

the first edge was going missing - request_irq wasn't completing before

the first edge arrived in 27uS.

There is no electrical reason why GPIO outputs cannot also be interrupt

sources. The generation of interrupts from edge/level events simply samples the state of the pad and acts accordingly - it doesn't care

driving it (or at least shouldn't do). I'd be interested to see if

any GPIO hardware that gates its interrupt generation system with "am I

input?". If the GPIO API doesn't support this then OK, there may be reasons why (such as the locking referred to in the linked thread).

Rethinking the patch a bit, if we're now right-justifying our decode

we shouldn't care if we miss interrupts for the start bit or the first phase of the response, so we can work around it.

Quoted 2 lines

Quoted 2 lines

- Only bother decoding for GPIO "high" bit-times - the high-time is

Quoted 3 lines

Quoted 3 lines

encodes the useful information.
- Use simple split-the-difference thresholding to decide if a received data bit is a "1" or "0", instead of comparing relative bit

Quoted 3 lines

These changes are actually in my queue of cleanup patches. However selecting the right threshold is not quite trivial: There are the

Quoted 1 line

differences between sensor variants but there are also timing

Quoted 3 lines

depending on cable length. I have DHT22s connected to cables up to 15 meters. You must be careful to select a threshold that really works with different impedances, etc.

I couldn't see any significant differences in the datasheets I've looked

at (which I thought I had gathered for all the models out there). They

seemed to suggest 28/70uS timing.

It's true that for long wires the timings will vary due to the extra capacitance. In particular the rising edge generated by the pull-up will

be slower, meaning your high-bit timings get compressed.

Quoted 5 lines

Quoted 3 lines

- Refine the start bit timing to use usleep_range() to stop the later sensor models from timing out if the msleep() takes longer than expected.

Do you have any data (or even experiemental results) backing this

Quoted 4 lines

The start bit timing is bothering me for a long time now: I never got any conclusive answer, what is best. However I never saw any problems obviously attributable to start bit time outs either. (Your change of course makes sense independent of the actual timing used.)

If I put more process pressure on the system (i.e. more task switches),

the start bit sometimes took an extra jiffy to complete. I would only

2 edges back, which I am assuming either a) the sensor doesn't respond,

b) the sensor responds but we trample the signal by holding the line

I've not tested the timing with a scope since the change, but the no-response failure mode I was seeing has disappeared.

Maybe the driver needs a few more (optional) DT properties. Since the start-bit varies so widely between models, perhaps a few more compatible

strings to tune start-bit length and a "wire-delay" parameter to tweak detection thresholds?

Quoted 7 lines

Quoted 1 line

Also tweak a few error paths to be more informative.

That's also on my todo list of cleanups. (And already begun with the patch
currently under review.)

Quoted 1 line

The sensor decode success rate on a Pi after these changes is 99.1%

Quoted 8 lines

Quoted 2 lines

no other hardware activity and approximately 78% with an Ethernet ping flood happening concurrently.

Actually if you used the information from the GPIO "low" timings for error
correction, you probably could improve the ping flood case. However I'm not
sure if it is worth the effort and code complexity.

How so? I am using all the timing information available (each recorded edge) to generate my decode stream. Interrupt latency has the effect of

smearing out the received timings into a sort-of Gaussian distribution.

Splitting the difference for the high-time seemed the most sensible

The majority of decode failures I get now are due to coalesced edges - i.e. sampling the pin after an entire low- or high-time has passed,

we can't do much about. The only extension I could see that wouldn't affect the complexity too much would be to infer the timing from the

received either side of the "missing" sample - but if our hard-IRQ

affects those as well, then this gets tricky.

Of course userspace could just try again in 2 seconds...

Quoted 9 lines

That was the original behaviour, but is actively prohibited by some GPIO chip drivers. We asked Linus Walleij about it in
https://lkml.org/lkml/2014/12/1/171 who replied:
"IRQs on pins set to output *does* *not* *make* *sense*." I still disagree,
but I don't feel like diving into discussions like this. I'll be happy if you can convince him, because the DHT11 driver lost a nice diagnostic feature with that change, but before that GPIO code is changed, there is no chance this can be added back.

These sensors are prodded by initially driving their data line low for a long period (some ms) and then reverting the pin to an input to listen to their response. The first bit-edge arrives about 27us after reverting the pin to an input.

I moved the request_irq() back to the probe function originally because the first edge was going missing - request_irq wasn't completing before the first edge arrived in 27uS.

There is no electrical reason why GPIO outputs cannot also be interrupt sources. The generation of interrupts from edge/level events simply samples the state of the pad and acts accordingly - it doesn't care what's driving it (or at least shouldn't do). I'd be interested to see if there's any GPIO hardware that gates its interrupt generation system with "am I an input?". If the GPIO API doesn't support this then OK, there may be reasons why (such as the locking referred to in the linked thread).

Rethinking the patch a bit, if we're now right-justifying our decode then we shouldn't care if we miss interrupts for the start bit or the first phase of the response, so we can work around it.