Battery Smbus enhancements #15369

BazookaJoe1900 · 2020-07-18T08:25:24Z

Describe problem solved by this pull request
The sbmus driver has many fixes and functional updates that can be done.

fixes - many code clean ups and optimizations
fixes/upgrade - how to behave when there is smbus interface errors
upgrade - support BQ40Z80 device
upgrade - publish mavlink SMART_BATTERY_INFO

Describe your solution
This PR is WIP to start and update the driver. it just cleanups before the "real work".
it can be merged, so the real work will be done on next step

Test data / coverage
I am working with BQ40Z80 only, and not BQ40Z50. (the code for the BQ40Z80 has not been pushed on first commits)
so if there someone that can test the code with BQ40Z50, to ensure nothing has break, that will be helpful

Additional context
Pr batt smbus stability #14409
SMBus battery (a.k.a. smart battery) enhancement. #14496

src/lib/drivers/smbus/SMBus.cpp

dagar · 2020-07-19T18:51:23Z

Looks good so far. I don't really have anything on hand to test this code. Any suggestions?

Bigger picture architecturally we really need to do something to split the messages for simple analog power modules and smart batteries. The existing battery_status is looking rather overloaded.

BazookaJoe1900 · 2020-07-19T19:19:05Z

Looks good so far.

Do you want to merge those changes, and further work to be done on other pr?

I don't really have anything on hand to test this code. Any suggestions?

I have tested it with mine BQ40Z80 based battery. @limhyon @dakejahl do you have setup to test with BQ40Z50?

Bigger picture architecturally we really need to do something to split the messages for simple analog power modules and smart batteries. The existing battery_status is looking rather overloaded.

@dagar I have the next proposal (That I have already implemented on my repo...).
all the data you can take from analog is already on SYS_STATUS message:

voltage_v
current_a
discharged_mah

leave it there.

the only thing that isn't there is the warning, but look at that, you have the "errors_count1" on SYS_STATUS which doesn't being used at all..... can we put the warring there?

only publish smart battery data on "BATTERY_STATUS", I have actually made new "SMART_BATTERY_STATUS_EXTENDED" that combine "BATTERY_STATUS" and "SMART_BATTERY_STATUS", if that something worth doing on master, tell me and I will make new pr for review.

dakejahl

Looks good to me, please see suggested changes.

edit: I do not have hardware to test this against btw

src/drivers/batt_smbus/batt_smbus.cpp

src/modules/mavlink/mavlink_messages.cpp

src/drivers/batt_smbus/batt_smbus.h

src/drivers/batt_smbus/batt_smbus.cpp

TSC21 · 2020-07-19T21:18:00Z

@themcfadden you might want to review this as well.

BazookaJoe1900 · 2020-07-19T21:23:01Z

Thanks all for the comments!
I have general question, how do you see the "resolve conversation" should be handled? does the one that open it should press it? or the one that "answer" to the conversation?

BazookaJoe1900 · 2020-07-19T21:30:47Z

@dagar I see now a build issue for linux builds, because of using crc8ccitt() function, which is a nuttx function.
any suggestion how to solve it?

TSC21 · 2020-07-19T21:31:43Z

@BazookaJoe1900 I would like to get some feedback from @themcfadden as he's using a BMS with the Texas BQ40Z80, which required also some mods on this driver.

TSC21 · 2020-07-19T21:32:59Z

@dagar I see now a build issue for linux builds, because of using crc8ccitt() function, which is a nuttx function.
any suggestion how to solve it?

Well this needs some support added, but a workaround is use some prepocessor directives.

BazookaJoe1900 · 2020-07-19T21:35:43Z

@BazookaJoe1900 I would like to get some feedback from @themcfadden as he's using a BMS with the Texas BQ40Z80, which required also some mods on this driver.

great, looking for @themcfadden notes about current pr. but pay attention that the BQ40Z80 related code has not yet merged, I didn't want to mess this pr with much more changes that are pending in my repo...

dakejahl · 2020-07-19T21:44:36Z

crc8ccitt() function, which is a nuttx function. any suggestion how to solve it?

Add back the CRC calculation code and wrap it in preprocessor directives for not __NUTTX

dagar · 2020-07-20T01:23:47Z

src/drivers/batt_smbus/batt_smbus.cpp

@@ -55,7 +56,7 @@ BATT_SMBUS::BATT_SMBUS(I2CSPIBusOption bus_option, const int bus, SMBus *interfa
 	_batt_topic = orb_advertise(ORB_ID(battery_status), &new_report);

 	int battsource = 1;
-	param_set(param_find("BAT_SOURCE"), &battsource);
+	param_set((param_t)(px4::params::BAT_SOURCE), &battsource);


Actually, maybe we should leave this alone until we can take a dedicated look at C++ param usage separately. The px4::params enum is really only meant to be used with ModuleParams, however I don't see why we shouldn't use it directly with along with a few other new helpers.

@dagar Done

dagar · 2020-07-20T01:30:47Z

src/drivers/batt_smbus/batt_smbus.cpp

 	}

 	if (lifetime_data_flush() == PX4_OK) {
 		// Flush needs time to complete, otherwise device is busy. 100ms not enough, 200ms works.
-		px4_usleep(200000);
+		px4_usleep(200_ms);


Be careful here if this is executed from the WQ thread (I2C bus). This is long enough to cause magnetometer or barometer timeouts if sharing the same bus.

that actually is simple refectory, so if there was problem before, it will be still a problem.
I am not sure how to handle that comment....

I'll take a closer look when I have some appropriate hardware on hand.

src/lib/drivers/smbus/SMBus.cpp

dagar · 2020-07-20T01:42:14Z

@dagar I have the next proposal (That I have already implemented on my repo...).
all the data you can take from analog is already on SYS_STATUS message:

I was actually only talking about battery_status within PX4, but yes it's a problem at the mavlink level as well.

the only thing that isn't there is the warning, but look at that, you have the "errors_count1" on SYS_STATUS which doesn't being used at all..... can we put the warring there?

I'm not sure of the exact details here, but in general to me it's a question of it being useful/actionable. Does an operator actually need that information for preflight or during operation?

themcfadden · 2020-07-21T13:37:28Z

Thanks @TSC21 for pulling me in. We are using the the BQ40Z80 in our battery design.
From my quick look this morning, I'm really liking all the good work here (@BazookaJoe1900).
I'd like to go over it in more detail, but I'm tied up again today and may not get to it until tomorrow. So if you can give me another day or so, that would be much appreciated.

themcfadden · 2020-07-22T16:13:40Z

I've had a chance to dig into this a bit more. As I said before I think the changes are good and don't want to hold anything up any longer.

The reason we designed a battery with the BQ40Z80 was to support 6S, which we need to meet a flight time requirement.
Given that the focus of the PR is to clean things up, I see that it does not add support for the additional cells (which was the main thing I was hoping to review).

What are the plans for this driver moving forward? Will it support the additional cells in the future?

We have it working with 6S, and I can help out with code submissions, reviews, and/or testing.

BazookaJoe1900 · 2020-07-22T20:03:10Z

@themcfadden

What are the plans for this driver moving forward? Will it support the additional cells in the future?

I am working with the BQ40Z80 too, and there is a bit more of just adding the extra cells due to compatibility differences.
I am already flying with updated code. so in the matter of functionality everything works.
there are other enhancement I made that should be discussed before I will feel confidence to merge, each can be discussed separately :

the current driver changes BAT_SOURCE parameter if it don't detect the smart battery. I don't think its right, if you should have smart battery and didn't detect it, its bad. you should warn the user.
the driver works at 10Hz, from what I understood the BQ data sample at 4Hz only.
the current driver seal/unseal the battery. I disabled that on my case. I don't think its smart to do it, unless you know what you are doing and you are in "maintenance mode".
this behavior can be changed to be controlled only if some parameter enable it
the current driver changes the under voltage protection during flight, I think that should be done by well defined configuration on the battery, and not by controlling the BQ during flight.
as above, this behavior can be changed to be controlled only if some parameter enable it
there are more efficiency staff to do, such reading blocks of data instead of single registers.
there is aslo issue of how to handle bad cases that there is interface errors

@dagar do you want me to add the Z80 code? or maybe to do some branch to the branch?

*EDIT - I have added basic code to use with Z80, for review and notes

dagar · 2020-07-23T13:57:30Z

If this has been and can pass CI I'm happy to merge other than 2 minor thing.

the param usage
- let's follow up and add a new proper c++ api for use with px4::params::BAT_SOURCE
- it's not okay (yet) to use the enum directly as it skips the param_find which marks the parameter active
the NuttX ifdef for crc8ccitt() usage
- I would drop this temporarily and we can do a quick pass to get the CRC lib in place, or even just pull in crc8ccitt() for posix. It's taken quite a long time to get to real cross platform drivers, I'd rather not let fragmentation creep in.

dagar · 2020-07-23T14:02:20Z

src/drivers/batt_smbus/parameters.c

+ * @value 1 BQ40Z50
+ * @value 2 BQ40Z80
+ */
+PARAM_DEFINE_INT32(BAT_MODEL, 0);


BAT_MODEL is a bit too generic. Maybe something like BAT_SMBUS_MODEL?

I'd also consider just skipping it entirely if the battery is easy to identify.

I will be happy to make it auto-detect, and I am sure it possible, but didn't had time to do it.
any way, I only have the Z80 to check it with, so I will need someone with Z50 to check it too

src/lib/drivers/smbus/SMBus.hpp

dagar · 2020-07-23T14:51:51Z

CI failing due to fmu-v2_default flash. Can you try merging current master into this branch or rebasing to see if it'll fit now?

EDIT: Looks like you already did before I hit "comment".

BazookaJoe1900 · 2020-07-23T16:18:50Z

@dagar anyway, don't merge before @themcfadden review, and functional tests please

themcfadden · 2020-08-14T18:34:38Z

listener battery_status is not showing individual cell voltages because the new_report.voltage_cell_v[] is not being populated. It is a 3 line change. Should I push it?

Otherwise it looks good.

themcfadden · 2020-08-14T18:41:01Z

I don't seem to have access rights to push. Here's the patch It is super simple.

commit 683f8e0c666028d5202116a2c5ba4006b146e359
Author: Matt McFadden <[email protected]>
Date:   Fri Aug 14 12:35:15 2020 -0600

    batt_smbus: Added missing cell voltages to battery_status_s.

diff --git a/src/drivers/batt_smbus/batt_smbus.cpp b/src/drivers/batt_smbus/batt_smbus.cpp
index 25ea6c74b1..f3c83a5a89 100644
--- a/src/drivers/batt_smbus/batt_smbus.cpp
+++ b/src/drivers/batt_smbus/batt_smbus.cpp
@@ -171,6 +171,9 @@ void BATT_SMBUS::RunImpl()
 		new_report.serial_number = _serial_number;
 		new_report.max_cell_voltage_delta = _max_cell_voltage_delta;
 		new_report.cell_count = _cell_count;
+    for (int i = 0; i < _cell_count; i++) {
+      new_report.voltage_cell_v[i] = _cell_voltages[i];
+    }

 		// Check if max lifetime voltage delta is greater than allowed.
 		if (_lifetime_max_delta_cell_voltage > BATT_CELL_VOLTAGE_THRESHOLD_FAILED) {

themcfadden · 2020-08-14T18:54:33Z

A couple more comments:

The driver was tested on the bench against our 4S BQ40Z50 Teal One battery, and
against our new 6S BQ40Z80 battery
With the latest stuff, QGC complains about some of the BAT params. I assume that is known.

- added support for BQ40Z80 based battery - added performance counter for interface errors - added SMART_BATTERY_INFO mavlink message - general code cleanups and optimization - fixed: void flooding the log in case of interface error - fixed: using _batt_startup_capacity instead of _batt_capacity for discharged_mah - update: read manufacture_date - update: get _cell_count from parameter and not const 4 - update: avoid re-reading data that has already been read and stored on class already - currently the battery type defined by BAT_SMBUS_MODEL parameter and not by auto detection

BazookaJoe1900 · 2020-08-14T20:55:53Z

@themcfadden - thanks for testing! I rebased, fixed more small staff and also fixed the cell issue you noted.
@dagar there is a 'ERROR [SPI_I2C] Bug: driver batt_smbus does not pass the I2C address to I2CSPIDriverBas' note on start up, but the driver works well. can you help me to solve? I am not sure what it is. (@themcfadden found that)

BazookaJoe1900 · 2020-08-18T16:56:42Z

@dagar this pr ready to merge. can you do that?

dagar · 2020-08-19T01:40:49Z

@dagar there is a 'ERROR [SPI_I2C] Bug: driver batt_smbus does not pass the I2C address to I2CSPIDriverBas' note on start up, but the driver works well. can you help me to solve? I am not sure what it is. (@themcfadden found that)

Let's take a look at that separately. It's harmless, but I would like to get rid of it.

BazookaJoe1900 force-pushed the pr-smbus branch from 2a7dce4 to 8b51ef2 Compare July 18, 2020 08:30

BazookaJoe1900 marked this pull request as draft July 18, 2020 08:36

TSC21 requested review from dakejahl and dagar July 18, 2020 20:04

weekly-digest bot mentioned this pull request Jul 19, 2020

Weekly Digest (12 July, 2020 - 19 July, 2020) #15376

Closed

dagar reviewed Jul 19, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.cpp Outdated Show resolved Hide resolved

dagar added Admin: Cleanup (housekeeping) 🧹 Admin: Enhancement (improvement) 💡 labels Jul 19, 2020

BazookaJoe1900 force-pushed the pr-smbus branch from 69e97d0 to 1337536 Compare July 19, 2020 19:46

BazookaJoe1900 changed the title ~~WIP: Battery Smbus enhancements~~ Battery Smbus enhancements Jul 19, 2020

BazookaJoe1900 marked this pull request as ready for review July 19, 2020 20:27

dakejahl requested changes Jul 19, 2020

View reviewed changes

src/drivers/batt_smbus/batt_smbus.cpp Outdated Show resolved Hide resolved

src/modules/mavlink/mavlink_messages.cpp Outdated Show resolved Hide resolved

src/drivers/batt_smbus/batt_smbus.h Outdated Show resolved Hide resolved

src/drivers/batt_smbus/batt_smbus.cpp Show resolved Hide resolved

BazookaJoe1900 force-pushed the pr-smbus branch 2 times, most recently from 367dca9 to e68b4ac Compare July 19, 2020 21:17

dakejahl self-requested a review July 19, 2020 21:23

dakejahl previously approved these changes Jul 19, 2020

View reviewed changes

BazookaJoe1900 mentioned this pull request Jul 19, 2020

New CRC lib #15378

Open

BazookaJoe1900 dismissed dakejahl’s stale review via d834f5d July 19, 2020 22:00

BazookaJoe1900 force-pushed the pr-smbus branch from e68b4ac to d834f5d Compare July 19, 2020 22:00

dagar reviewed Jul 20, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.cpp Outdated Show resolved Hide resolved

BazookaJoe1900 force-pushed the pr-smbus branch from b73b7b2 to f9ec4f3 Compare July 22, 2020 20:32

dagar reviewed Jul 23, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.hpp Outdated Show resolved Hide resolved

dagar reviewed Jul 23, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.hpp Outdated Show resolved Hide resolved

dagar requested a review from limhyon July 23, 2020 14:25

BazookaJoe1900 force-pushed the pr-smbus branch from 0013ec5 to ef86f98 Compare July 23, 2020 14:50

weekly-digest bot mentioned this pull request Jul 26, 2020

Weekly Digest (19 July, 2020 - 26 July, 2020) #15419

Closed

BazookaJoe1900 force-pushed the pr-smbus branch from b661af1 to 1ca689e Compare August 14, 2020 20:54

dagar approved these changes Aug 19, 2020

View reviewed changes

dagar merged commit 190b96a into master Aug 19, 2020

dagar deleted the pr-smbus branch August 19, 2020 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Battery Smbus enhancements #15369

Battery Smbus enhancements #15369

BazookaJoe1900 commented Jul 18, 2020

dagar commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020 •

edited

Loading

dakejahl left a comment •

edited

Loading

TSC21 commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020

TSC21 commented Jul 19, 2020

TSC21 commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020

dakejahl commented Jul 19, 2020 •

edited

Loading

dagar Jul 20, 2020

BazookaJoe1900 Jul 23, 2020

dagar Jul 20, 2020

BazookaJoe1900 Jul 20, 2020

dagar Jul 23, 2020

dagar commented Jul 20, 2020

themcfadden commented Jul 21, 2020

themcfadden commented Jul 22, 2020

BazookaJoe1900 commented Jul 22, 2020 •

edited

Loading

dagar commented Jul 23, 2020

dagar Jul 23, 2020

BazookaJoe1900 Jul 23, 2020

dagar commented Jul 23, 2020 •

edited

Loading

BazookaJoe1900 commented Jul 23, 2020

themcfadden commented Aug 14, 2020

themcfadden commented Aug 14, 2020 •

edited

Loading

themcfadden commented Aug 14, 2020

BazookaJoe1900 commented Aug 14, 2020 •

edited

Loading

BazookaJoe1900 commented Aug 18, 2020

dagar commented Aug 19, 2020

Battery Smbus enhancements #15369

Battery Smbus enhancements #15369

Conversation

BazookaJoe1900 commented Jul 18, 2020

dagar commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020 • edited Loading

dakejahl left a comment • edited Loading

Choose a reason for hiding this comment

TSC21 commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020

TSC21 commented Jul 19, 2020

TSC21 commented Jul 19, 2020

BazookaJoe1900 commented Jul 19, 2020

dakejahl commented Jul 19, 2020 • edited Loading

dagar Jul 20, 2020

Choose a reason for hiding this comment

BazookaJoe1900 Jul 23, 2020

Choose a reason for hiding this comment

dagar Jul 20, 2020

Choose a reason for hiding this comment

BazookaJoe1900 Jul 20, 2020

Choose a reason for hiding this comment

dagar Jul 23, 2020

Choose a reason for hiding this comment

dagar commented Jul 20, 2020

themcfadden commented Jul 21, 2020

themcfadden commented Jul 22, 2020

BazookaJoe1900 commented Jul 22, 2020 • edited Loading

dagar commented Jul 23, 2020

dagar Jul 23, 2020

Choose a reason for hiding this comment

BazookaJoe1900 Jul 23, 2020

Choose a reason for hiding this comment

dagar commented Jul 23, 2020 • edited Loading

BazookaJoe1900 commented Jul 23, 2020

themcfadden commented Aug 14, 2020

themcfadden commented Aug 14, 2020 • edited Loading

themcfadden commented Aug 14, 2020

BazookaJoe1900 commented Aug 14, 2020 • edited Loading

BazookaJoe1900 commented Aug 18, 2020

dagar commented Aug 19, 2020

BazookaJoe1900 commented Jul 19, 2020 •

edited

Loading

dakejahl left a comment •

edited

Loading

dakejahl commented Jul 19, 2020 •

edited

Loading

BazookaJoe1900 commented Jul 22, 2020 •

edited

Loading

dagar commented Jul 23, 2020 •

edited

Loading

themcfadden commented Aug 14, 2020 •

edited

Loading

BazookaJoe1900 commented Aug 14, 2020 •

edited

Loading