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

[benpicco]

@benpicco commented on this pull request.

Yea that looks better, but I'm a bit confused by the order of the bytes. 

What is the output of `tests/driver_hsc` ?

> +#define HSC_ADDR                    (0x28)   /* 7 bit address */
+/** @} */
+
+/**
+ * @name    HSC Max Range
+ * @{
+ */
+#define HSC_RANGE                   (40U)    /* in mbar dimension */
+/** @} */
+
+/**
+ * @name    HSC I2C Packet Readout
+ * @{
+ */
+#define HSC_PRESSURE_DATA_LENGTH    (2)      /* Pressure is stored in the first 2 bytes of data */
+#define HSC_FULL_DATA_LENGTH        (4)	     /* Pressure + temperature data is 4 bytes long */

```suggestion
#define HSC_FULL_DATA_LENGTH        (4)      /* Pressure + temperature data is 4 bytes long */
```
only use spaces for indentation 

> +    /*DEBUG("UT: %i\n", (int)*(output));*/
+

```suggestion
```

> @@ -0,0 +1 @@
+USEMODULE += xtimer

```suggestion
FEATURES_REQUIRED += periph_i2c
```
xtimer is not used by the driver

> +
+    /* Read uncompensated pressure value */
+    _read_up(dev, &up);
+
+    /* Compute true pressure value following datasheet formulas */
+    return (int32_t)((up - 1638) * 2000 * HSC_RANGE / (14745 - 1638) - 1000 * HSC_RANGE);
+}
+
+/*------------------------------------------------------------------------------------*/
+/*                                Internal functions                                  */
+/*------------------------------------------------------------------------------------*/
+
+static int _read_ut(const hsc_t *dev, int32_t *output)
+{
+    /* Read UT (Uncompsensated Temperature value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};

```suggestion
    uint8_t buf[HSC_FULL_DATA_LENGTH];
```

no need to set this to 0, will be overwritten anyway. 

> +    return (int32_t)((up - 1638) * 2000 * HSC_RANGE / (14745 - 1638) - 1000 * HSC_RANGE);
+}
+
+/*------------------------------------------------------------------------------------*/
+/*                                Internal functions                                  */
+/*------------------------------------------------------------------------------------*/
+
+static int _read_ut(const hsc_t *dev, int32_t *output)
+{
+    /* Read UT (Uncompsensated Temperature value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
+    
+    /* Acquire exclusive access */
+    i2c_acquire(DEV_I2C);
+    
+    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, &buf[0], HSC_FULL_DATA_LENGTH, 0) < 0) {

```suggestion
    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, buf, sizeof(buf), 0) < 0) {
```

> +        return -1;
+    }
+
+    i2c_release(DEV_I2C);
+
+    *output = (((buf[2] << 8) | buf[3]) >> HSC_TEMPERATURE_SHIFT);
+        
+    /*DEBUG("UT: %i\n", (int)*(output));*/
+
+    return 0;
+}
+
+static int _read_up(const hsc_t *dev, int32_t *output)
+{
+    /* Read UP (Uncompsensated Pressure value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};

```suggestion
    uint8_t buf[2];
```
since we are only interested in the first two bytes here

> +
+    /* Read uncompensated temperature value */
+    _read_ut(dev, &ut);
+
+    /* Compute true temperature value following datasheet formulas */
+    return (int16_t)(ut * 200 / 2047 - 50);
+}
+
+int32_t hsc_read_pressure(const hsc_t *dev)
+{
+    int32_t up = 0;
+
+    /* Read uncompensated pressure value */
+    _read_up(dev, &up);
+
+    /* Compute true pressure value following datasheet formulas */

Which data sheet did you use?

> + *
+ * @author      Quang Pham <phhr_quang at live.com>
+ */
+
+#ifndef HSC_INTERNALS_H
+#define HSC_INTERNALS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    HSC I2C Address
+ * @{
+ */
+#define HSC_ADDR                    (0x28)   /* 7 bit address */

```suggestion
#ifndef HSC_PARAM_ADDR
#define HSC_PARAM_ADDR              (0x28)   /* 7 bit address */
#endif
```

To follow the naming convention and allow to overwrite the address as sensors can have different addresses. 

> +#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    HSC I2C Address
+ * @{
+ */
+#define HSC_ADDR                    (0x28)   /* 7 bit address */
+/** @} */
+
+/**
+ * @name    HSC Max Range
+ * @{
+ */
+#define HSC_RANGE                   (40U)    /* in mbar dimension */

```suggestion
#ifndef HSC_PARAM_RANGE
#define HSC_PARAM_RANGE             (40U)    /* in mbar dimension */
#endif
```

> +    /* Read UP (Uncompsensated Pressure value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
+        
+    /* Acquire exclusive access */
+    i2c_acquire(DEV_I2C);
+    
+    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);
+
+    *output = ((buf[0] << 8) | buf[1]) & HSC_PRESSURE_MASK;

Doesn't buf[0] contain the status byte? 

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