[riot-notifications] [RIOT-OS/RIOT] Driver for Honeywell HSC series pressure and temperature sensor (#15383)

[Marian Buschsieweke]

@maribu commented on this pull request.

Looks quite good. I have some more comments inline.

> +
+    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, buf, sizeof(buf), 0) < 0) {
+        i2c_release(DEV_I2C);
+        DEBUG("[hsc] read_bytes fails\n");
+
+        return -EIO;
+    }
+
+    i2c_release(DEV_I2C);
+
+    /* Check the status of the sensor */
+    uint8_t status = buf[0];
+
+    switch (status & HSC_STATUS_MASK) {
+    case HSC_STATUS_OK:
+        return 0;

```suggestion
        break;
```

The `return 0;` in line 73 is currently unreachable. You could either drop the `return 0;` there, or make it reachable by turning the `return 0;` here into a `break;`. I personally would prefer using the `break;` here, but I guess it is more a matter of personal taste.

> +        return 0;
+    case HSC_STATUS_STALE_DATA:
+        return -EAGAIN;
+    default:
+        return -EIO;
+    }
+
+    return 0;
+}
+
+int16_t hsc_read_temperature(const hsc_t *dev)
+{
+    int32_t ut = 0;
+
+    /* Read uncompensated temperature value */
+    _read_ut(dev, &ut);

The return value here is not used. As I2C is not a super reliable bus, it makes IMO sense to either propagate the error or handle it.

E.g. what about something like this (but with the comments of the original code):

```C
int hsc_read_temperature(const hsc_t *dev, int16_t *dest)
{
    int32_t ut;
    int retval;
    if ((retval = _read_ut(dev, &ut)) != 0) {
        return retval;
    }
    *dest = ut * 2000 / 2047 - 500;
    return 0;
}
```

> +    int32_t ut = 0;
+
+    /* Read uncompensated temperature value */
+    _read_ut(dev, &ut);
+
+    /* Formular from section 4.0 in 
+     * https://sensing.honeywell.com/i2c-comms-digital-output-pressure-sensors-tn-008201-3-en-final-30may12.pdf */
+    return (int16_t)(ut * 2000 / 2047 - 500);
+}
+
+int32_t hsc_read_pressure(const hsc_t *dev)
+{
+    int32_t up = 0;
+
+    /* Read uncompensated pressure value */
+    _read_up(dev, &up);

Here again, potential errors are not handled. However, as pressure cannot be negative, you can just return the error codes here. So any non-negative value would indicate that the return value is indeed pressure information, every negative value would indicate an error instead.

> +    return (int32_t)((up - 1638) * 2000 * HSC_PARAM_RANGE / \
+                     (14745 - 1638) - 1000 * HSC_PARAM_RANGE);

```suggestion
    return (up - 1638) * 2000 * HSC_PARAM_RANGE
           / (14745 - 1638) - 1000 * HSC_PARAM_RANGE);
```

In C a statement ends with a semicolon (;), not with the end of line. No need to escape the line break with the backslash (\). (A backslash is only needed for multi-line preprocessor statements.)

Also: As `up` is already `int32_t`, all the other values will be propagated to `int32_t` and the result will already be `int32_t`. So no need to cast here. In general: Explicit casts should be avoided when possible, as they disable type safe.

> @@ -0,0 +1,7 @@
+include ../Makefile.tests_common
+
+USEMODULE += hsc
+USEMODULE += xtimer
+FEATURES_REQUIRED += periph_i2c

```suggestion
```

The driver itself should require `periph_i2c`, so no need to ask for it again here.

> +    res->val[0] = (int16_t)(hsc_read_pressure(dev));
+    res->unit = UNIT_BAR;
+    res->scale = -6;

```suggestion
    res->unit = UNIT_BAR;
    res->scale = -6;
    int32_t value = hsc_read_pressure(dev);
    phydat_fit(res, &value, 1);
    return 1;
```

When you just cast an `int32_t` to an `int16_t`, data above 32.767 mBar will be cropped. However, `phydat_fit()` will instead adjust `res->scale` until the value in fits into 16 bit. It even does correct scientific rounding.

> @@ -0,0 +1,2 @@
+USEMODULE_INCLUDES_hsc := $(LAST_MAKEFILEDIR)/include
+USEMODULE_INCLUDES += $(USEMODULE_INCLUDES_hsc)

Please also add a `Makefile.dep` to your driver that contains:

```Makefile
FEATURES_REQUIRED += periph_i2c
```

-- 
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/15383#pullrequestreview-533497124
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201118/4da59f6c/attachment-0001.htm>