drivers: new driver for TI BQ27441 Lithium Battery Fuel Gauge #10882

anatoliy6 · 2019-01-27T13:53:06Z

Contribution description

BQ27441 fuel gauge for single-cell Li-Ion batteries.
This work is port of https://github.com/sparkfun/Battery_Babysitter code

Testing procedure

We are using custom hardware with bq27441-G1 that is connected to a STM32F446RE, but the setup can be replicated with any MCU and SparkFun's Battery Babysitter, or any other board that has bq27441-G1.

Issues/PRs references

No issues, no references. New driver v1.

anatoliy6 · 2019-02-09T12:38:14Z

Hey @PeterKietzmann @MichelRottleuthner,

It's been a while since i've submitter this PR and i see a lot of request have been piling up, so i guess you're quite busy. Is there anything i can do or clarify to help with the review of this PR? If not i won't be bothering you soon.

Regards, Anatoliy

PeterKietzmann · 2019-02-11T08:23:10Z

@anatoliy6 please have a look at our coding conventions and adapt your PR accordingly. Furthermore you might want to run uncrustify which is explained in more detail in there.

PeterKietzmann · 2019-05-21T14:53:55Z

@anatoliy6 did you have a chance to check our conventions?

anatoliy6 · 2019-05-21T19:52:19Z

Hey Peter, Yes I did. My C programming skills are a bit rusty and i had a little progress beyond the uncrustify script. My colleague is pushing me to finish the work this week, so I'm back on it. Apologize for the delay, I'm looking forward to get the patch into a better shape shortly. Regards, Anatoliy

…

On Tue, 21 May 2019 at 17:54, Peter Kietzmann ***@***.***> wrote: @anatoliy6 <https://github.com/anatoliy6> did you have a chance to check our conventions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10882?email_source=notifications&email_token=ALGCNKAOMTR73ZUG5GGEYELPWQEJFA5CNFSM4GSS5LV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4FR3Q#issuecomment-494426350>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALGCNKFOQZU35UGO3LPP6D3PWQEJFANCNFSM4GSS5LVQ> .

PeterKietzmann · 2019-05-22T07:48:17Z

@anatoliy6 no worries! Please ping @MichelRottleuthner and me once done

anatoliy6 · 2019-07-28T21:06:09Z

Dear @PeterKietzmann @MichelRottleuthner,

I've managed to run the uncrustify script and put the last patch. I read through the coding guidelines in an effort to be compliant, but i'm not that good with the language to really determine additional changes. Please let me know if this needs more work.

Regards, Anatoliy

MichelRottleuthner

Thanks for contributing with this PR @anatoliy6 !
I did a first review round. There are still style and doc issues and some things related to the code/functionality.
General remarks: the interface is pretty huge. For the feature completeness that is great, but adding separate calls for reading single flags seems a bit convoluted. Returning a status register value and using that value in combination with bitmasks could help here. Multiple functions could be replaced by single parameterized implementations. See below for details.
I have the hardware at hand so I can help with testing once the code is ready :)

MichelRottleuthner · 2019-07-29T08:21:12Z

drivers/bq27441/bq27441.c

+#include "debug.h"
+
+#ifndef I2C_ACK
+#define I2C_ACK         (0)


nitpicky, but please align defined values whenever possible

MichelRottleuthner · 2019-07-29T08:37:38Z

drivers/bq27441/bq27441.c

+#endif
+
+#include "periph/gpio.h"
+#include "xtimer.h"


please move all include lines together (these two up to the others) and please don't mix the different include types. I.e. group them into system/toolchain headers (#include <foo.h>) and other includes (include "bar.h")

MichelRottleuthner · 2019-07-29T08:39:53Z

drivers/bq27441/bq27441.c

+#include "xtimer.h"
+
+#define BQ72441_I2C_TIMEOUT 2000
+#define BQ72441_I2C_BUFSIZE (128U)


Please be consistent with the style here and add parentheses to the upper one - the timeout is probably also an unsigned?

MichelRottleuthner · 2019-07-29T08:53:53Z

drivers/bq27441/bq27441.c

+}
+
+
+// Initializes I2C and verifies communication with the BQ27441.


please remove all of these comments from the c file. The documentation should be only be in the header.

MichelRottleuthner · 2019-07-29T08:57:40Z

drivers/bq27441/bq27441.c

+// Configures the design capacity of the connected battery.
+bool bq27441_set_capacity(bq27441_t *dev, uint16_t capacity)
+{
+    // Write to STATE subclass (82) of BQ27441 extended memory.


For comments describing details inline please always use the multi-line format: /* this is a comment */

MichelRottleuthner · 2019-07-29T13:49:00Z

tests/driver_bq27441/main.c

+    const unsigned int BATTERY_CAPACITY = 850;
+    gpio_t out_pin = GPIO_PIN(3, 3); // TODO: DEFINE CORRECTLY
+
+    i2c_acquire(params_default[0].bus);


our convention is to handle acquire and release inside of the driver functions

MichelRottleuthner · 2019-07-29T13:52:39Z

tests/driver_bq27441/main.c

+        printf("Testing GPOUT Pulse\n");
+
+        bq27441_pulse_gpout(&dev);
+        int timeout = 10000;    // The pulse can take a while to occur. Set max to 10s


avoid magic numbers

MichelRottleuthner · 2019-07-29T13:52:55Z

tests/driver_bq27441/main.c

+        if (timeout > 0) {
+            // If GPOUT pulsed, print success message.
+            printf("GPOUT test successful!");
+            printf("( %d )", (10000 - timeout));


MichelRottleuthner · 2019-07-29T14:00:59Z

drivers/bq27441/bq27441.c

+// Exit configuration mode with the option to perform a resimulation
+bool bq27441_exit_config_mode(const bq27441_t *dev, bool resim)
+{
+    // There are two methods for exiting config mode:


this is helpful documentation but should be moved to the header file so a developer using the api actually sees it.

MichelRottleuthner · 2019-07-29T14:06:35Z

drivers/bq27441/bq27441.c

+// Get SOC1_Set Threshold - threshold to set the alert flag
+uint8_t bq27441_get_soc1_set_threshold(bq27441_t *dev)
+{
+    return _bq27441_read_extended_data(dev, BQ27441_ID_DISCHARGE, 0);


use defines for the magic numbers here and below

MichelRottleuthner · 2019-09-17T10:11:07Z

@anatoliy6 do you still want to work on this?

anatoliy6 · 2019-10-04T12:14:47Z

@anatoliy6 do you still want to work on this?

Yes. I am. Almost ready with refactoring.

daexel · 2019-11-04T10:33:16Z

I have tested the code and it looks like the command sequence to modify a Data Memory parameters is not working. In the code its this function: bool bq27441_enter_config_mode(bq27441_t *dev, bool userControl)

PeterKietzmann · 2020-02-25T17:36:43Z

@anatoliy6 any progress?

jeandudey · 2020-02-25T21:06:40Z

If wanted I could continue/test this driver if needed.

miri64 · 2020-06-24T19:16:06Z

If wanted I could continue/test this driver if needed.

@jeandudey did you?

jeandudey · 2020-06-24T19:18:41Z

If wanted I could continue/test this driver if needed.

@jeandudey did you?

I have a stripped down version of this driver, no config (yet) and with the correct timing requirements for the device, would be glad to open a PR once configuration part is done

stale · 2020-12-26T20:04:40Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

aabadie added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties labels Jan 29, 2019

aabadie requested a review from PeterKietzmann January 29, 2019 16:06

PeterKietzmann requested a review from MichelRottleuthner January 29, 2019 17:00

anatoliy6 added 3 commits July 28, 2019 11:41

drivers: new driver for TI BQ27441 Lithium Battery Fuel Gauge

a66ba1e

drivers/bq27441: fix identation

21c0cad

drivers/bq27441: uncrustify script

6e7612d

anatoliy6 force-pushed the master branch from 945a4a2 to 6e7612d Compare July 28, 2019 11:46

MichelRottleuthner requested changes Jul 29, 2019

View reviewed changes

PeterKietzmann removed the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 25, 2020

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Dec 26, 2020

stale bot closed this Jan 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: new driver for TI BQ27441 Lithium Battery Fuel Gauge #10882

drivers: new driver for TI BQ27441 Lithium Battery Fuel Gauge #10882

anatoliy6 commented Jan 27, 2019

anatoliy6 commented Feb 9, 2019

PeterKietzmann commented Feb 11, 2019

PeterKietzmann commented May 21, 2019

anatoliy6 commented May 21, 2019 via email

PeterKietzmann commented May 22, 2019

anatoliy6 commented Jul 28, 2019

MichelRottleuthner left a comment

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner Jul 29, 2019

MichelRottleuthner commented Sep 17, 2019

anatoliy6 commented Oct 4, 2019

daexel commented Nov 4, 2019

PeterKietzmann commented Feb 25, 2020

jeandudey commented Feb 25, 2020

miri64 commented Jun 24, 2020

jeandudey commented Jun 24, 2020

stale bot commented Dec 26, 2020

		}


		// Initializes I2C and verifies communication with the BQ27441.

drivers: new driver for TI BQ27441 Lithium Battery Fuel Gauge #10882

drivers: new driver for TI BQ27441 Lithium Battery Fuel Gauge #10882

Conversation

anatoliy6 commented Jan 27, 2019

Contribution description

Testing procedure

Issues/PRs references

anatoliy6 commented Feb 9, 2019

PeterKietzmann commented Feb 11, 2019

PeterKietzmann commented May 21, 2019

anatoliy6 commented May 21, 2019 via email

PeterKietzmann commented May 22, 2019

anatoliy6 commented Jul 28, 2019

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichelRottleuthner commented Sep 17, 2019

anatoliy6 commented Oct 4, 2019

daexel commented Nov 4, 2019

PeterKietzmann commented Feb 25, 2020

jeandudey commented Feb 25, 2020

miri64 commented Jun 24, 2020

jeandudey commented Jun 24, 2020

stale bot commented Dec 26, 2020