Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks #12034

dagar · 2019-05-17T13:47:28Z

Describe problem solved by the proposed pull request
The firmware should be able to handle the case when more than one battery is being used.

Test data / coverage
A Pixhawk 4 was used, with two batteries at different charge levels. Each battery was plugged in and removed from the board, and the MAVLink messages were observed. The batteries were switched so that in some tests, batter 1 was lowest, and in others, battery 2 was lowest. In every case, the correct MAVLink messages were sent.

Describe your preferred solution
For the BATTERY_STATUS topic, one message is sent for each battery, each with a unique id. For the SYS_STATUS and HIGH_LATENCY2 topics, the battery with the lowest remaining percentage is used.

Describe possible alternatives
SYS_STATUS and HIGH_LATENCY2 could be populated always with Battery 0, or with the "main" battery. But using the lowest battery is the safest option, so that a user will not fail to see a low battery if it is the secondary.

dagar · 2019-05-17T15:27:31Z

TODO: update the size based on number of connected batteries.

ryanjAA · 2019-05-17T15:34:39Z

Tested and working as per your directions) on pixhawk 4.
listener battery_status
listener battery_status -i 1

Images of two battery bricks attached:

DonLakeFlyer · 2019-05-17T16:39:19Z

So from looking at code second battery will come across as BATTERY_STATUS.id==1 right? That should work with existing QGC code.

ryanjAA · 2019-05-17T18:08:30Z

@DonLakeFlyer interestingly though, it doesn't show up in QGC when looking at battery data, only from the console.

DonLakeFlyer · 2019-05-17T18:21:30Z

This can be debugged using QGC by slowing down the battery_status stream rate to the point you can see the last BATTERY_STATUS message in the mavlink inspector. For two batteries that should have BATTERY_STATUS.id = 1 and good values in it.

ryanjAA · 2019-05-17T22:40:01Z

Unless I am missing something, I am not able to alter the BATTERY_STATUS stream. When I try to alter it I get a mavlink on network on port 0 is not running (i tried starting various new mavlink streams as usual). I do see it in uorb top though.

AmeliaEScott · 2019-07-02T12:07:19Z

I rebased this to master and added a fix to make it actually publish the BATTERY_STATUS messages.

Some questions have come up: How should we populate the SYS_STATUS fields voltage_battery, current_battery, and battery_remaining when there are multiple batteries? Right now, they are always populated by battery 0. Should this stay this way? Should we somehow combine or average both batteries?

These same questions hold for the HIGH_LATENCY2 message.

AmeliaEScott · 2019-07-03T14:13:58Z

Right now, SYS_STATUS and HIGH_LATENCY2 are being populated with the data from the connected battery with the lowest percentage remaining. This is for safety: If any legacy system is only looking at the status of one battery (i.e., using SYS_STATUS instead of BATTERY_STATUS), it should see the lowest battery. If one battery is full and the other almost empty, a system should not see the full battery and assume everything is fine, unless it is explicitly handling multiple batteries.

julianoes

Looks good overall, couple of small comments.

src/modules/mavlink/mavlink_high_latency2.cpp

src/modules/mavlink/mavlink_messages.cpp

feemi · 2019-07-09T17:30:26Z

Hi @dagar is this development only for pixhawk 4? We are trying to log 2 battery on cube autopilot. Current config is just for 1 brick. We modified fmuv3 config and brick valid pins and voltage and current adc pins. Currently it looks like it logs properly on log. And it also started to show on qgc. But it shows only one battery on at a time on qgc. if both is connected then it show brick 1 value. And also parameters should be added for battery 2. How should we progress on this issue?

AmeliaEScott · 2019-07-10T07:37:34Z

@feemi One of my colleagues (@stefandunca) is currently working on supporting multiple batteries in QGC. It should be merged there soon. But with the current version of QGC and this branch of Firmware, QGC should always show the information about the battery with the lowest percentage charge remaining. Is this what you are seeing?

Parameters for extra batteries are a good point, I'll look into that.

feemi · 2019-07-10T18:37:19Z

With current version of qgc and my px4 modification for fmuv3. QGC shows battery 1 bricks voltage values. If I disconnect battery 1 then it switches to secondary battery brick. Actually we are using multiple battery monitoring for VTOL power test measurement. Primary battery monitors for MC motors and secondary battery monitors FW motor.

AmeliaEScott · 2019-07-11T07:33:52Z

Do you have a log that I could see? Everything seems to work for me on a Pixhawk 4, so I am wondering if you have a configuration problem, or if I have done something wrong here.

stefandunca · 2019-07-11T09:36:38Z

Here is the corresponding QGC PR: mavlink/qgroundcontrol#7575

bkueng

Reviewed, and added some inline comments.

src/modules/mavlink/mavlink_high_latency2.h

src/modules/mavlink/mavlink_messages.cpp

bkueng

A couple more details, otherwise good to merge.

bkueng · 2019-07-17T11:55:24Z

src/modules/mavlink/mavlink_high_latency2.h

+struct PerBatteryData {
+	PerBatteryData() : analyzer(SimpleAnalyzer::AVERAGE) {}
+	MavlinkOrbSubscription *subscription;
+	SimpleAnalyzer analyzer;


You can do:

Suggested change

SimpleAnalyzer analyzer;

SimpleAnalyzer analyzer{SimpleAnalyzer::AVERAGE};

bkueng · 2019-07-17T11:56:00Z

src/modules/mavlink/mavlink_high_latency2.h

@@ -130,7 +135,7 @@ class MavlinkStreamHighLatency2 : public MavlinkStream

 	SimpleAnalyzer _airspeed;
 	SimpleAnalyzer _airspeed_sp;
-	SimpleAnalyzer _battery;
+	// Use the SimplerAnalyzer because C++ needs this data type to have a default constructor


Left-over comment

bkueng · 2019-07-17T11:56:39Z

src/modules/mavlink/mavlink_high_latency2.h

@@ -44,6 +44,14 @@
 #include "mavlink_simple_analyzer.h"
 #include "mavlink_stream.h"

+struct PerBatteryData {


I'd move the struct to the private part of MavlinkStreamHighLatency2

DonLakeFlyer · 2019-07-17T16:45:58Z

With regards to BATTERY_STATUS and SYS_STATUS mavlink spec:

Could the spec be changed to be specific about what BATTERY_STATUS.id should be? For example first battery should be 0. Second battery 1 and so forth?
Also some more specific about how SYS_STATUS should report when it has multiple batteries?

feemi · 2019-07-18T06:53:17Z

Do you have a log that I could see? Everything seems to work for me on a Pixhawk 4, so I am wondering if you have a configuration problem, or if I have done something wrong here.

Yes I have @ItsTimmy, Sorry for delayed answer btw

Platform fmu-v3 pixhawk cube 2.1
px4 version 1.9.2-custom
(Changed fmu-v3 pin configurations to correct ones)

You can see battery voltage and current values on both and how measured on firmware

On the log you will also see major changes on voltage values. These are the plug disconnections to test how measurement changes.

TimeStamp --------- Battery 1 --------- Battery 2
0 Connected Connected
47000000~ Disconnected Connected
56000000~ Disconnected Disconnected
64000000~ Connected Disconnected
76000000~ Disconnected Disconnected
79000000~ Connected Connected

battery-log-dual.zip

…icks

AmeliaEScott · 2019-07-18T11:27:24Z

@feemi I think the problem is that you are using batteries with different cell counts (is that correct?). The current implementation requires that all batteries have the same cell counts. So at the times where both batteries are connected, Battery 1 is getting an appropriate measurement of ~50% full, while Battery 2 is reading as 100% full because it is over the maximum voltage of a 4S battery.

I'm not sure if this issue should be addressed in a separate PR, or should be addressed in this PR before merging. @bkueng ?

bkueng

I'm not sure if this issue should be addressed in a separate PR, or should be addressed in this PR before merging. @bkueng ?

Let's do a separate PR.

hamishwillee · 2019-07-19T02:53:49Z

FYI, clarifying in MAVLink to be done here: mavlink/mavlink#1182

feemi · 2019-07-19T07:44:15Z

@ItsTimmy Actually if there is no disconnection. 2 batteries are logging fine. I used different cells to show these measuring errors if you disconnected one batteries. If you want to address each log for each bricks it looks wrong data because currently firmware prioritize power brick according battery number. So even you use battery from brick 2. It starts to log it to battery_0 on log file. I think its kind of misleading. When you disconnect battery 1. It stops to log battery 2 to own log and it consider it is the only battery (which is true) and starts to log it on battery_0 section.

AmeliaEScott · 2019-07-19T08:03:57Z

Oh, I see the problem now. I will be working soon on an overhaul of the battery calibration system, and I think I should be able to address this issue there. I will tag you when I have a PR ready for those changes. I'm sorry for the confusion!

dagar added the Admin: Enhancement (improvement) 💡 label May 17, 2019

dagar requested a review from DonLakeFlyer May 17, 2019 13:47

dagar force-pushed the pr-mavlink_battery_status_all branch from 6b5c4c9 to beff6bf Compare May 17, 2019 13:50

weekly-digest bot mentioned this pull request May 19, 2019

Weekly Digest (12 May, 2019 - 19 May, 2019) #12043

Closed

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from beff6bf to d34041d Compare July 2, 2019 12:03

AmeliaEScott marked this pull request as ready for review July 3, 2019 14:14

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from dfd9fe6 to 00c2a74 Compare July 3, 2019 14:14

AmeliaEScott changed the title ~~[WIP] Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks~~ Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks Jul 3, 2019

weekly-digest bot mentioned this pull request Jul 7, 2019

Weekly Digest (30 June, 2019 - 7 July, 2019) #12430

Closed

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from 00c2a74 to 5f26bc1 Compare July 9, 2019 07:55

AmeliaEScott requested a review from julianoes July 9, 2019 09:56

julianoes reviewed Jul 9, 2019

View reviewed changes

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from ef9d1c9 to 52e34ef Compare July 10, 2019 07:16

bkueng added this to the Release v1.10.0 milestone Jul 10, 2019

stefandunca mentioned this pull request Jul 11, 2019

Dual battery display mavlink/qgroundcontrol#7575

Closed

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from 52e34ef to a98d918 Compare July 11, 2019 14:35

weekly-digest bot mentioned this pull request Jul 14, 2019

Weekly Digest (7 July, 2019 - 14 July, 2019) #12474

Closed

AmeliaEScott requested a review from bkueng July 16, 2019 09:58

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from a98d918 to 331b78b Compare July 16, 2019 10:01

bkueng reviewed Jul 16, 2019

View reviewed changes

src/modules/mavlink/mavlink_high_latency2.h Outdated Show resolved Hide resolved

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

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

bkueng reviewed Jul 17, 2019

View reviewed changes

dagar and others added 7 commits July 18, 2019 12:14

Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all br…

aa26727

…icks

Added battery status stream

71616e1

Fixed BATTERY_STATUS stream rate

786a008

Publish lowest battery data to SYS_STATUS

bb97247

Choose lowest battery for HIGH_LATENCY2

2b229d9

Code cleanup

c8ef3cd

Fixed array bounds

95153d3

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from 687c4ef to 053479a Compare July 18, 2019 10:14

Code cleanup

452deb3

AmeliaEScott force-pushed the pr-mavlink_battery_status_all branch from 053479a to 452deb3 Compare July 18, 2019 11:10

bkueng approved these changes Jul 18, 2019

View reviewed changes

bkueng merged commit 9986a88 into PX4:master Jul 18, 2019

hamishwillee mentioned this pull request Jul 19, 2019

Clarify use of multiple batteries. mavlink/mavlink#1182

Closed

weekly-digest bot mentioned this pull request Jul 21, 2019

Weekly Digest (14 July, 2019 - 21 July, 2019) #12528

Closed

AmeliaEScott mentioned this pull request Jul 24, 2019

Add calibration parameters for multiple batteries #12551

Merged

AmeliaEScott mentioned this pull request Aug 19, 2019

Multiple battery module support #12033

Closed

ThomasRigi mentioned this pull request Apr 15, 2020

v1.11.0 Release Notes #14402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks #12034

Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks #12034

dagar commented May 17, 2019 •

edited by AmeliaEScott

Loading

dagar commented May 17, 2019

ryanjAA commented May 17, 2019

DonLakeFlyer commented May 17, 2019

ryanjAA commented May 17, 2019

DonLakeFlyer commented May 17, 2019

ryanjAA commented May 17, 2019

AmeliaEScott commented Jul 2, 2019

AmeliaEScott commented Jul 3, 2019

julianoes left a comment

feemi commented Jul 9, 2019

AmeliaEScott commented Jul 10, 2019 •

edited

Loading

feemi commented Jul 10, 2019

AmeliaEScott commented Jul 11, 2019

stefandunca commented Jul 11, 2019

bkueng left a comment

bkueng left a comment

bkueng Jul 17, 2019

bkueng Jul 17, 2019

bkueng Jul 17, 2019

DonLakeFlyer commented Jul 17, 2019

feemi commented Jul 18, 2019 •

edited

Loading

AmeliaEScott commented Jul 18, 2019

bkueng left a comment

hamishwillee commented Jul 19, 2019

feemi commented Jul 19, 2019

AmeliaEScott commented Jul 19, 2019

	SimpleAnalyzer analyzer;
	SimpleAnalyzer analyzer{SimpleAnalyzer::AVERAGE};

Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks #12034

Mavlink split BATTERY_STATUS from SYS_STATUS update and handle all bricks #12034

Conversation

dagar commented May 17, 2019 • edited by AmeliaEScott Loading

dagar commented May 17, 2019

ryanjAA commented May 17, 2019

DonLakeFlyer commented May 17, 2019

ryanjAA commented May 17, 2019

DonLakeFlyer commented May 17, 2019

ryanjAA commented May 17, 2019

AmeliaEScott commented Jul 2, 2019

AmeliaEScott commented Jul 3, 2019

julianoes left a comment

Choose a reason for hiding this comment

feemi commented Jul 9, 2019

AmeliaEScott commented Jul 10, 2019 • edited Loading

feemi commented Jul 10, 2019

AmeliaEScott commented Jul 11, 2019

stefandunca commented Jul 11, 2019

bkueng left a comment

Choose a reason for hiding this comment

bkueng left a comment

Choose a reason for hiding this comment

bkueng Jul 17, 2019

Choose a reason for hiding this comment

bkueng Jul 17, 2019

Choose a reason for hiding this comment

bkueng Jul 17, 2019

Choose a reason for hiding this comment

DonLakeFlyer commented Jul 17, 2019

feemi commented Jul 18, 2019 • edited Loading

AmeliaEScott commented Jul 18, 2019

bkueng left a comment

Choose a reason for hiding this comment

hamishwillee commented Jul 19, 2019

feemi commented Jul 19, 2019

AmeliaEScott commented Jul 19, 2019

dagar commented May 17, 2019 •

edited by AmeliaEScott

Loading

AmeliaEScott commented Jul 10, 2019 •

edited

Loading

feemi commented Jul 18, 2019 •

edited

Loading