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

benpicco notifications at github.com
Thu Sep 26 11:48:24 CEST 2019


benpicco commented on this pull request.

Three more comments, then this should be good to go!

> +            i2c_release(DEV_I2C);
+            LOG_ERROR("opt3001_read: Conversion in progress!\n");
+            return -OPT3001_ERROR;
+        }
+
+        if (i2c_read_regs(DEV_I2C, DEV_ADDR, OPT3001_REGS_CONFIG, &reg, 2, 0) < 0) {
+            i2c_release(DEV_I2C);
+            LOG_ERROR("opt3001_read: Error reading BUS!\n");
+            return -OPT3001_ERROR_BUS;
+        }
+
+        if (htons(reg) & OPT3001_REGS_CONFIG_CRF) {
+            break;
+        }
+
+        xtimer_usleep(1000);

You should do `i2c_release(DEV_I2C);` before sleeping and then do `i2c_acquire(DEV_I2C);
` again afterwards.
This way you do not block the bus if some other threads wants to access a different I2C peripheral.

Also, if this will ever be implemented, it would allow the MCU to enter a deeper sleep state because the I2C peripheral is not in use. Unfortunately this is not the case yet.

> + *
+ * @return                  0 on success
+ * @return                  -1 on error
+ */
+int opt3001_set_active(const opt3001_t *dev);
+
+/**
+ * @brief   Read sensor's raw data and convert it to milliLux.
+ *
+ * @param[in]  dev          device descriptor of sensor
+ * @param[out] rawl         ambient light in milliLux
+ *
+ * @return                  0 on success
+ * @return                  -1 on error
+ */
+int opt3001_read_lux(const opt3001_t *dev, int32_t *convl);

I think lux values can never be negative, so just use `uint32_t`.

> +    if (i2c_read_regs(DEV_I2C, DEV_ADDR, OPT3001_REGS_RESULT, &reg, 2, 0) < 0) {
+        i2c_release(DEV_I2C);
+        LOG_ERROR("opt3001_read: Error reading BUS!\n");
+        return -OPT3001_ERROR_BUS;
+    }
+
+    exponent = OPT3001_REGS_REG_EXPONENT(htons(reg));
+    mantissa = OPT3001_REGS_REG_MANTISSA(htons(reg));
+
+    *convl = (1 << exponent) * mantissa * 10;
+
+    i2c_release(DEV_I2C);
+    return OPT3001_OK;
+}
+
+int opt3001_read_lux_saul(const opt3001_t *dev, int16_t *convl)

Since this is only used by saul, you can put that logic into `opt3001_saul.c`.
It only uses public functions anyway.

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


More information about the notifications mailing list