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

Leandro Lanzieri notifications at github.com
Tue Feb 26 18:45:29 CET 2019


leandrolanzieri commented on this pull request.



> + *
+ * @param[in] dev   device descriptor
+ * @param[in] addr  new address for the device. Range: 0x01 - 0x7f
+ *
+ * @return @ref PH_OEM_OK on success
+ * @return @ref PH_OEM_WRITE_ERR if writing to the device failed
+ */
+int ph_oem_set_i2c_address(ph_oem_t *dev, uint8_t addr);
+
+/**
+ * @brief   Enable the pH OEM interrupt pin if @ref ph_oem_params_t.interrupt_pin
+ *          is defined.
+ *          @note Provide the PH_OEM_PARAM_INTERRUPT_OPTION flag in your
+ *          makefile. Valid options see: @ref ph_oem_irq_option_t.
+ *          The default is @ref PH_OEM_IRQ_BOTH
+ *

Maybe add a little comment saying that `ph_oem_reset_interrupt_pin` must be called from `cb`.

> + *
+ * @param[in] dev       device descriptor
+ * @param[in] cb        callback called when the pH OEM interrupt pin fires
+ * @param[in] arg       callback argument
+ * @param[in] gpio_mode @ref gpio_mode_t <br>
+ *                      most likely @ref GPIO_IN. If the pin is to sensitive use
+ *                      @ref GPIO_IN_PU for @ref PH_OEM_IRQ_FALLING or <br>
+ *                      @ref GPIO_IN_PD for @ref PH_OEM_IRQ_RISING and
+ *                      @ref PH_OEM_IRQ_BOTH
+ *
+ * @return @ref PH_OEM_OK on success
+ * @return @ref PH_OEM_WRITE_ERR if writing to the device failed
+ * @return @ref PH_OEM_INTERRUPT_GPIO_UNDEF if the interrupt pin is undefined
+ * @return @ref PH_OEM_GPIO_INIT_ERR if initializing the interrupt gpio pin failed
+ */
+int ph_oem_enable_interrupt(ph_oem_t *dev, ph_oem_interrupt_pin_cb_t cb,

Now that the interrupt option has been moved to the params and as the gpio mode is not likely to change during runtime maybe it can also be moved there (as it depends on the selected interrupt option). 

> +#define I2C (dev->params.i2c)
+#define ADDR (dev->params.addr)
+#define IRQ_OPTION (dev->params.irq_option)
+
+static int ph_oem_init_test(const ph_oem_t *dev);
+
+int ph_oem_init(ph_oem_t *dev, const ph_oem_params_t *params)
+{
+    assert(dev && params);
+
+    dev->params = *params;
+
+    return ph_oem_init_test(dev);
+}
+
+static int ph_oem_init_test(const ph_oem_t *dev)

Here and bellow, according to the [Coding conventions](https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#Naming) private functions do not have the module prefix.

> +
+/* off by default, so it won't reset your previous calibration */
+#define CALIBRATION_TEST_ENABLED    (false)
+
+static ph_oem_t dev;
+
+static void interrupt_pin_cb(void *arg)
+{
+    puts("\n[IRQ - Reading done]");
+
+    /* stop pH sensor from taking further readings*/
+    ph_oem_set_device_state(&dev, PH_OEM_STOP_READINGS);
+
+    /* reset interrupt pin in case of falling or rising flank */
+    if (dev.params.irq_option != PH_OEM_IRQ_BOTH) {
+        ph_oem_reset_interrupt_pin(&dev);

Is it not valid to call this function when `PH_OEM_IRQ_BOTH` is being used? If so maybe check for the case in the function and return directly.

-- 
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-208103145
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190226/3c4c33ef/attachment-0001.html>


More information about the notifications mailing list