[riot-notifications] [RIOT-OS/RIOT] drivers/hx711: driver and SAUL adaption for the HX711 sensor (#11416)

Leandro Lanzieri notifications at github.com
Tue Apr 23 16:39:45 CEST 2019


leandrolanzieri requested changes on this pull request.

Thanks for the contribution! Here are some comments.

> @@ -0,0 +1,127 @@
+/*
+ * Copyright (C) 2019 Philipp-Alexander Blum <philipp-blum at jakiku.de>
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    drivers_hx711 HX711 digital scale sensor

The group should be defined only once, in the header file.

> + */
+
+#ifndef HX711_H
+#define HX711_H
+
+#include <stdint.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief Initialization of the connected HX711 IC
+ */
+void hx711_init(void);
+

extra space

> @@ -0,0 +1,29 @@
+# name of your application
+APPLICATION = hx711_driver_calibration
+
+# Use the ST B-L072Z-LRWAN1 board by default:
+BOARD ?= native

Comment says it uses 'ST B-L072Z-LRWAN1' by default but native is set

> +#ifndef HX711_PARAMS_H
+#define HX711_PARAMS_H
+
+#include "board.h"
+#include "hx711.h"
+#include "saul_reg.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+* @name   Set default configuration parameters for the HX711 driver
+* @{
+*/
+#ifndef HX711_PARAM_SCK

It would be preferable to group all these parameters into some `hx711_params_t` structure, and then pass it to the initialization function. See [Device descriptor and parameter configuration](https://github.com/RIOT-OS/RIOT/wiki/Guide:-Writing-a-device-driver-in-RIOT#device-descriptor-and-parameter-configuration) section on our wiki.

> +extern "C" {
+#endif
+
+/**
+* @name   Set default configuration parameters for the HX711 driver
+* @{
+*/
+#ifndef HX711_PARAM_SCK
+#define HX711_PARAM_SCK     GPIO_PIN(0, 0)
+#endif
+#ifndef HX711_PARAM_DOUT
+#define HX711_PARAM_DOUT    GPIO_PIN(0, 0)
+#endif
+/**
+ * @note Read the HX711 datasheet for more information
+ * CHANNEL_A_128 => 1

Maybe define an enum for these values, so they are documented.

> +
+/**
+ * @brief Needs to wait for device to be ready while initialization
+ */
+void wait_for_ready(void)
+{
+    while(gpio_read(HX711_PARAM_DOUT) > 0){
+        xtimer_usleep(HX711_PARAM_SLEEP_TIME);
+    }
+}
+
+/**
+ * @brief Gets a single 24-Bit value from the HX711.
+ * @return The 24-Bit shift in value
+ */
+int32_t hx711_read(void)

Are `hx711_read`, `hx711_read_average`, `hx711_get_value` supposed to be private functions? If so, they should be named like `_read` and declared static. The same for `wait_for_ready`. If, on the other hand, they are part of the public API then please add them to the header and move the documentation there.

> +/**
+ * @brief Read a value a configurable times and return the average value. Always rounded up.
+ * @param times
+ * @return returns AVG(RAW_VALUE) - HX711_PARAM_OFFSET
+ */
+int32_t hx711_get_value(uint8_t times)
+{
+    return (int32_t) ((hx711_read_average(times) - HX711_PARAM_OFFSET) + 0.5);
+}
+
+/**
+ * @brief Read the average of a configurable times of a cleared and scaled value. Always rounded up.
+ * @param times
+ * @return returns (AVG(RAW_VALUE) - HX711_PARAM_OFFSET) / HX711_PARAM_SCALE
+ */
+int32_t hx711_get_units(uint8_t times)

Leave documentation only on the header file.

> +/**
+ * @brief Power down the HX711 IC
+ */
+void hx711_power_down(void);
+
+/**
+ * @brief Power up the HX711 IC
+ */
+void hx711_power_up(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* HX711_H */
+/** @} */

Missing newline.

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

I don't think an example application is needed for this. The test application should be enough.

> @@ -199,6 +199,11 @@ ifneq (,$(filter hts221,$(USEMODULE)))
   FEATURES_REQUIRED += periph_i2c
 endif
 
+ifneq (,$(filter hx711,$(USEMODULE)))
+  FEATURES_REQUIRED += periph_gpio

Doesn't this module also depend on the gpio_util module as well?

> @@ -0,0 +1 @@
+include $(RIOTBASE)/Makefile.base

Missing newline.

> +
+#include <stdint.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief Initialization of the connected HX711 IC
+ */
+void hx711_init(void);
+
+
+/**
+ * @brief Read the average of a configurable times of a cleared and scaled value. Always rounded up.
+ * @param times

Please add a description for `times`

> +#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief Initialization of the connected HX711 IC
+ */
+void hx711_init(void);
+
+
+/**
+ * @brief Read the average of a configurable times of a cleared and scaled value. Always rounded up.
+ * @param times
+ * @return returns (AVG(RAW_VALUE) - HX711_PARAM_OFFSET) / HX711_PARAM_SCALE
+ */
+int32_t hx711_get_units(uint8_t times);

Is the average always needed? Why not provide a simple read function that returns the value already affected by the offset and the scale?
Also, the function name seems a little bit unrelated to its functionality. 

> + * @}
+ */
+
+#include <string.h>
+#include <stdio.h>
+
+#include "saul.h"
+#include "hx711.h"
+#include "hx711_params.h"
+
+static int read_weight(const void *dev, phydat_t *res)
+{
+    (void)dev;
+    uint32_t result = hx711_get_units(HX711_PARAM_AVG_TIMES);
+    res->val[0] = (int16_t) result;
+    res->unit = UNIT_G;

Does `hx711_get_units` already return the value in grams?

-- 
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/11416#pullrequestreview-229559543
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190423/58d52b1d/attachment.html>


More information about the notifications mailing list