[riot-notifications] [RIOT-OS/RIOT] drivers/opt3001: Initial support for the Opt3001 sensor (#12260)

benpicco notifications at github.com
Thu Sep 19 10:28:05 CEST 2019


benpicco commented on this pull request.

The driver looks good, but there some possibilities for improvement.

Also no need to add an empty `Readme.md` file.

> + *
+ * @author      Jannes Volkens <jannes.volkens at haw-hamburg.de>
+ */
+
+#ifndef OPT3001_H
+#define OPT3001_H
+
+#include <stdint.h>
+#include "periph/i2c.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define DEV_I2C     (dev->params.i2c_dev) /**< BUS */
+#define DEV_ADDR    (dev->params.i2c_addr) /**< ADDR */

Please put that into the `.c` file, otherwise it may lead to conflicts when other drivers/users do that.

> + * @name  OPT3001 registers
+ * @{
+ */
+ #define OPT3001_REGS_RESULT          0x00 /**< Result register */
+ #define OPT3001_REGS_CONFIG          0x01 /**< Configuration register */
+ #define OPT3001_REGS_LOW_LIMIT       0x02 /**< Low Limit register */
+ #define OPT3001_REGS_HIGH_LIMIT      0x03 /**< High Limit register */
+ #define OPT3001_REGS_MANUFACTURER_ID 0x7E /**< Manufacturer ID register */
+ #define OPT3001_REGS_DEVICE_ID       0x7F /**< Device ID register */
+
+ #define OPT3001_DID_VALUE (0x3001) /**< Device ID value */
+
+ #define OPT3001_CONFIG_RESET (0xC810) /**< Reset value */
+
+ #define OPT3001_CONFIG_RN_FSR      (0xC)     /**< Automatic full-scale setting mode */
+ #define OPT3001_CONFIG_RN_SHIFT	  (12U)     /**< Range number field shift */

Indentation 

> +
+ #define OPT3001_CONFIG_RN_FSR      (0xC)     /**< Automatic full-scale setting mode */
+ #define OPT3001_CONFIG_RN_SHIFT	  (12U)     /**< Range number field shift */
+ #define OPT3001_CONFIG_RN_MASK	    (0xF000)  /**< Range number field mask */
+ #define OPT3001_REGS_CONFIG_RN(x)  (((uint16_t)(((uint16_t)(x)) \
+                                    << OPT3001_CONFIG_RN_SHIFT)) \
+                                    & OPT3001_CONFIG_RN_MASK) /**< Range number field */
+
+ #define OPT3001_REGS_CONFIG_CT_SHIFT  (11U)    /**< Conversion time shift */
+ #define OPT3001_REGS_CONFIG_CT_MASK   (0x0800) /**< Conversion time mask */
+ #define OPT3001_REGS_CONFIG_CT(x)     (((uint16_t)(((uint16_t)(x)) \
+                                       << OPT3001_REGS_CONFIG_CT_SHIFT)) \
+                                       & OPT3001_REGS_CONFIG_CT_MASK) /**< Conversion time field */
+
+ #define OPT3001_CONFIG_M_SHUTDOWN     (0x0) /**< Shutdown mode */
+ #define OPT3001_CONFIG_M_SINGLE	     (0x1) /**< Single-shot mode */

Indentation

> +
+ #define OPT3001_REGS_CONFIG_FC_MASK  (0x0003) /**< Fault count field mask */
+
+ #define OPT3001_REGS_LOW_LIMIT_EOC_ENABLE 0xC000 /**< End-of-conversion enable */
+
+ #define OPT3001_REGS_REG_EXPONENT(x)		((x) >> 12) /**< Exponent */
+ #define OPT3001_REGS_REG_MANTISSA(x)		((x) & 0xFFF) /**< Mantissa */
+
+/*
+ * Time to wait for the conversion to complete.
+ * The data sheet of the device (sect. 6.5) indicates that the conversion time is the integration time plus 3 ms.
+ * It has been added a bit more time just to be safe.
+ */
+ #define OPT3001_REGS_INT_TIME_SHORT	100000 /**< Integration time of 100ms */
+ #define OPT3001_REGS_INT_TIME_LONG		800000 /**< Integration time of 800ms */
+

Indentation

> + */
+ #define OPT3001_REGS_RESULT          0x00 /**< Result register */
+ #define OPT3001_REGS_CONFIG          0x01 /**< Configuration register */
+ #define OPT3001_REGS_LOW_LIMIT       0x02 /**< Low Limit register */
+ #define OPT3001_REGS_HIGH_LIMIT      0x03 /**< High Limit register */
+ #define OPT3001_REGS_MANUFACTURER_ID 0x7E /**< Manufacturer ID register */
+ #define OPT3001_REGS_DEVICE_ID       0x7F /**< Device ID register */
+
+ #define OPT3001_DID_VALUE (0x3001) /**< Device ID value */
+
+ #define OPT3001_CONFIG_RESET (0xC810) /**< Reset value */
+
+ #define OPT3001_CONFIG_RN_FSR      (0xC)     /**< Automatic full-scale setting mode */
+ #define OPT3001_CONFIG_RN_SHIFT	  (12U)     /**< Range number field shift */
+ #define OPT3001_CONFIG_RN_MASK	    (0xF000)  /**< Range number field mask */
+ #define OPT3001_REGS_CONFIG_RN(x)  (((uint16_t)(((uint16_t)(x)) \

Are those casts needed?

> + * @brief   Default raw value mode
+ *
+ * If set to 0, measurements will be converted to lux.
+ * If set to 1, raw readings will be returned.
+ */
+#ifndef OPT3001_USE_RAW_VALUES
+#define OPT3001_USE_RAW_VALUES (0)
+#endif
+
+/**
+ * @brief   Parameters needed for device initialization
+ */
+typedef struct {
+    i2c_t i2c_dev; /**< I2C device, the sensor is connected to */
+    uint8_t i2c_addr; /**< The sensor's slave address on the I2C bus */
+    uint16_t conversion_time; /**< Conversion time */

This is never used.

> +
+    *rawl = htons(reg);
+
+    i2c_release(DEV_I2C);
+    return OPT3001_OK;
+}
+
+void opt3001_convert(int16_t rawl, float *convl)
+{
+  uint16_t mantissa;
+  uint8_t exponent;
+
+  exponent = OPT3001_REGS_REG_EXPONENT(rawl);
+  mantissa = OPT3001_REGS_REG_MANTISSA(rawl);
+
+  *convl = 0.01 * pow(2, exponent) * mantissa;

You could just convert to milliLux and save yourself the `float`.

> +    i2c_release(DEV_I2C);
+    return OPT3001_OK;
+}
+
+int opt3001_read(const opt3001_t *dev, uint16_t *crf, uint16_t *rawl)
+{
+    uint16_t reg;
+
+    i2c_acquire(DEV_I2C);
+
+    /* wait for the conversion to finish */
+    if (OPT3001_CONVERSION_TIME) {
+        xtimer_usleep(OPT3001_CONVERSION_TIME_LONG);
+    } else{
+        xtimer_usleep(OPT3001_CONVERSION_TIME_SHORT);
+    }

Why sleep always and not just when `!crf`?
Then you would also not have to return that parameter to the user.

> +
+void opt3001_convert(int16_t rawl, float *convl)
+{
+  uint16_t mantissa;
+  uint8_t exponent;
+
+  exponent = OPT3001_REGS_REG_EXPONENT(rawl);
+  mantissa = OPT3001_REGS_REG_MANTISSA(rawl);
+
+  *convl = 0.01 * pow(2, exponent) * mantissa;
+}
+
+int opt3001_read_lux(const opt3001_t *dev, int16_t *convl)
+{
+    uint16_t crf;
+#if (!OPT3001_USE_RAW_VALUES)

What is the idea behind the function returning different things depending on the config?

-- 
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/12260#pullrequestreview-290382389
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190919/6e723b0b/attachment-0001.htm>


More information about the notifications mailing list