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

benpicco notifications at github.com
Wed Nov 4 16:54:10 CET 2020


@benpicco commented on this pull request.

Thank you for contribution!
I had a brief look, some comments:
 - why those static arrays and returning pointers? Just return the measured value, makes the API much simpler
 - no need for that `I2C_DEVNUM` and those loops - SAUL can handle multiple devices just fine on it's own :wink: 
- please use `/* comments */` instead of `// comments` to comply with the coding style
- does the device have a proper name? 'HSC' sounds a bit too generic. Please rename the driver & functions accordingly. 

> +#define DEV_I2C      (dev->params.i2c_dev)
+#define DEV_ADDR     (dev->params.i2c_addr)
+
+// Internal function prototypes
+// Reading raw (bit) value.  For faster reading, can be removed
+static int _read_ut(const hsc_t *dev, int32_t *ut);
+static int _read_up(const hsc_t *dev, int32_t *up);
+
+/*---------------------------------------------------------------------------*
+ *                          HSC Core API                                     *
+ *---------------------------------------------------------------------------*/
+
+int hsc_init(hsc_t *dev, const hsc_params_t *params)
+{
+    dev->params = *params;
+    uint8_t buf[HSC_FULL_DATA_LENGTH*I2C_DEVNUM] = {0};

Why multiply by `I2C_DEVNUM`?

> + * @author      Quang Pham
+ *
+ * @}
+ */
+
+#include "math.h"
+#include "hsc.h"
+#include "hsc_internals.h"
+#include "hsc_params.h"
+#include "xtimer.h"
+#include "periph/i2c.h"
+
+#define ENABLE_DEBUG        (0)
+#include "debug.h"
+
+#define I2C_DEVNUM          (1)

What is this used for? 

> +    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, &buf[0], HSC_FULL_DATA_LENGTH, 0) < 0) {
+        i2c_release(DEV_I2C);
+        DEBUG("[hsc] read_bytes fails\n");
+
+        return -1;
+    }
+
+    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;
+            break;

No need to `break` after `return` :wink: 

> +        case HSC_STATUS_OK:
+            return  0;
+            break;
+        case HSC_STATUS_STALE_DATA:
+            return -1;
+            break;
+        default:
+            return -2;
+    }
+
+    return 0;
+}
+
+int16_t *hsc_read_temperature(const hsc_t *dev)
+{
+    int32_t ut[I2C_DEVNUM] = {0};

```suggestion
    int32_t ut;
```

> +    }
+
+    return 0;
+}
+
+int16_t *hsc_read_temperature(const hsc_t *dev)
+{
+    int32_t ut[I2C_DEVNUM] = {0};
+    static int16_t tt[I2C_DEVNUM];
+
+    // Read uncompensated temperature value
+    _read_ut(dev, &ut[0]);
+
+    // Compute true temperature value following datasheet formulas
+    for (int i = 0; i < I2C_DEVNUM; i++) {
+        tt[i] = (int16_t)(ut[i] * 200 / 2047 - 50);

```suggestion
        ut = (int16_t)(ut * 200 / 2047 - 50);
```

> +            return -2;
+    }
+
+    return 0;
+}
+
+int16_t *hsc_read_temperature(const hsc_t *dev)
+{
+    int32_t ut[I2C_DEVNUM] = {0};
+    static int16_t tt[I2C_DEVNUM];
+
+    // Read uncompensated temperature value
+    _read_ut(dev, &ut[0]);
+
+    // Compute true temperature value following datasheet formulas
+    for (int i = 0; i < I2C_DEVNUM; i++) {

What is this loop for?

> +}
+
+int16_t *hsc_read_temperature(const hsc_t *dev)
+{
+    int32_t ut[I2C_DEVNUM] = {0};
+    static int16_t tt[I2C_DEVNUM];
+
+    // Read uncompensated temperature value
+    _read_ut(dev, &ut[0]);
+
+    // Compute true temperature value following datasheet formulas
+    for (int i = 0; i < I2C_DEVNUM; i++) {
+        tt[i] = (int16_t)(ut[i] * 200 / 2047 - 50);
+    }
+
+    return (int16_t*)tt;

```suggestion
    return ut;
```

> +/*
+ * 
+ */
+

```suggestion
```

> @@ -0,0 +1,166 @@
+/*
+ * 
+ */
+
+/**
+ * @ingroup     drivers_hsc

Surely the part is not just called HSC - does it have a part number?

-- 
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-523482204
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201104/cad11c5a/attachment-0001.htm>


More information about the notifications mailing list