[riot-notifications] [RIOT-OS/RIOT] drivers: Add AD7746 capacitance sensor driver (#10919)

Peter Kietzmann notifications at github.com
Wed Feb 6 14:41:23 CET 2019


PeterKietzmann commented on this pull request.

@leandrolanzieri thanks for your contribution. The code looks really good and is in perfect shape! I left a couple of comments anyway which not all need to be addressed, but at least should be considered.

> +    if (mode == AD7746_VT_MD_DIS) {
+        reg &= ~(1 << AD7746_VT_SETUP_VTEN_BIT);
+    }
+    else {
+        /* Enable voltage / temperature channel and set mode */
+        reg &= ~((1 << AD7746_VT_SETUP_VTMD0_BIT) |
+                 (1 << AD7746_VT_SETUP_VTMD1_BIT));
+        reg |= (1 << AD7746_VT_SETUP_VTEN_BIT) |
+               (mode << AD7746_VT_SETUP_VTMD0_BIT);
+    }
+
+    if (i2c_write_reg(I2C, ADDR, AD7746_REG_VT_SETUP, reg, 0)) {
+        DEBUG("[ad7746] set_vt_ch - error: unable to set v/t channel mode\n");
+        goto release;
+    }
+

When does this function return with success?

> +                 (1 << AD7746_VT_SETUP_VTMD1_BIT));
+        reg |= (1 << AD7746_VT_SETUP_VTEN_BIT) |
+               (mode << AD7746_VT_SETUP_VTMD0_BIT);
+    }
+
+    if (i2c_write_reg(I2C, ADDR, AD7746_REG_VT_SETUP, reg, 0)) {
+        DEBUG("[ad7746] set_vt_ch - error: unable to set v/t channel mode\n");
+        goto release;
+    }
+
+release:
+    i2c_release(I2C);
+    return res;
+}
+
+int ad7746_read_raw_ch(const ad7746_t *dev, uint8_t ch, uint32_t *raw)

I'd prefer separate read functions for each measurement type. What's the benefit of one API?

> +    if (i2c_read_reg(I2C, ADDR, AD7746_REG_STATUS, buf, 0)) {
+        goto release;
+    }
+
+    /* check if data is available from the requested channel */
+    if (buf[0] & (1 << ch)) {
+        res = AD7746_NODATA;
+            DEBUG("[ad7746] read_raw: No data available\n");
+        goto release;
+    }
+
+    if (i2c_read_regs(I2C, ADDR, reg, buf, 3, 0)) {
+        goto release;
+    }
+
+    *raw = (((uint32_t)buf[0] << 16) | ((uint32_t)buf[1] << 8) |

Mention in the documentation that `raw` points to a 24-bit value?

> +    if (i2c_read_reg(I2C, ADDR, AD7746_REG_STATUS, buf, 0)) {
+        goto release;
+    }
+
+    /* check if data is available from the requested channel */
+    if (buf[0] & (1 << ch)) {
+        res = AD7746_NODATA;
+            DEBUG("[ad7746] read_raw: No data available\n");
+        goto release;
+    }
+
+    if (i2c_read_regs(I2C, ADDR, reg, buf, 3, 0)) {
+        goto release;
+    }
+
+    *raw = (((uint32_t)buf[0] << 16) | ((uint32_t)buf[1] << 8) |

Please also check [this](https://github.com/RIOT-OS/RIOT/wiki/Guide:-Writing-a-device-driver-in-RIOT#additional-sensor-driver-checklist) paragraph of the device driver guide. 

> +    }
+
+    if (i2c_read_regs(I2C, ADDR, reg, buf, 3, 0)) {
+        goto release;
+    }
+
+    *raw = (((uint32_t)buf[0] << 16) | ((uint32_t)buf[1] << 8) |
+            ((uint32_t)buf[2]));
+    res = AD7746_OK;
+
+release:
+    i2c_release(I2C);
+    return res;
+}
+
+int ad7746_raw_to_capacitance(uint32_t raw)

The above linked guide mentions that "all 'read' functions return values in their physical representation". If this was implemented, you would make the conversion within the drivers "read" function. In that case, the conversion helper might be `static inline` and you might remove it from the header as well.

> +    *raw = (((uint32_t)buf[0] << 16) | ((uint32_t)buf[1] << 8) |
+            ((uint32_t)buf[2]));
+    res = AD7746_OK;
+
+release:
+    i2c_release(I2C);
+    return res;
+}
+
+int ad7746_raw_to_capacitance(uint32_t raw)
+{
+    int32_t value = (raw - AD7746_ZERO_SCALE_CODE) >> 11;
+    return (int)value;
+}
+
+int ad7746_raw_to_temperature(uint32_t raw)

Same as above

> +    return res;
+}
+
+int ad7746_raw_to_capacitance(uint32_t raw)
+{
+    int32_t value = (raw - AD7746_ZERO_SCALE_CODE) >> 11;
+    return (int)value;
+}
+
+int ad7746_raw_to_temperature(uint32_t raw)
+{
+    int value = (raw >> 11) - 4096;
+    return value;
+}
+
+int ad7746_raw_to_voltage(uint32_t raw)

Same as above

> + */
+int ad7746_raw_to_temperature(uint32_t raw);
+
+/**
+ * @brief   Converts a raw code into voltage expressed in mV
+ *
+ * @param[in] raw   Raw code from the device
+ * @return voltage in mV
+ *
+ * @note Note when using the @ref AD7746_VT_MD_VDD mode, that the voltage from the
+ *       VDD pin is internally attenuated by 6
+ */
+int ad7746_raw_to_voltage(uint32_t raw);
+
+/**
+ * @brief   Sets the current input for the capacitive measurement

What is the difference between the inputs? This function is not used, not even during initialization. Which channel is default?

> + */
+int ad7746_raw_to_temperature(uint32_t raw);
+
+/**
+ * @brief   Converts a raw code into voltage expressed in mV
+ *
+ * @param[in] raw   Raw code from the device
+ * @return voltage in mV
+ *
+ * @note Note when using the @ref AD7746_VT_MD_VDD mode, that the voltage from the
+ *       VDD pin is internally attenuated by 6
+ */
+int ad7746_raw_to_voltage(uint32_t raw);
+
+/**
+ * @brief   Sets the current input for the capacitive measurement

Is this something one would change during runtime? If not, maybe we could get rid of the function and make it only compile-time configurable?

> + *
+ * @return AD7746_OK on success
+ * @return AD7746_NOI2C on error
+ */
+int ad7746_set_cap_ch_input(const ad7746_t *dev, uint8_t input);
+
+/**
+ * @brief   Sets the mode for the voltage / temperature channel
+ *
+ * @param[in] dev   device descriptor
+ * @param[in] mode  mode to which the channel has to be set
+ *
+ * @return AD7746_OK on success
+ * @return AD7746_NOI2C on error
+ */
+int ad7746_set_vt_channel_mode(const ad7746_t *dev, ad7746_vt_mode_t mode);

Same as above: need to have this compile-time configurable?

> + *
+ * @return AD7746_OK on success
+ * @return AD7746_NOI2C on error
+ */
+int ad7746_set_vt_sr(const ad7746_t *dev, ad7746_vt_sample_rate_t sr);
+
+/**
+ * @brief   Sets the sample rate for the capacitance channel
+ *
+ * @param[in] dev   device descriptor
+ * @param[in] sr    sample rate
+ *
+ * @return AD7746_OK on success
+ * @return AD7746_NOI2C on error
+ */
+int ad7746_set_cap_sr(const ad7746_t *dev, ad7746_cap_sample_rate_t sr);

I really don't know anything about the device (yet) but I'm wondering if there's need for us to maintain different sample rates for both sensors. What can we expect, energy savings, better resolution?

> +    if (ad7746_read_raw_ch((const ad7746_t *)dev, AD7746_READ_CAP_CH, &raw)) {
+        return -ECANCELED;
+    }
+
+    res->val[0] = ad7746_raw_to_capacitance(raw);
+    res->unit = UNIT_F;
+    res->scale = -15;
+
+    return 1;
+}
+
+const saul_driver_t ad7746_saul_driver = {
+    .read = _read,
+    .write = saul_notsup,
+    .type = SAUL_SENSE_CAP,
+};

Why do you only expose the CAP sensor to saul?

> + */
+
+#ifndef AD7746_H
+#define AD7746_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "periph/i2c.h"
+#include "periph/gpio.h"
+
+/**
+ * @brief  AD7746 default address
+ */
+#ifndef AD7746_I2C_ADDRESS

This can be removed right? You should have the bus address in the device descriptor.

> +#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "periph/i2c.h"
+#include "periph/gpio.h"
+
+/**
+ * @brief  AD7746 default address
+ */
+#ifndef AD7746_I2C_ADDRESS
+#define AD7746_I2C_ADDRESS  (0x48)
+#endif
+
+/**
+ * @brief 0fF capacitance code

0fF looks like a misspelled hex number but it's supposed to mean something else, right? femtoFarad?

> +    AD7746_CAP_SR_500 = 2, /**< 50.0 Hz */
+    AD7746_CAP_SR_263 = 3, /**< 26.3 Hz */
+    AD7746_CAP_SR_161 = 4, /**< 16.1 Hz */
+    AD7746_CAP_SR_130 = 5, /**< 13.0 Hz */
+    AD7746_CAP_SR_109 = 6, /**< 10.9 Hz */
+    AD7746_CAP_SR_091 = 7  /**<  9.1 Hz */
+} ad7746_cap_sample_rate_t;
+
+/**
+ * @brief Excitation signal outpus configuration
+ */
+typedef enum {
+    AD7746_EXC_A = 0x06,   /**< enable only exc A output */
+    AD7746_EXC_B = 0x09,   /**< enable only exc B output */
+    AD7746_EXC_AB = 0x0A   /**< enable exc A and B outputs */
+} ad7746_exc_config_t;

If I see it correctly, this is not used anywhere so I'd remove the typedef and well as it's usage in the device descriptor below 

> + * @brief Excitation signal outpus configuration
+ */
+typedef enum {
+    AD7746_EXC_A = 0x06,   /**< enable only exc A output */
+    AD7746_EXC_B = 0x09,   /**< enable only exc B output */
+    AD7746_EXC_AB = 0x0A   /**< enable exc A and B outputs */
+} ad7746_exc_config_t;
+
+/**
+ * @brief   AD7746 params
+ */
+typedef struct ad7746_params {
+    i2c_t i2c;                                /**< i2c device */
+    uint8_t addr;                             /**< i2c address */
+    uint8_t dac_a_cap;                        /**< DAC A capacitance */
+    uint8_t dac_b_cap;                        /**< DAC B capacitance */

Do we need to hold two capacitance values in this struct? 

> + */
+typedef enum {
+    AD7746_EXC_A = 0x06,   /**< enable only exc A output */
+    AD7746_EXC_B = 0x09,   /**< enable only exc B output */
+    AD7746_EXC_AB = 0x0A   /**< enable exc A and B outputs */
+} ad7746_exc_config_t;
+
+/**
+ * @brief   AD7746 params
+ */
+typedef struct ad7746_params {
+    i2c_t i2c;                                /**< i2c device */
+    uint8_t addr;                             /**< i2c address */
+    uint8_t dac_a_cap;                        /**< DAC A capacitance */
+    uint8_t dac_b_cap;                        /**< DAC B capacitance */
+    ad7746_exc_config_t exc_config;           /**< excitation signal config */

Needed in this struct?

> @@ -59,6 +59,7 @@ const char *saul_class_to_str(const uint8_t class_id)
         case SAUL_SENSE_CURRENT:   return "SENSE_CURRENT";
         case SAUL_SENSE_OCCUP:     return "SENSE_OCCUP";
         case SAUL_SENSE_PM:        return "SENSE_PM";
+        case SAUL_SENSE_CAP:       return "SENSE_CAP";

I'd go with `SENSE_CAPACITY`

> @@ -0,0 +1,87 @@
+/*

Once we agreed on the interface and the test is adapted accordingly, you should add a short README

-- 
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/10919#pullrequestreview-200522885
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190206/2c0385f2/attachment-0001.html>


More information about the notifications mailing list