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

Leandro Lanzieri notifications at github.com
Mon Feb 25 11:52:03 CET 2019


leandrolanzieri requested changes on this pull request.

@skullbox305 thanks for the changes. Please next time don't squash and force push the modifications so it is easier to follow during the reviewing process, that can be done before merging.
Here are some more comments, please tell me what's your opinion on the `ph_oem_set_interrupt_pin` function.

> +
+#include "xtimer.h"
+#include "assert.h"
+#include "periph/i2c.h"
+#include "periph/gpio.h"
+
+#include "ph_oem.h"
+#include "ph_oem_params.h"
+#include "ph_oem_regs.h"
+
+#define ENABLE_DEBUG    (0)
+#include "debug.h"
+
+#define I2C (dev->params.i2c)
+#define ADDR (dev->params.addr)
+#define EN_PIN (dev->params.enable_pin)

`EN_PIN` is not used anymore right?

> + *
+ *          The pin will not auto reset on option @ref PH_OEM_IRQ_RISING
+ *          and @ref PH_OEM_IRQ_FALLING after interrupt fires,
+ *          so call this function again to reset the pin state, providing the
+ *          same option (@ref PH_OEM_IRQ_RISING or @ref PH_OEM_IRQ_FALLING).
+ *
+ *          Settings are not retained if the power is cut, so you have to call
+ *          this function again after powering on the device.
+ *
+ * @param[in] dev    device descriptor
+ * @param[in] option @ref ph_oem_irq_option_t
+ *
+ * @return @ref PH_OEM_OK on success
+ * @return @ref PH_OEM_WRITE_ERR if writing to the device failed
+ */
+int ph_oem_set_interrupt_pin(const ph_oem_t *dev, ph_oem_irq_option_t option);

I'm not really convinced by having this function in the public API like this. Though I understand that a function that resets the interrupt pin is needed. Do you think maybe the irq option could be moved to the device descriptor parameters, and just have a `ph_oem_reset_interrupt_pin` function or something like that which would be called from the callback?

> +}
+
+int ph_oem_enable_interrupt(ph_oem_t *dev, ph_oem_interrupt_pin_cb_t cb,
+                            void *arg, ph_oem_irq_option_t option, gpio_mode_t gpio_mode)
+{
+    if (dev->params.interrupt_pin == GPIO_UNDEF) {
+        return PH_OEM_INTERRUPT_GPIO_UNDEF;
+    }
+
+    if (ph_oem_set_interrupt_pin(dev, option) < 0) {
+        return PH_OEM_WRITE_ERR;
+    }
+
+    int gpio_flank = 0;
+
+    dev->arg = arg;

As defined this function is also expected to be called in order to disable the interrupts, I would think that in that case `arg` and `cb` could be NULL, so this assignment should be done after checking `option`.

> +
+    if (ph_oem_set_interrupt_pin(dev, option) < 0) {
+        return PH_OEM_WRITE_ERR;
+    }
+
+    int gpio_flank = 0;
+
+    dev->arg = arg;
+    dev->cb = cb;
+
+    switch (option) {
+        case PH_OEM_IRQ_DISABLED:
+            break;
+        case PH_OEM_IRQ_FALLING:
+            gpio_flank = GPIO_FALLING;
+            //gpio_mode = GPIO_IN_PU;

Please remove comments

> +            puts("[Failed]");
+            return -1;
+        }
+
+        printf("Reading calibration state, should be 0... ");
+        if (ph_oem_read_calibration_state(&dev, &data) == PH_OEM_OK
+            && data == 0) {
+            puts("[OK]");
+        }
+        else {
+            puts("[Failed]");
+            return -1;
+        }
+
+        /* Don't forget to provide temperature compensation for the calibration */
+        printf("Setting temperature compensation to 22 Celcius... ");

```suggestion
        printf("Setting temperature compensation to 22 Celsius... ");
```

-- 
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-207286894
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190225/5499e6cc/attachment.html>


More information about the notifications mailing list