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

benpicco notifications at github.com
Wed Sep 25 13:10:51 CEST 2019


benpicco commented on this pull request.

Almost there :)

You can still simplify the API:
 - The `crf` parameter is now not needed anymore
 - instead of returning raw sensor data and having to call `opt3001_convert()` on it, just always return milliLux - there is no loss in precision. Then you can also drop the `opt3001_read_lux()` function.

There are still some cosmetic issues where indentation is out of place.

> +/**
+ * @brief   Set active mode, this enables periodic measurements.
+ *
+ * @param[in]  dev          device descriptor of sensor
+ *
+ * @return                  0 on success
+ * @return                  -1 on error
+ */
+int opt3001_set_active(const opt3001_t *dev);
+
+/**
+ * @brief   Read sensor's data.
+ *
+ * @param[in]  dev          device descriptor of sensor
+ * @param[out] rawl         raw lux value
+ * @param[out] crf          conversion ready, 0 if a conversion is in progress

Now you don't need this parameter anymore.

> + * @param[in]  dev          device descriptor of sensor
+ * @param[out] rawl         raw lux value
+ * @param[out] crf          conversion ready, 0 if a conversion is in progress
+ *
+ * @return                  0 on success
+ * @return                  -1 on error
+ */
+int opt3001_read(const opt3001_t *dev, uint16_t *crf, uint16_t *rawl);
+
+/**
+ * @brief   Convert raw sensor values to lux.
+ *
+ * @param[in]  rawl         raw lux value
+ * @param[out] convl        Ambient Light in milliLux
+ */
+void opt3001_convert(int16_t rawl, uint32_t *convl);

What is the use-case for returning raw sensor values in `opt3001_read()`?
You could simplify the driver API a lot by always just always returning milliLux and dropping the `opt3001_convert()` function.

> +/**
+ * @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      (0xC000)     /**< Automatic full-scale setting mode */

Indentation 

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


More information about the notifications mailing list