[riot-notifications] [RIOT-OS/RIOT] drivers/ph_oem: support for Atlas Scientific pH OEM sensor (#10983)

Leandro Lanzieri notifications at github.com
Tue Feb 19 11:52:59 CET 2019


leandrolanzieri requested changes on this pull request.

@skullbox305 thanks for your contribution. I left some comments for you to check.

> +    }
+    i2c_release(I2C);
+
+    return PH_OEM_OK;
+}
+
+int ph_oem_start_new_reading(const ph_oem_t *dev)
+{
+    if (ph_oem_set_device_state(dev, PH_OEM_TAKE_READINGS) < 0) {
+        return PH_OEM_WRITE_ERR;
+    }
+
+    /* if interrupt pin undefined, poll till new reading was taken */
+    if (dev->params.interrupt_pin == GPIO_UNDEF) {
+        if (ph_oem_new_reading_available(dev) < 0) {
+            return PH_OEM_READ_WRITE_ERR;

Why introduce a new error code just for this case when the actual error reason (either Read or Write) can be known?

> +/**
+ * @brief   pH OEM device descriptor
+ */
+typedef struct ph_oem {
+    ph_oem_params_t params;         /**< device driver configuration */
+    ph_oem_interrupt_pin_cb_t cb;   /**< interrupt pin callback */
+    void *arg;                      /**< interrupt pin callback param */
+} ph_oem_t;
+
+/**
+ * @brief   Initialize a pH OEM sensor
+ *
+ * @param[in,out]   dev      device descriptor
+ * @param[in]       params   device configuration
+ *
+ * @return zero on successful initialization, non zero on error

Here and bellow please document with the defined return codes

> +        i2c_release(I2C);
+        return PH_OEM_NODEV;
+    }
+
+    /* Test if the device ID of the attached pH OEM sensor equals the
+     * value of the PH_OEM_REG_DEVICE_TYPE register
+     * */
+    if (reg_data != PH_OEM_DEVICE_TYPE_ID) {
+        DEBUG("\n[ph_oem debug] init - error: the attached device is not a pH OEM "
+              "Sensor. Read Device Type ID is: %i\n", reg_data);
+        i2c_release(I2C);
+        return PH_OEM_NOT_PH;
+    }
+    i2c_release(I2C);
+
+    /* Initilize enable gpio pin if it is defined */

```suggestion
    /* Initialize enable gpio pin if it is defined */
```

> +            break;
+        case PH_OEM_IRQ_RISING:
+            gpio_flank = GPIO_RISING;
+            gpio_mode = GPIO_IN_PD;
+            break;
+        case PH_OEM_IRQ_BOTH:
+            gpio_flank = GPIO_BOTH;
+            gpio_mode = GPIO_IN_PD;
+            break;
+    }
+
+    if (option != PH_OEM_IRQ_DISABLED) {
+        if (gpio_init_int(dev->params.interrupt_pin,
+                          gpio_mode, gpio_flank, cb, arg) < 0) {
+
+            DEBUG("\n[ph_oem debug] Initilizing enable gpio pin failed.\n");

```suggestion
            DEBUG("\n[ph_oem debug] Initializing enable gpio pin failed.\n");
```

> +
+        if (ph_oem_set_device_state(dev, PH_OEM_STOP_READINGS) < 0) {
+            return PH_OEM_WRITE_ERR;
+        }
+    }
+    return PH_OEM_OK;
+}
+
+int ph_oem_clear_calibration(const ph_oem_t *dev)
+{
+    uint8_t reg_value;
+
+    i2c_acquire(I2C);
+    if (i2c_write_reg(I2C, ADDR, PH_OEM_REG_CALIBRATION_REQUEST, 0x01, 0) < 0) {
+        DEBUG("\n[ph_oem debug] Clearing calibration failed \n");
+        return PH_OEM_WRITE_ERR;

Please release the I2C bus before returning.

> +
+int ph_oem_set_led_state(const ph_oem_t *dev, ph_oem_led_state_t state)
+{
+    i2c_acquire(I2C);
+
+    if (i2c_write_reg(I2C, ADDR, PH_OEM_REG_LED, state, 0x0) < 0) {
+        DEBUG("\n[ph_oem debug] Setting LED state to %d failed.\n", state);
+        i2c_release(I2C);
+        return PH_OEM_WRITE_ERR;
+    }
+    i2c_release(I2C);
+
+    return PH_OEM_OK;
+}
+
+int ph_oem_enable_device(const ph_oem_t *dev, bool enable)

I don't really think the enable pin should be included in the device driver as it is not a functionality provided by the actual device and needs external setup. This should be handled in the application.

> +}
+
+int ph_oem_enable_interrupt(ph_oem_t *dev, ph_oem_interrupt_pin_cb_t cb,
+                            void *arg, ph_oem_irq_option_t option)
+{
+    if (dev->params.interrupt_pin == GPIO_UNDEF) {
+        return PH_OEM_INTERRUPT_GPIO_UNDEF;
+    }
+
+    int gpio_flank = 0;
+    int gpio_mode = 0;
+
+    dev->arg = arg;
+    dev->cb = cb;
+
+    /* gpio_mode might be different for your microcontroller. Just GPIO_IN

If this is MCU dependent, maybe the gpio_mode should be passed as a parameter?

> +
+    i2c_acquire(I2C);
+
+    if (i2c_write_reg(I2C, ADDR, PH_OEM_REG_ADDRESS, addr, 0x0) < 0) {
+        DEBUG("\n[ph_oem debug] Setting I2C address to %x failed\n", addr);
+        i2c_release(I2C);
+        return PH_OEM_WRITE_ERR;
+    }
+
+    dev->params.addr = addr;
+    i2c_release(I2C);
+
+    return PH_OEM_OK;
+}
+
+int ph_oem_enable_interrupt(ph_oem_t *dev, ph_oem_interrupt_pin_cb_t cb,

Why not set the interrupt pin mode also on the device when this function is called?

> +/**
+ * @brief   Sets the device state (active/hibernate) of the pH OEM sensor by
+ *          writing to the @ref PH_OEM_REG_HIBERNATE register.
+ *          Once the device has been woken up it will continuously take readings
+ *          every 420ms. Waking the device is the only way to take a reading.
+ *          Hibernating the device is the only way to stop taking readings.
+ *
+ * @param[in] dev   device descriptor
+ * @param[in] state @ref ph_oem_device_state_t
+ *
+ * @return zero on successful write, non zero on error
+ */
+int ph_oem_set_device_state(const ph_oem_t *dev, ph_oem_device_state_t state);
+
+/**
+ * @brief   Starts a new reading by setting the device state to

Maybe you could add that in case of polling it will set the device to STOP mode again.

> +    if (i2c_read_reg(I2C, ADDR, PH_OEM_REG_CALIBRATION_CONFIRM,
+                     calibration_state, 0) < 0) {
+        DEBUG("\n[ph_oem debug] Failed at reading calibration confirm register\n");
+        i2c_release(I2C);
+        return PH_OEM_READ_ERR;
+    }
+    i2c_release(I2C);
+    return PH_OEM_OK;
+}
+
+int ph_oem_set_compensation(const ph_oem_t *dev,
+                            uint16_t temperature_compensation)
+{
+    uint8_t reg_value[4];
+
+    reg_value[0] = 0x00;

Here the `temperature_compensation` value could be checked to verify it is inside the accepted range (1 - 20000) according to the datasheet. Also this constraint should be added to the documentation of the function.

> +    i2c_acquire(I2C);
+
+    if (i2c_write_regs(I2C, ADDR, PH_OEM_REG_TEMP_COMPENSATION_BASE,
+                       &reg_value, 4, 0) < 0) {
+        DEBUG("\n[ph_oem debug] Setting temperature compensation of device to "
+              "%d failed\n", temperature_compensation);
+        i2c_release(I2C);
+        return PH_OEM_WRITE_ERR;
+    }
+    i2c_release(I2C);
+
+    return PH_OEM_OK;
+}
+
+int ph_oem_read_compensation(const ph_oem_t *dev,
+                             int16_t *temperature_compensation)

Why `int16_t`? The temperature compensation value is always positive.

> +    /* if successfully unlocked the register will equal 0x00 */
+    i2c_read_reg(I2C, ADDR, PH_OEM_REG_UNLOCK, &reg_value, 0x0);
+
+    if (reg_value != 0x00) {
+        DEBUG("\n[ph_oem debug] Failed at unlocking I2C address register. \n");
+        i2c_release(I2C);
+        return PH_OEM_WRITE_ERR;
+    }
+    i2c_release(I2C);
+
+    return PH_OEM_OK;
+}
+
+int ph_oem_set_i2c_address(ph_oem_t *dev, uint8_t addr)
+{
+    if (ph_oem_unlock_address_reg(dev) != PH_OEM_OK) {

Here and in the other public functions it would be good to check for a not NULL `dev` pointer

> +    /* Read raw pH value */
+    if (ph_oem_read_ph(mydev, res->val) < 0) {
+        return -ECANCELED;
+    }
+    res->unit = UNIT_PH;
+    res->scale = -3;
+
+    return 1;
+}
+
+/* Sets the temperature compensation for taking accurate pH readings */
+static int set_temp_compensation(const void *dev, phydat_t *res)
+{
+    const ph_oem_t *mydev = dev;
+
+    ph_oem_set_compensation(mydev, res->val[0]);

Can you check the returned value of this call to detect errors?

> +    }
+
+    ph_oem_start_new_reading(mydev);
+
+    /* Read raw pH value */
+    if (ph_oem_read_ph(mydev, res->val) < 0) {
+        return -ECANCELED;
+    }
+    res->unit = UNIT_PH;
+    res->scale = -3;
+
+    return 1;
+}
+
+/* Sets the temperature compensation for taking accurate pH readings */
+static int set_temp_compensation(const void *dev, phydat_t *res)

Can you add a note in `ph_oem.h` explaining that the write operation in SAUL sets the temperature compensation? (Also add the valid range of values for it)

-- 
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/10983#pullrequestreview-205133825
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190219/60cefbe3/attachment-0001.html>


More information about the notifications mailing list