drivers/ltc4150: (Re-)implemented driver for the LTC4150 coulomb counter by maribu · Pull Request #10755 · RIOT-OS/RIOT

maribu · 2019-01-11T15:49:14Z

Contribution description

This PR implements a driver for the LTC4150 coulomb counter. A driver for that device was previously part for RIOT, but was dropped because of quality shortcomings. This driver is written from scratch.

Testing procedure

The LTC4150 is integrated in the MSB-A2 board, thus using that board will be most convenient to use for testing. However, this chip is also cheaply available as breakout board from various vendors, which all seem to be based on the same open hardware PCB.

Testing:

Build, flash & run tests/driver_ltc4150 on the MSB-A2. I intended the program to be self-explaining - so I will not provide more information here to see if I indeed reached that goal
Build, flash & run example/default. Try to read the data using the saul shell command

Issues/PRs references

This PR competes with PR #9653 also opened by me. I personally favor this PR strongly over the old approach.

maribu · 2019-01-11T15:57:10Z

Before you start to review: Let me fix the missing doc Travis complained about and squash

maribu · 2019-01-11T21:18:36Z

I'm aware that commit "tests/driver_ltc4150: Workarround for msp430" could be squashed into commit "tests: Added test for ltc4150 driver". But I would actually prefer to keep them separated: That commit did not fix a bug in the test, but was a workarround for a bug in the msp430 toolchain (the missing fputs). Once the c lib of the msp430 toolchain gains support for fputs, it can be safely reverted to make the test source code shorter and more readable. For that reason I would like to keep that commit separate.

maribu · 2019-01-11T21:19:52Z

@MichelRottleuthner: Could you kindly do the testing? I think @smlng does not have a MSB-A2 board at hand (but I'm not sure).

miri64 · 2019-01-12T18:01:09Z

I believe @MichelRottleuthner doesn't have one either, but I have 8 ;-). Once the fundamentals and and code design are cleared (e.g. by @MichelRottleuthner or @smlng) I'm happy to test this PR.

drivers/include/ltc4150.h

smlng · 2019-01-15T15:26:57Z

drivers/ltc4150/ltc4150.c

+        return -EINVAL;
+    }
+
+    memset(dev, 0x00, sizeof(ltc4150_dev_t));


this shouldn't be necessary and also avoidable, see discussion in #10751 or #10728

Could you elaborate on that?

#10751 is about a potential security issue when sensitive data should be cleared but the compiler optimized out zeroing out the data. In this case it would be no problem at all if the compiler detects that e.g. dev->params is overwrite one line later anyway and skips zeroing out that data. And #10728 is about the use of memcpy, not about memset().

I would say this is a textbook example for when to use memset(): When you want to bring a structure into a defined state before using it. One could also using something like:

dev->params = *params; dev->start_sec = 0; dev->charged = 0; dev->discharged = 0;

But what happens when the structure is extended by one member? Using memset() would still work fine to initialize the whole structure, but the above could would only initialize parts of the struct.

yep, true that ... leave as is.

However, please be consistent and style/usage, i.e. here you write memset(..., 0x00 ...) and below memset(..., 0, ...) ... I think the latter is nicer.

However, please be consistent and style/usage, i.e. here you write memset(..., 0x00 ...) and below memset(..., 0, ...) ... I think the latter is nicer.

+1

drivers/ltc4150/ltc4150_last_minute.c

tests/driver_ltc4150/main.c

tests/saul/Makefile

drivers/include/ltc4150.h

smlng · 2019-01-15T16:14:38Z

drivers/ltc4150/include/ltc4150_params.h

+#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?

Indeed, sorry

smlng · 2019-01-15T16:16:34Z

drivers/ltc4150/ltc4150.c

+    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))) {

miri64 · 2019-01-16T13:08:33Z

@maribu just a remark: if you use the commit summary git commit --fixup=<commit> provides you with, squashing becomes easier due to git rebase -i <base> --autosquash ;-).

miri64

Tests work as expected, though I was confused due to text speaking about drawn charges, while the "Charging" column was raising constantly.

maribu · 2019-01-16T13:17:52Z

Tests work as expected, though I was confused due to text speaking about drawn charges, while the "Charging" column was raising constantly.

Ohh! Let me check

maribu · 2019-01-16T13:22:41Z

@miri64: Fixed :-) I put the values in the wrong columns

miri64 · 2019-01-16T13:24:36Z

OK, this makes more sense, but my confusion stemmed more from the fact, that I connect "more power" with "more charge drawn", but that obviously isn't what the text meant (especially since it stresses "The expected result is that the required current increases with the system load". However, just from the stable I'm still not able to see exactly when the current is supposed to raise. Maybe add another

+---------------+---------------+---------+-------------+-----------+

Whenever the corresponding minute has passed?

miri64 · 2019-01-16T13:24:58Z

(I intentionally didn't look at the test's code yet)

maribu · 2019-01-16T14:12:30Z

@miri64: I added the line between load levels and tried to make the description clearer.

tests/driver_ltc4150/main.c

miri64 · 2019-01-16T14:28:02Z

Ok, now I can see the raising current clearer. Maybe add a hint, that it is the right column the tester is supposed to look at. Also just from the description it wasn't clear to me, that the test cycles and not just ends after the 3rd level.

maribu · 2019-01-16T14:40:09Z

@miri64: Addressed :-)

maribu · 2019-01-18T11:39:01Z

May I squash?

Do we all agree that this PR is better than the competing PR at #9653? If so, I would close #9653

MichelRottleuthner · 2019-01-18T12:14:57Z

I prefer this PR over the old one. Regarding the squash @miri64 and @smlng should decide because they requested the changes. From my side it's good to go!

miri64 · 2019-01-18T12:16:16Z

I hadn't looked at the code as much, just tested and my comments were addressed. So I'm fine with squashing as well.

miri64 · 2019-01-18T12:16:35Z

(you should wait for @smlng's go though)

maribu · 2019-01-28T11:44:16Z

@smlng: May I squash?

smlng · 2019-01-28T12:30:44Z

sure, please squash

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

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.

Implemented an example `ltc4150_recorder_t` implementation as a proof of concept for the recorder API.

The msp430 toolchain is missing an `fputs()` implementation. This commit makes them use the `printf("%s", str);` instead of `fputs(str, stdout);`, which is semantically equivalent (but has more overhead).

maribu · 2019-01-28T13:00:33Z

Hmm, the "force-pushed" link is not clever enough to filter out changes due to rebasing against current master. I still have a back up here https://github.com/maribu/RIOT/tree/ltc4150-new-prior-squash for reference, which is neither squashed nor rebased (in other words: Contains the state prior force push).

miri64 · 2019-01-28T13:05:38Z

Hmm, the "force-pushed" link is not clever enough to filter out changes due to rebasing against current master.

That's why I usually just squash (i.e. make a rebase against the merge-base with master) if it is not absolutely necessary [to rebase]. It's not about cleverness. The link is just an HTML-ized version of git diff <pre>..<post>.

maribu · 2019-01-28T13:24:53Z

The link is just an HTML-ized version of git diff <pre>..<post>.

If merging of the old state into upstream can be done automatically, this approach would need to be only slightly adapted to be more clever: Use git diff <pre merged into upstream> .. <post> if the force push did a rebase.

miri64 · 2019-01-28T13:30:45Z

Welp, that's for @github to resolve ;-)

miri64 · 2019-01-28T14:05:48Z

Re-run the tests. Still working.

maribu · 2019-01-28T15:01:41Z

@miri64, @MichelRottleuthner, @smlng: Thank you very much for your valuable feedback :-)

maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 11, 2019

maribu requested review from MichelRottleuthner and smlng January 11, 2019 15:49

maribu force-pushed the ltc4150-new branch from 962743c to fbf1986 Compare January 11, 2019 15:59

smlng suggested changes Jan 15, 2019

View reviewed changes

smlng self-assigned this Jan 16, 2019

miri64 reviewed Jan 16, 2019

View reviewed changes

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 16, 2019

miri64 reviewed Jan 16, 2019

View reviewed changes

tests/driver_ltc4150/main.c Outdated Show resolved Hide resolved

miri64 mentioned this pull request Jan 18, 2019

drivers: Add driver for the LTC4150 coulomb counter #9653

Closed

smlng approved these changes Jan 28, 2019

View reviewed changes

maribu added 11 commits January 28, 2019 13:33

sys/shell: Removed fragments of old driver

d180339

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

sys/phydat: Added unit Coulomb

cc0e6dc

drivers/saul: Added coulomb counter & ammeter

ac98c98

tests/saul: Updated BOARD_INSUFFICIENT_MEMORY

5acd87b

drivers/ltc4150: (Re-)implemented LTC4150 driver

c00886a

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

c434c5c

boards/msba2: Added ltc4150 to saul_default

fa0d08a

drivers/ltc4150: Allow tracking last minute charge

db0c66e

Implemented an example `ltc4150_recorder_t` implementation as a proof of concept for the recorder API.

tests: Added test for ltc4150 driver

618e2e5

tests/driver_ltc4150: Workarround for msp430

2ba57ac

The msp430 toolchain is missing an `fputs()` implementation. This commit makes them use the `printf("%s", str);` instead of `fputs(str, stdout);`, which is semantically equivalent (but has more overhead).

tests/driver_ltc4150: BOARD_INSUFFICIENT_MEMORY

075ad47

maribu force-pushed the ltc4150-new branch from 76c6599 to 075ad47 Compare January 28, 2019 12:56

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

miri64 merged commit caa1d0b into RIOT-OS:master Jan 28, 2019

maribu deleted the ltc4150-new branch January 28, 2019 15:01

danpetry added this to the Release 2019.04 milestone Mar 12, 2019

Conversation

maribu commented Jan 11, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu commented Jan 11, 2019

Uh oh!

maribu commented Jan 11, 2019

Uh oh!

maribu commented Jan 11, 2019

Uh oh!

miri64 commented Jan 12, 2019

Uh oh!

Uh oh!

smlng Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

maribu Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

smlng Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

maribu Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smlng Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

maribu Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

smlng Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 16, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Jan 16, 2019

Uh oh!

maribu commented Jan 16, 2019

Uh oh!

miri64 commented Jan 16, 2019

Uh oh!

miri64 commented Jan 16, 2019

Uh oh!

maribu commented Jan 16, 2019

Uh oh!

Uh oh!

miri64 commented Jan 16, 2019

Uh oh!

maribu commented Jan 16, 2019

Uh oh!

maribu commented Jan 18, 2019

Uh oh!

MichelRottleuthner commented Jan 18, 2019

Uh oh!

miri64 commented Jan 18, 2019

Uh oh!

miri64 commented Jan 18, 2019

Uh oh!

maribu commented Jan 28, 2019

Uh oh!

smlng commented Jan 28, 2019

Uh oh!

maribu commented Jan 28, 2019

Uh oh!

miri64 commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Jan 28, 2019

Uh oh!

miri64 commented Jan 28, 2019

miri64 commented Jan 28, 2019 •

edited

Loading