[riot-notifications] [RIOT-OS/RIOT] drivers/ltc4150: (Re-)implemented driver for the LTC4150 coulomb counter (#10755)

Sebastian Meiling notifications at github.com
Tue Jan 15 17:18:42 CET 2019


smlng requested changes on this pull request.



> + *
+ * In case a battery is used the POL pin connected to another GPIO. This allows
+ * to distinguish between charge drawn from the battery and charge transferred
+ * into the battery (used to load it).
+ *
+ * In case support to power off the LTC4150 is desired, the /SHDN pin needs to
+ * be connected to a third GPIO.
+ *
+ * # Things to keep in mind
+ * The LTC4150 creates pulses with a frequency depending on the current drawn.
+ * Thus, more interrupts need to be handled when more current is drawn, which
+ * in turn increases system load (and power consumption). The interrupt service
+ * routing is quite short and even when used outside of specification less than
+ * 20 ticks per second will occur. Hence, this effect should hopefully be
+ * negligible.
+ * @{

minor nit: add new/empty line before `@{`.

> +        for (unsigned i = 0; dev->params.recorders[i] != NULL; i++) {
+            dev->params.recorders[i]->pulse(dev, dir, now,
+                                            dev->params.recorder_data[i]);
+        }
+    }
+
+    dev->last_update_sec = now / US_PER_SEC;
+}
+
+int ltc4150_init(ltc4150_dev_t *dev, const ltc4150_params_t *params)
+{
+    if (!dev || !params) {
+        return -EINVAL;
+    }
+
+    memset(dev, 0x00, sizeof(ltc4150_dev_t));

this shouldn't be necessary and also avoidable, see discussion in https://github.com/RIOT-OS/RIOT/issues/10751 or https://github.com/RIOT-OS/RIOT/pull/10728

> +
+static void init_or_reset(ltc4150_dev_t *dev, uint64_t now_usec, void *arg);
+static void pulse(ltc4150_dev_t *dev, ltc4150_dir_t dir, uint64_t now_usec,
+                  void *arg);
+
+const ltc4150_recorder_t ltc4150_last_minute = {
+    .reset = init_or_reset,
+    .pulse = pulse,
+};
+
+static void init_or_reset(ltc4150_dev_t *dev, uint64_t now_usec, void *arg)
+{
+    (void)dev;
+    ltc4150_last_minute_data_t *data = arg;
+
+    memset(data, 0, sizeof(ltc4150_last_minute_data_t));

more readable (and should do the same): ``` data = {0};```

> +static void spin(uint32_t seconds)
+{
+    uint32_t till = xtimer_now_usec() + US_PER_SEC * seconds;
+    while (xtimer_now_usec() < till) { }
+}
+
+/**
+ * @brief Thread that will put three levels of CPU load on the MCU
+ */
+static void *busy_thread(void *arg)
+{
+    (void)arg;
+    while (1) {
+        /* one minute of ~0% CPU usage */
+        LED0_OFF;
+        LED1_OFF;

not all boards will have 2 leds hence `LED1_` macros might be undefined, or at least not functional. 

> @@ -5,4 +5,7 @@ USEMODULE += saul_default
 
 USEMODULE += xtimer
 
+# Too little flash:
+BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-uno

that's only partly true, bc the plain `tests/saul` does fit on both boards, it might just not fit with `USEMODULE= ltc4150` - but then again the onboard LED(s) or button(s) do work with SAUL. Output for Arduino-uno:

```
2019-01-15 16:37:00,922 - INFO # Dev: LED	Type: ACT_SWITCH
2019-01-15 16:37:00,942 - INFO # Data:	          0
2019-01-15 16:37:00,942 - INFO # 
2019-01-15 16:37:00,971 - INFO # ##########################
```

> +     * @brief External pull on the /POL pin is present
+     */
+    LTC4150_POL_EXT_PULL_UP = 0x02,
+    /**
+     * @brief External pull on the /INT *and* the /POL pin is present
+     */
+    LTC4150_EXT_PULL_UP = LTC4150_INT_EXT_PULL_UP | LTC4150_POL_EXT_PULL_UP,
+};
+
+/**
+ * @brief Enumeration of directions in which the charge can be transferred
+ */
+typedef enum {
+    LTC4150_CHARGE,             /**< The battery is charged */
+    LTC4150_DISCHARGE,          /**< Charge is drawn from the battery */
+} ltc4150_dir_t;

so its either `charge` or `discharge`, never both, right? So why not use a simple `bool charge` with `true` and `false` in the implementation. To me that would be more readable, e.g. by omitting `switch () case`.

> +     * @brief External pull on the /POL pin is present
+     */
+    LTC4150_POL_EXT_PULL_UP = 0x02,
+    /**
+     * @brief External pull on the /INT *and* the /POL pin is present
+     */
+    LTC4150_EXT_PULL_UP = LTC4150_INT_EXT_PULL_UP | LTC4150_POL_EXT_PULL_UP,
+};
+
+/**
+ * @brief Enumeration of directions in which the charge can be transferred
+ */
+typedef enum {
+    LTC4150_CHARGE,             /**< The battery is charged */
+    LTC4150_DISCHARGE,          /**< Charge is drawn from the battery */
+} ltc4150_dir_t;

plus it saves 16 Bytes, when using `bool` instead of the `enum` ...

but I leave it to you, that's a non-blocking suggestion

> +     * @brief External pull on the /POL pin is present
+     */
+    LTC4150_POL_EXT_PULL_UP = 0x02,
+    /**
+     * @brief External pull on the /INT *and* the /POL pin is present
+     */
+    LTC4150_EXT_PULL_UP = LTC4150_INT_EXT_PULL_UP | LTC4150_POL_EXT_PULL_UP,
+};
+
+/**
+ * @brief Enumeration of directions in which the charge can be transferred
+ */
+typedef enum {
+    LTC4150_CHARGE,             /**< The battery is charged */
+    LTC4150_DISCHARGE,          /**< Charge is drawn from the battery */
+} ltc4150_dir_t;

@MichelRottleuthner what do you think?

> +
+/**
+ * @name    Set default configuration parameters for the LTC4150
+ * @{
+ */
+#ifndef LTC4150_PARAM_INT
+#define LTC4150_PARAM_INT             (GPIO_PIN(0, 4))
+#endif
+#ifndef LTC4150_PARAM_POL
+#define LTC4150_PARAM_POL             (GPIO_UNDEF)
+#endif
+#ifndef LTC4150_PARAM_SHUTDOWN
+#define LTC4150_PARAM_SHUTDOWN        (GPIO_PIN(0, 5))
+#endif
+#ifndef LTC4150_PARAM_PULSES
+#define LTC4150_PARAM_PULSES          (45700)

didn't we agree on adding (at least) a `U` suffix in the previous PR?

> +#include <string.h>
+
+#include "ltc4150.h"
+#include "xtimer.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+static void pulse_cb(void *_dev)
+{
+    uint64_t now;
+    ltc4150_dir_t dir;
+    ltc4150_dev_t *dev = _dev;
+
+    if (
+        (dev->params.polarity == GPIO_UNDEF) ||

personally I would not put this on a new line, as the indention stays the same, hence I think its fine to write this as:

```
if ((dev->params.polarity == GPIO_UNDEF) ||
    (!gpio_read(dev->params.polarity))) {
```

-- 
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/10755#pullrequestreview-192698140
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190115/d4a8f05e/attachment-0001.html>


More information about the notifications mailing list