[riot-notifications] [RIOT-OS/RIOT] drivers/ds75lx: add basic driver for temperature sensor (#11505)

Parks Projets notifications at github.com
Tue May 14 21:12:25 CEST 2019


ParksProjets requested changes on this pull request.

Hi everyone!  
I'm quite new on RIOT OS but we made a project using RIOT-OS on a IM880-A (which is almost identical to IM880-B) and used a DS75LX.

I have read the code of this driver and some points are weird. I wonder if the implementation has been tested with negative temperatures because I'd be surprised that it works.

> +        i2c_release(DEV_I2C);
+
+        return -DS75LX_ERR_I2C;
+    }
+
+    temp = (tmp[0] << 8) | tmp[1];
+    DEBUG("[ds75lx] temperature register content 0x%04X\n", temp);
+
+    /* Manage sign bit */
+    bool negative = false;
+    if (temp & 0x8000) {
+        negative = true;
+        temp &= ~0x8000;
+    }
+
+    *temperature = (temp >> 5);

Resolution 12 needs 12 bits. By shift left by `5` bits, the maximum resolution you can get is 11.

> +        return -DS75LX_ERR_I2C;
+    }
+
+    temp = (tmp[0] << 8) | tmp[1];
+    DEBUG("[ds75lx] temperature register content 0x%04X\n", temp);
+
+    /* Manage sign bit */
+    bool negative = false;
+    if (temp & 0x8000) {
+        negative = true;
+        temp &= ~0x8000;
+    }
+
+    *temperature = (temp >> 5);
+    if (negative) {
+        *temperature = -*temperature;

The specification indicates that the result is a **16-bit two’s complement number**. Here you interpret the result as SMR (Signed Magnitude Representation) which will give wrong (and quite big) results for negative values.

In fact when the temperature is negative, it can be direclty be interpreted as a negative number by the CPU (if the CPU uses two’s complement number, which is usually the case).

https://datasheets.maximintegrated.com/en/ds/DS75LX.pdf

> +/**
+ * @brief   Initialize the given DS75LX device
+ *
+ * @param[out] dev          Initialized device descriptor of DS75LX device
+ * @param[in]  params       Initialization parameters
+ *
+ * @return                  DS75LX_OK on success
+ * @return                  -DS75LX_ERR_I2C if an error occured when reading/writing
+ */
+int ds75lx_init(ds75lx_t *dev, const ds75lx_params_t *params);
+
+/**
+ * @brief   Read temperature value from the given DS75LX device, returned in d°C
+ *
+ * @param[in] dev           Device descriptor of DS75LX device
+ * @param[out] temperature  Temperature in d°C

Maybe indicates that `temperature` is a 16 bits fixed point number. This number has 13 bits on integer part and 3 bits on fractional parts (it's because of the left shift of 5 bits).  
Personnaly, I think it's a bit weird and a symmetrical fixed point number (both 8 bits on integer and fractional parts) would be better.

> +    printf("\n+--------Starting Measurements--------+\n");
+    int16_t temperature;
+    while (1) {
+        ds75lx_wakeup(&dev);
+        /* Get temperature in degrees celsius */
+        ds75lx_read_temperature(&dev, &temperature);
+        ds75lx_shutdown(&dev);
+
+        bool negative = (temperature < 0);
+        if (negative) {
+            temperature = -temperature;
+        }
+
+        printf("Temperature [°C]: %c%d.%d\n",
+               negative ? '-': ' ',
+               (int)(temperature / 10),

As `ds75lx_read_temperature` returns a **fixed point number** (at least when the temperature is positive), you can't divide the temperature by `10`. As you the fixed point number has 3 bits on fractional part, you have to divide by `8` (or a left shift of 3 bits to remove fractional part).

Maybe you had plausible results. This is because increments of value (when we look at it as a normal integer, not a fixed point number) are of `0.125 °C` which is close to `0.1 °C`, but causes an error of 20 % on temperature.

> +    int16_t temperature;
+    while (1) {
+        ds75lx_wakeup(&dev);
+        /* Get temperature in degrees celsius */
+        ds75lx_read_temperature(&dev, &temperature);
+        ds75lx_shutdown(&dev);
+
+        bool negative = (temperature < 0);
+        if (negative) {
+            temperature = -temperature;
+        }
+
+        printf("Temperature [°C]: %c%d.%d\n",
+               negative ? '-': ' ',
+               (int)(temperature / 10),
+               (int)(temperature % 10));

Same here. You can use `(temperature & 0b111) * 125` for getting the fractional part as decimal.

-- 
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/11505#pullrequestreview-237416731
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190514/fbceb078/attachment-0001.html>


More information about the notifications mailing list