[riot-notifications] [RIOT-OS/RIOT] drivers: add gp2y10xx dust sensor (#15445)

benpicco notifications at github.com
Sun Nov 15 21:43:57 CET 2020


@benpicco commented on this pull request.

looks good, appears to be a pretty simple device. 

> +     * TODO: this line should be rewritten in a way that floating point
+     * isn't used */
+    *density = (int16_t)((float)voltage * CONVERSION_RATIO);

so why not

```suggestion
    *density = (voltage * 2) / 10;
```

> +    /* turn ILED on/off and wait a little bit to read the ADC */
+    _iled_on(dev);
+    xtimer_usleep(ILED_PULSE_WAIT_US);
+    if ((adc_value = adc_sample(dev->params.aout, dev->params.adc_res)) < 0) {
+        _iled_off(dev);
+        return GP2Y10XX_ERR_ADC;
+    }
+    _iled_off(dev);
+
+    DEBUG("[gp2y10xx] read: unfiltered adc_value=%"PRIi32"\n", adc_value);
+    adc_value = _adc_filter(dev, adc_value);
+    DEBUG("[gp2y10xx] read: filtered adc_value=%"PRIi32"\n", adc_value);
+
+    /* convert ADC reading to a voltage */
+    voltage = (dev->params.vref * adc_value);
+    voltage /= _adc_resolution(dev->params.adc_res);

```suggestion
    voltage >>= _adc_resolution(dev->params.adc_res);
```

> +        case ADC_RES_12BIT:
+            exp = 12;
+            break;
+        case ADC_RES_14BIT:
+            exp = 14;
+            break;
+        case ADC_RES_16BIT:
+            exp = 16;
+            break;
+
+        default:
+            assert(0);
+            break;
+    }
+
+    return (1 << exp);

```suggestion
    return exp;
```

you can just do a shift instead of a division by a power of two

> +    if (adc_init(dev->params.aout) < 0) {
+        return GP2Y10XX_ERR_ADC;
+    }
+
+    if (gpio_init(dev->params.iled_pin, GPIO_OUT) < 0) {
+        return GP2Y10XX_ERR_ILED;
+    }
+    _iled_off(dev);
+
+    return GP2Y10XX_OK;
+}
+
+int gp2y10xx_read_density(gp2y10xx_t *dev, int16_t *density)
+{
+    int32_t adc_value;
+    int32_t voltage;

Can voltage ever be negative?

> +    if (dev->params.iled_level == GP2Y10XX_ILED_LEVEL_HIGH) {
+        gpio_set(dev->params.iled_pin);
+    }
+    else {
+        gpio_clear(dev->params.iled_pin);
+    }

```suggestion
    gpio_write(dev->params.iled_pin, dev->params.iled_level == GP2Y10XX_ILED_LEVEL_HIGH);
```

> +/* calculates the ADC sample value by filtering it with all
+ * previous ADC readings */
+static int32_t _adc_filter(gp2y10xx_t *dev, int32_t adc_line_value) {

Does this make sense?
The intervals in between which `gp2y10xx_read_density()` is called are entirely up to the application - so you might read some *very* old data here.

Wouldn't you rather want to do the averaging inside `gp2y10xx_read_density()` by taking multiple samples? 

> +    }
+    else {
+        printf("[Failed] res=%i\n", res);
+        return -1;
+    }
+
+    while (1) {
+        res = gp2y10xx_read_density(&dev, &density);
+        if (res == GP2Y10XX_OK) {
+            printf("Dust density %"PRIi16" ug/m3\n", density);
+        }
+        else {
+            printf("Error reading data. err: %d\n", res);
+        }
+
+        xtimer_usleep(SLEEP_USEC);

```suggestion
        xtimer_msleep(1);
```

> +    }
+    else {
+        printf("[Failed] res=%i\n", res);
+        return -1;
+    }
+
+    while (1) {
+        res = gp2y10xx_read_density(&dev, &density);
+        if (res == GP2Y10XX_OK) {
+            printf("Dust density %"PRIi16" ug/m3\n", density);
+        }
+        else {
+            printf("Error reading data. err: %d\n", res);
+        }
+
+        xtimer_usleep(SLEEP_USEC);

actually that will generate a lot of output - maybe rather use 250 ms here 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15445#pullrequestreview-530828436
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201115/6b0b0d9d/attachment.htm>


More information about the notifications mailing list