* [PATCHv2 2/3] iio: dht11: Simplify decoding algorithm
2016-01-17 16:13 [PATCHv2 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits Harald Geyer
@ 2016-01-17 16:13 ` Harald Geyer
2016-01-24 16:59 ` Jonathan Cameron
2016-01-17 16:13 ` [PATCHv2 3/3] iio: dht11: Improve logging Harald Geyer
2016-01-24 16:58 ` [PATCHv2 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits Jonathan Cameron
2 siblings, 1 reply; 8+ messages in thread
From: Harald Geyer @ 2016-01-17 16:13 UTC (permalink / raw)
To: linux-iio, Jonathan Cameron, Jonathan Bell
Cc: Richard Weinberger, 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>
---
changes since V1:
* Simplify overly explicit code - no real change
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..96185f8 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;
}
- 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCHv2 2/3] iio: dht11: Simplify decoding algorithm
2016-01-17 16:13 ` [PATCHv2 2/3] iio: dht11: Simplify decoding algorithm Harald Geyer
@ 2016-01-24 16:59 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2016-01-24 16:59 UTC (permalink / raw)
To: Harald Geyer, linux-iio, Jonathan Bell; +Cc: Richard Weinberger
On 17/01/16 16:13, 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>
Applied to the togreg branch of iio.git - initially pushed out as testing
for the autobuilders to play with it.
Thanks,
Jonathan
> ---
> changes since V1:
> * Simplify overly explicit code - no real change
>
> 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..96185f8 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;
> }
>
> - 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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2 3/3] iio: dht11: Improve logging
2016-01-17 16:13 [PATCHv2 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits Harald Geyer
2016-01-17 16:13 ` [PATCHv2 2/3] iio: dht11: Simplify decoding algorithm Harald Geyer
@ 2016-01-17 16:13 ` Harald Geyer
2016-01-24 16:59 ` Jonathan Cameron
2016-01-24 16:58 ` [PATCHv2 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits Jonathan Cameron
2 siblings, 1 reply; 8+ messages in thread
From: Harald Geyer @ 2016-01-17 16:13 UTC (permalink / raw)
To: linux-iio, Jonathan Cameron, Jonathan Bell
Cc: Richard Weinberger, 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>
---
changes since V1:
* rebased onto the updated series
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 96185f8..4992e8c 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;
}
@@ -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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCHv2 3/3] iio: dht11: Improve logging
2016-01-17 16:13 ` [PATCHv2 3/3] iio: dht11: Improve logging Harald Geyer
@ 2016-01-24 16:59 ` Jonathan Cameron
2016-04-10 15:22 ` Harald Geyer
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2016-01-24 16:59 UTC (permalink / raw)
To: Harald Geyer, linux-iio, Jonathan Bell; +Cc: Richard Weinberger
On 17/01/16 16:13, 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>
Applied.
Thanks,
Jonathan
> ---
> changes since V1:
> * rebased onto the updated series
>
> 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 96185f8..4992e8c 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;
> }
>
> @@ -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)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2 3/3] iio: dht11: Improve logging
2016-01-24 16:59 ` Jonathan Cameron
@ 2016-04-10 15:22 ` Harald Geyer
2016-04-10 16:40 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Harald Geyer @ 2016-04-10 15:22 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Jonathan Bell, Richard Weinberger
Hi Jonathan!
Jonathan Cameron writes:
> On 17/01/16 16:13, 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>
> Applied.
I can't find this patch in any repository (while the other patches of the
series are there). Did I miss a reason this was dropped or is this an
oversightr? Should I resend this patch?
TIA,
Harald
=
> Thanks,
> =
> Jonathan
> > ---
> > changes since V1:
> > * rebased onto the updated series
> > =
> > 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 96185f8..4992e8c 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 =3D 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 =3D 0;
> > @@ -119,8 +137,12 @@ static int dht11_decode(struct dht11 *dht11, int =
offset)
> > for (i =3D 0; i < DHT11_BITS_PER_READ; ++i) {
> > t =3D 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] =3D t > DHT11_THRESHOLD;
> > }
> > =
> > @@ -130,8 +152,10 @@ static int dht11_decode(struct dht11 *dht11, int =
offset)
> > temp_dec =3D dht11_decode_byte(&bits[24]);
> > checksum =3D dht11_decode_byte(&bits[32]);
> > =
> > - if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) !=3D checksum=
)
> > + if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) !=3D checksum=
) {
> > + dev_dbg(dht11->dev, "invalid checksum\n");
> > return -EIO;
> > + }
> > =
> > dht11->timestamp =3D 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 =3D 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_de=
v,
> > =
> > free_irq(dht11->irq, iio_dev);
> > =
> > +#ifdef CONFIG_DYNAMIC_DEBUG
> > + dht11_edges_print(dht11);
> > +#endif
> > +
> > if (ret =3D=3D 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 =3D -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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCHv2 3/3] iio: dht11: Improve logging
2016-04-10 15:22 ` Harald Geyer
@ 2016-04-10 16:40 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2016-04-10 16:40 UTC (permalink / raw)
To: Harald Geyer; +Cc: linux-iio, Jonathan Bell, Richard Weinberger
On 10/04/16 16:22, Harald Geyer wrote:
> Hi Jonathan!
>
> Jonathan Cameron writes:
>> On 17/01/16 16:13, 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>
>> Applied.
>
> I can't find this patch in any repository (while the other patches of the
> series are there). Did I miss a reason this was dropped or is this an
> oversightr? Should I resend this patch?
Hi Harald,
Not sure where this disappeared to. Anyhow, I've just applied it (with a spot of fuzz)
to the togreg branch of iio.git, currently pushed out as testing for the autobuilders to
play with it.
Sorry about that.
Jonathan
>
> TIA,
> Harald
>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> changes since V1:
>>> * rebased onto the updated series
>>>
>>> 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 96185f8..4992e8c 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;
>>> }
>>>
>>> @@ -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)
>>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits
2016-01-17 16:13 [PATCHv2 1/3] iio: dht11: Improve reliability - be more tolerant about missing start bits Harald Geyer
2016-01-17 16:13 ` [PATCHv2 2/3] iio: dht11: Simplify decoding algorithm Harald Geyer
2016-01-17 16:13 ` [PATCHv2 3/3] iio: dht11: Improve logging Harald Geyer
@ 2016-01-24 16:58 ` Jonathan Cameron
2 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2016-01-24 16:58 UTC (permalink / raw)
To: Harald Geyer, linux-iio, Jonathan Bell; +Cc: Richard Weinberger
On 17/01/16 16:13, 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>
As there are no signs of other testing going on and you have clearly
hammered this reasonably hard, I'm applying it now. It'll be in my
tree until next weekend at the earliest if anyone has anything to add.
Applied to the togreg branch of iio.git - initially pushed out as
testing for the autobuilders to play with it.
Thanks,
Jonathan
> ---
> No changes since V1, but compile tested the entire series patch by patch
> additionally to test the whole series with DHT11 and DHT22 on an
> imx233-olinuxino board.
>
> 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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread