drivers: Add driver for the LTC4150 coulomb counter by maribu · Pull Request #9653 · RIOT-OS/RIOT

maribu · 2018-07-31T15:44:04Z

This PR adds support for the LTC4150 coulomb counter (a.k.a. bidirectional current sensor or battery gauge) and integrates this driver into SAUL.

On the MSBA2 this driver can be easily tested using the saul shell command in the default example. There is also an open hardware breakout board that can be easily attached to any 3.3V or 5V board.

This driver is implemented in interrupt driven mode. Thus, only 1 GPIO (with interrupt support) is needed in case the user does not want to power down the LTC4150 and no battery is connected. (In case a battery is connected one GPIO is required to detect if the battery is charged or discharged. Another GPIO is required if the LTC4150 should be powered off.)

Note: An LTC4150 driver was already part of RIOT, but was dropped. This driver does not inherit any code from the depreciated and removed driver.

PeterKietzmann · 2018-07-31T17:39:47Z

@MichelRottleuthner interested?

maribu · 2018-08-01T06:44:03Z

I want to add a bugfix and an improvement to this PR. Please wait with reviewing...

maribu · 2018-08-01T08:10:04Z

Done. Changes include:

Some bugfixes
Improved documentation
Added pseudomodule ltc4150_battery. Without this module the memory footprint (mostly ram) is reduced by not keeping track in which direction (charging/discharging) the charge was transferred. This information is only relevant in case a battery is used which is charged while RIOT is running, hence the module name
The API is the same no matter if ltc4150_battery is used or not. (Without ltc4150_battery the return values for the charge in "charging" direction is hardcoded to zero.) This should make using the driver easier.

MichelRottleuthner

Thanks for this PR! Looking forward to use this driver :)
Not tested yet. Parts of the interface could use more self-explaining names. The averaging features look like quite some overhead and I'm not sure if everybody needs it. See below for details.

MichelRottleuthner · 2018-08-03T07:55:52Z

drivers/include/ltc4150.h

+ * # Wiring the LTC4150
+ * Please note that this driver works interrupt driven and does not clear the
+ * signal. Thus, the CLR and INT pins on the LTC4150 need to be connected
+ * (solder jumper SJ1 closed on the breakout board), so that the signal is


Detailed documentation is nice but this is a driver for LTC4150, not some specific breakout board, right? Maybe a link to specific documentation and/or an explicit statement on what exact board you are referring to would be better.

M Grusin released his breakout board schematics as open hardware. So, every available breakout boards uses this schematics, even the cheapest clones. So this basically leaves two flavors how the LTC4150 is used, either as the breakout board or embedded in the boards (e.g. like in the MSBA2). I could remove the hint for the breakout board, if you prefer

Maybe I just add the link to the schematics and state that as of now virtually every breakout board uses that schematics?

MichelRottleuthner · 2018-08-03T08:02:59Z

drivers/include/ltc4150.h

+ * for interrupts, /SHDN and POL (if connected) do not require interrupt
+ * support.
+ *
+ * In case a battery is used, the module `ltc4150_battery` should also be added


not sure if ltc4150_battery is a unambiguous name. First you can not only measure batteries but any kind of current source/sink. Second, the name doesn't really make clear that the important feature of the module is the bidirectional measuring.

How about ltc4150_bidirectional?

MichelRottleuthner · 2018-08-03T08:14:43Z

drivers/include/ltc4150.h

+ * 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, so this effect should hopefully be negligible.


Just a thought for the future: would be nice to combine this with a hardware counter...

MichelRottleuthner · 2018-08-03T08:26:55Z

drivers/include/ltc4150.h

+ */
+typedef struct {
+    uint32_t total;         /**< Total number of pulses */
+    uint16_t sec[60];       /**< Running average over each second in the last minute */


This is quite a memory overhead and probably not everyone needs this feature. Maybe this should be optional or at least let the number of seconds be changeable by define. Another way would be to just track the counter and let the user decide on how much (and when) temporal resolution is needed by calling the read function more often.

Actually the motivation wasn't to keep track of each second and rather to allow updating the last minute value in O(1), so that the ISR is not too expensive.

I just checked how many ticks I expected in a second as worst case. Lets assume we estimate the maximum current to 0.1A, then we would choose 0.05V / 0.1A = 0.5Ohm as R_sense. If we now would have a current of 1A (ten times higher than estimated maximum), then we would get 32.55*0.5 = 16.275 ticks per coulomb (or 16.275 ticks per second with a current of 1A). Hence, it seems pretty safe to assume at most 20 ticks per second will occur. Thus, to achieve O(1) updates for the last minute counter only one byte per every ten seconds could be used (instead 2 bytes per second), or 6 bytes instead of 120 bytes.

Also: The per second value seems to be of little value, as e.g. the MSBA2 gets 0-1 ticks per second without any other hardware attached. I think this is simple to few sample to be of any value at all.

6 bytes instead of 120 sounds much better. Just tracking the counter would simplify that but I see that it's nice to have that functionality - especially for the provided saul implementation.

MichelRottleuthner · 2018-08-03T08:28:11Z

drivers/include/ltc4150.h

+    gpio_t interrupt;
+#ifdef MODULE_LTC4150_BATTERY
+    /**
+     * @brief Pin indicating (dis-)charging, labeled POL


maybe add info on which polarity is indicating what, like above?

MichelRottleuthner · 2018-08-03T09:55:22Z

drivers/ltc4150/ltc4150.c

+    return 0;
+}
+
+int ltc4150_deinit(ltc4150_dev_t *dev)


since you are calling the function deinit, does it make sense to also clear the state in the device struct? Otherwise, something like power{off|down} may be a better name.

MichelRottleuthner · 2018-08-03T09:58:27Z

drivers/ltc4150/ltc4150.c

+        return -EINVAL;
+    }
+
+    irq_flags = irq_disable();


I think using a mutex would be better than disabling all interrupts

I actually thought about just doing that. But then it would be required to acquire the mutex from the ISR. I'm not sure about how this is in RIOT, but in general it seemed to me as a bad idea to acquire a mutex in interrupt context.

right, I also just saw that mutex lock also disables irqs. hmm.. using messages comes to my mind, but that needs a thread.

Using atomic accesses could be used with very careful thought. But maybe that would be too hard to debug and to get right.

maybe (parts of) the pulse counter can be reused

Without the functionally to keeping track of the charge transferred within the last minute the pulse counter could be used as it is. But I think keeping that data while trying to reuse code would result in more lines of code.

MichelRottleuthner · 2018-08-03T09:59:49Z

drivers/ltc4150/ltc4150.c

+        off -= 60;
+    }
+
+    irq_flags = irq_disable();


same as above, avoid disabling irqs if not absolutely necessary

MichelRottleuthner · 2018-08-03T10:02:11Z

drivers/ltc4150/ltc4150_saul.c

+        res->unit = UNIT_COULOMB;
+#ifdef MODULE_LTC4150_BATTERY
+        /* Changing scale until value fits int16_t */
+        while ((temp[1] > INT16_MAX) || (temp[2] > INT16_MAX)) {


phydat_fit could be your friend ;)

The API of phydat_fit is way more complex to use than needed. I think this would make more sense:

void phydat_fit(phydat_t *dat, long *values, unsigned dim)

Or with only three dimensions maybe even

void phydat_fit1(phydat_t *dat, long value)

void phydat_fit2(phydat_t *dat, long val1, long val2)

void phydat_fit3(phydat_t *dat, long val1, long val2, long val3)

I'll prepare a separate PR for that.

Done. I'm waiting for that PR to be evaluated. If it get's merged, I can rebase against the master and use the new phydat_fit api.

MichelRottleuthner · 2018-08-03T10:03:51Z

drivers/ltc4150/ltc4150_saul.c

+    int32_t temp[3];
+
+    if (ltc4150_accumulated_second(dev, temp, 0) == 0) {
+        /* Even with maximum allowed current int16_t will not overflow here */


is this also true for other shunt values on custom ltc4150 boards?

The result is the charge transferred in micro coulomb within the last second. Thus up to 32.767 coulomb can fit into int16_t. In order to overflow more than 32.767 ampere current would be required - I guess most boards would just melt down with that current. So I think the assumption should be true for any use case :-)

that's a good point ;)

maribu · 2018-08-06T16:24:01Z

I'll update the PR based on the proposed phydat_fit API change to move this PR forward. (But I'm aware that this PR must not be merged until the new phydat_fit API is either accepted and this PR is rebased against the new master, or the phydat_fit API change is rejected and this PR is adapted to the current phydat_fit API.)

maribu · 2018-08-07T14:51:08Z

I think I address all issues :-) I already squashed the commits, because of the major rewrite the history was quite unreadable. But in my -dev branch the full history is still available.

maribu · 2018-10-26T11:33:42Z

@MichelRottleuthner: I rebased this PR on top of the now merged new phydat_fit API. Sadly, due to an unrelated issue, currently this PR will not work on the MSB-A2. For testing on the MSB-A2 I recommend to either disable the sht1x module or to apply this PR.

Thank you for your patience!

maribu · 2018-11-06T14:56:41Z

@MichelRottleuthner: I just rebased against current master. Thus, testing on the MSB-A2 can now be done without any workaround.

The shell handlers of the old, depreciated and removed LTC4150 driver are still in place. This commit removes them

maribu · 2018-12-13T09:35:44Z

@MichelRottleuthner: I rebased again against current master to fix some merge conflicts.

maribu · 2018-12-13T09:55:14Z

@MichelRottleuthner: I found two issues and I apparently forgot to address one of your comments. Those issues are now address. May I squash?

maribu · 2018-12-18T15:32:39Z

@MichelRottleuthner: Ping?

MichelRottleuthner · 2018-12-18T16:13:44Z

sorry for the delay. Yes! go ahead and squash I'll see if I can test it on hardware today

The LTC4150 is a coulomb counter (a.k.a. battery sensor or bidirectional current sensor) that is used in the MSBA2 board and available for little money as easy to use break out board.

MichelRottleuthner · 2018-12-18T20:11:11Z

I ran a test with a sparkfun breakout board in both configurations (ltc4150_bidirectional and ltc4150) - looks good to me. The only thing I noticed is that the average current value is not of much value due to its calculation over the whole runtime. But I think people have different opinions on this. ACK from my side

MichelRottleuthner · 2018-12-19T12:08:14Z

@maribu can you have a look at the murdock output? It seems the saul test now uses too much memory on arduino-duemilanove and arduino-uno... This makes me wonder if (the string stuff of) the current saul abstraction is suboptimal in general. Looking at phydat_str.c or even worse saul_str.c shows that for all kinds of sensors the string representation is always compiled in. Though, its probably rarely the case that an application uses more than a handful of them.

maribu · 2018-12-26T20:12:59Z

Thanks for the review.

It seems the saul test now uses too much memory on arduino-duemilanove and arduino-uno.

Indeed 😦

This makes me wonder if (the string stuff of) the current saul abstraction is suboptimal in general. Looking at phydat_str.c or even worse saul_str.c shows that for all kinds of sensors the string representation is always compiled in.

I'm not sure if there is much room for improvement. One could try the following:

Put every string into #ifdef MODULE_FOO ... #endif
- Pros:
  - Reduces the used ROM size when only few sensors/actuators are in use
- Contras:
  - Really messy and probably buggy
Don't use a switch() over some enum to get the string, but instead have all strings as const char * symbols available. E.g. a saul_driver_t could have an additional member const char *type_str that would point to the corresponding "global" string.
- Pros:
  - The linker would reliably garbage collect unneeded strings
  - No messy #ifdefs, no need for manual dependency tracking
  - More efficient lookup of strings
  - The "to string"-function can be dropped, which reduces ROM size even more
- Contras:
  - 4 additional bytes of overhead on 32 bit architectures for the pointer to the string
  - The "to string" functions are likely only used by the shell command. Not having the to string function would currently garbage collect this function and all strings. With this change, the strings of the used sensors/actuators would be compiled in no matter if they are used or not

Only approach 2 would be better than the current approach in my opinion, but only when the "to-string" functions are actually used. When flash is extremely scarce, the "to-string" functions are likely not used at all. In this case, approach 2. would increase ROM size instead of reducing it...

maribu · 2018-12-27T09:11:00Z

The only thing I noticed is that the average current value is not of much value due to its calculation over the whole runtime.

I have to admit that this driver was written with the ComSys Testbed in mind, in which the natural way would be to compare the power efficiency of different images one after another. (E.g. first one test would be 30 minutes running image A, then collect the result. The same would be repeated for images B, C etc.)

A way of resetting the counter would improve the situation. But honestly, I now think it might be better to drop all but the total coulomb counting feature and allow adding a user defined callback function instead. That would allow the user to gather the data she/he is interested in. Some predefined callback functions could also be implemented by the driver, e.g. the last minute charge as is already present. Those callbacks would be garbage collected by the linker, if unused - so no need of pseudo modules and #ifdefs to prevent them to bloat the ROM size.

What do you think?

Update: Here is a quick proof of concept. Beware: Completely untested ;-)

smlng · 2019-01-07T16:10:35Z

drivers/include/ltc4150.h

+ *
+ * @author      Marian Buschsieweke <marian.buschsieweke@ovgu.de>
+ *
+ * # Wiring the LTC4150


this detailed text should go below the first @brief and before @{ to be directly visible in the doxygen html output.

smlng · 2019-01-07T16:11:52Z

drivers/include/ltc4150.h

+#ifndef LTC4150_H
+#define LTC4150_H
+
+#include <mutex.h>


don't use system style include for RIOT headers use #include "<name>.h" instead, also (we mostly) order sys-includes before RIOT includes, hence move <stdint.h�> up

maybe also separated by an empty line from the RIOT headers.

drivers/ltc4150/include/ltc4150_params.h

smlng · 2019-01-07T16:17:42Z

drivers/ltc4150/include/ltc4150_params.h

+#define LTC4150_PARAM_SHUTDOWN        (GPIO_PIN(0, 5))
+#endif
+#ifndef LTC4150_PARAM_PULSES
+#define LTC4150_PARAM_PULSES          (45700)


here parentheses are good, but add better write (45700UL) to explicitly make it unsigned

I do not agree on the parentheses remarks. In my opinion there should be a reasonable rule to where to add them. A reasonable rule to could be one of the following to me:

add enclosing parentheses in every preprocessor macro, just to be safe

add enclosing parentheses when order of precedence bugs can be introduced

A bug could be introduced e.g. in:

#define FOO 3 + 4 int bar = 2 * FOO; /* (2 * 3) + 4 not 2 * (3 + 4) */

But a precedence bug cannot be introduced by an integer constant. So unless adding enclosing parentheses everywhere, no parentheses are needed here. But if enclosing parentheses would be needed everywhere, they should also be placed around GPIO_PIN(0, 4).

fine, but then leave parentheses in place as is.

TL;DR: go for rule 1

Regarding the UL suffix I also disagree. According to the C99 standard in section 6.4.4.1 a decimal constant without any suffix will have the smallest integer type of the following list: int, long int, long long int. Suffixes will only be needed in two cases:

The decimal number does fit into unsigned long long int, but not into long long int

The decimal number is used in expressions in which could overflow the automatically determent type of the number, e.g. uint64_t foo = 42 << 32;

(Explaining 2.: The 42 would fit into int, thus this would be the determent type. However, shifting any int by 32 bits to the left will result in 0. Thus, foo will have the value 0 here. Using uint64_t foo = 42ULL << 32 would have the expected result - so here the suffix would be needed for correct operation.)

Suffixes are really rarely needed. But they make reading the code harder. Adding them only were actually harm could occur will be more readable in my opinion and raise awareness of the actual reason, why the suffixes are needed in some places.

to clarify (from my side) on the suggested UL suffix:

we had lots of issue with signed/unsigned comparison errors/warnings, hence the U

RIOT also supports 8bit platforms such as the atmega arduinos and (IIRC) it was mostly required to explicitly state UL, bc w/o any suffix or just U 45700 will not fit into a 16bit int.

maybe U only would also do for the arduinos, but not sure about that

Good point with the signed/unsigned comparison. There should be no issue with the Arduino, as the compiler will (according to the standard) just fall back to long int or even long long int if the decimal number is too big.

But honestly: I believe if the compiler does complain about signed/unsigned comparison with a constant decimal, I would consider this as a compiler bug. The compiler is aware of the value of the constant decimal number and of the type of the other side of the comparison. This should be enough to tell for sure if the comparison will work as expected or not.

smlng · 2019-01-07T16:18:15Z

drivers/ltc4150/include/ltc4150_params.h

+                                      { .name = "LTC4150 last minute charge" }, \
+                                      { .name = "LTC4150 average current" }
+#endif
+


remove empty line

smlng · 2019-01-07T16:18:31Z

drivers/ltc4150/include/ltc4150_params.h

+                                      { .name = "LTC4150 average current" }
+#endif
+
+/**@}*/


add spaces around @}

smlng · 2019-01-07T16:19:46Z

drivers/ltc4150/ltc4150.c

+
+/**
+ * @ingroup     drivers_ltc4150
+ * @brief       Driver for the Linear Tech LTC4150 Coulomb Counter


this will not be seen or override the short text of the doxygen group, either move to the @defgroup or to the @brief below

smlng · 2019-01-07T16:21:07Z

drivers/ltc4150/ltc4150.c

+
+int ltc4150_init(ltc4150_dev_t *dev, const ltc4150_params_t *params)
+{
+    timex_t now;


don't use that anymore

smlng · 2019-01-07T16:22:14Z

drivers/ltc4150/ltc4150.c

+        return -EIO;
+    }
+
+    xtimer_now_timex(&now);


use uint64_t now = xtimer_now_usec64() / US_PER_SEC; or its 32 bit variant if sufficient

maribu · 2019-01-08T10:32:50Z

@smlng: Thank you for your comments. They were very helpful. Based on @MichelRottleuthner feedback I came up with a different approach of this driver placed here which I now favor over the code in this PR. You remarks apply to that branch as well. (I will fix it there and then squash.)

The difference is that the last minute counter is removed and the possibility to add callbacks was added. This makes this driver more lightweight when only the total charge drawn is of interest and still allows to track more precise information (e.g. last minute charge) by adding a callback function for that.

smlng · 2019-01-08T10:40:31Z

@maribu: yep the new driver looks way cleaner, I also like that you got rid of the #ifdefs. So, open a new PR reference this one but leave it open for comparison ... close it later.

maribu · 2019-01-08T10:44:07Z

open a new PR reference this one but leave it open for comparison ... close it later.

Excellent idea. I wonder why that didn't occur to me in the first place

miri64 · 2019-01-18T12:17:07Z

Closed in favor of #10755.

PeterKietzmann added the Area: drivers Area: Device drivers label Jul 31, 2018

PeterKietzmann requested a review from MichelRottleuthner July 31, 2018 19:31

maribu force-pushed the ltc4150 branch from 0f2b246 to 392d89c Compare August 1, 2018 07:52

MichelRottleuthner requested changes Aug 3, 2018

View reviewed changes

maribu force-pushed the ltc4150 branch from 4695062 to acb6ce9 Compare August 7, 2018 14:44

maribu force-pushed the ltc4150 branch from acb6ce9 to b15907d Compare August 23, 2018 09:33

maribu mentioned this pull request Sep 19, 2018

sys/phydat: New phydat_fit API #9702

Merged

maribu force-pushed the ltc4150 branch from b15907d to a2af819 Compare October 26, 2018 11:30

maribu force-pushed the ltc4150 branch from a2af819 to ccb29f9 Compare November 6, 2018 14:34

maribu added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Dec 5, 2018

maribu added 3 commits December 13, 2018 10:20

sys/shell: Removed fragments of old driver

31fd76d

The shell handlers of the old, depreciated and removed LTC4150 driver are still in place. This commit removes them

sys/phydat: Added unit Coulomb

1840da0

drivers/saul: Added coulomb counter & ammeter

b70789d

maribu force-pushed the ltc4150 branch from ccb29f9 to 9b197d8 Compare December 13, 2018 09:33

maribu added 3 commits December 18, 2018 17:35

drivers/ltc4150: (Re-)implemented LTC4150 driver

4366674

The LTC4150 is a coulomb counter (a.k.a. battery sensor or bidirectional current sensor) that is used in the MSBA2 board and available for little money as easy to use break out board.

drivers/saul: Integrated ltc4150

4d45093

boards/msba2: Added ltc4150 to saul_default

4171bf8

maribu force-pushed the ltc4150 branch from 75d53d9 to 4171bf8 Compare December 18, 2018 16:36

MichelRottleuthner approved these changes Dec 18, 2018

View reviewed changes

MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 18, 2018

tests/saul: Updated BOARD_INSUFFICIENT_MEMORY

34825f0

smlng suggested changes Jan 7, 2019

View reviewed changes

fixup! Addressed @smlng's comments

61c475d

maribu mentioned this pull request Jan 11, 2019

drivers/ltc4150: (Re-)implemented driver for the LTC4150 coulomb counter #10755

Merged

fixup! Using uint64_t in calucation to prevent overflow

2ef7aaf

maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 11, 2019

miri64 closed this Jan 18, 2019

maribu deleted the ltc4150 branch March 8, 2019 16:42

Conversation

maribu commented Jul 31, 2018

Uh oh!

PeterKietzmann commented Jul 31, 2018

Uh oh!

maribu commented Aug 1, 2018

Uh oh!

maribu commented Aug 1, 2018

Uh oh!

MichelRottleuthner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu commented Aug 6, 2018

Uh oh!

maribu commented Aug 7, 2018

Uh oh!

maribu commented Oct 26, 2018

Uh oh!

maribu commented Nov 6, 2018

Uh oh!

maribu commented Dec 13, 2018

Uh oh!

maribu commented Dec 13, 2018

Uh oh!

maribu commented Dec 18, 2018

Uh oh!

MichelRottleuthner commented Dec 18, 2018

MichelRottleuthner left a comment •

edited

Loading

maribu Aug 3, 2018 •

edited

Loading

maribu commented Dec 27, 2018 •

edited

Loading

smlng Jan 8, 2019 •

edited

Loading