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

Sebastian Meiling notifications at github.com
Mon May 13 21:18:51 CEST 2019


smlng requested changes on this pull request.



> +}
+
+int ds75lx_init(ds75lx_t *dev, const ds75lx_params_t *params)
+{
+    dev->params = *params;
+
+    /* Acquire exclusive access */
+    i2c_acquire(DEV_I2C);
+
+    uint8_t config = (dev->params.resolution << DS75LX_CONF_R0_POS);
+    /* force shutdown of the sensor */
+    config |= (1 << DS75LX_CONF_SD_POS);
+
+    DEBUG("[ds75lx] configuration register value: 0x%02X\n", config);
+
+    if (i2c_write_reg(DEV_I2C, DEV_ADDR, DS75LX_REG_CONFIGURATION, config, 0) < 0) {

can't you use your `_set_configuration_bit` helper here, too?

> +        /* Release I2C device */
+        i2c_release(DEV_I2C);
+
+        return -DS75LX_ERR_I2C;
+    }
+
+    /* Release I2C device */
+    i2c_release(DEV_I2C);
+
+    return DS75LX_OK;
+}
+
+int ds75lx_read_temperature(const ds75lx_t *dev, int16_t *temperature)
+{
+    /* Wait max conversion time (depends on resolution) */
+    xtimer_usleep(_max_conversion_times[dev->params.resolution] * US_PER_MS);

why? This looks weird, as you do not trigger a conversion before. I would rather move this into the wakeup function if necessary at all.

> @@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2019 Inria
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     drivers_ds75lx
+ * @brief       Internal addresses, registers, constants for the DS75LX sensor.

this `@brief` is for the `@ingroup` (or would change it) IMHO remove here

> + *
+ * @file
+ * @brief       Internal addresses, registers, constants for the DS75LX sensor.
+ *
+ * @author      Alexandre Abadie <alexandre.abadie at inria.fr>
+ */
+
+#ifndef DS75LX_INTERNALS_H
+#define DS75LX_INTERNALS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    DS75LX I2C address

use `@brief` here instead

> + *
+ * @author      Alexandre Abadie <alexandre.abadie at inria.fr>
+ */
+
+#ifndef DS75LX_INTERNALS_H
+#define DS75LX_INTERNALS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    DS75LX I2C address
+ * @{
+ */
+#define DS75LX_ADDR                     (0x48) /* 7 bit address */

move comment up, i.e. `DS75LX I2C 7 bit address` or `DS75LX I2C address (7 bit)`, or similar 

> +
+#ifndef DS75LX_H
+#define DS75LX_H
+
+#include "saul.h"
+#include "periph/i2c.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   Thermometer resolution
+ */
+typedef enum {
+    DS75LX_RESOLUTION_9 = 0,        /**< 9 bits resolution, 25ms max conversion time */

why don't you map this directly to `25`, `50` and so on milliseconds here, instead of going through this indirection of 0-3 here and then remap to the values in the driver?

> +
+/**
+ * @brief   Read temperature value from the given DS75LX device, returned in d°C
+ *
+ * @param[in] dev           Device descriptor of DS75LX device to read from
+ * @param[out] temperature  Temperature in d°C
+ *
+ * @return                  DS75LX_OK on success
+ * @return                  -DS75LX_ERR_I2C if an error occured when reading/writing
+ */
+int ds75lx_read_temperature(const ds75lx_t *dev, int16_t *temperature);
+
+/**
+ * @brief   Wakeup the sensor
+ *
+ * @param[in] dev           Device descriptor of DS75LX device to read from

what are you reading here 😄 Copy-Paste here and below

-- 
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-236861217
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190513/31dcc4c1/attachment.html>


More information about the notifications mailing list