Add calibration parameters for multiple batteries #12551

AmeliaEScott · 2019-07-24T14:46:59Z

Describe problem solved by the proposed pull request
Support for multiple batteries was improved in #12034 , but there is still the issue that different batteries all use the same calibration parameters.

Test data / coverage
On Pixhawk 4, multiple batteries with different levels of charge and numbers of cells were tested.

This PR makes changes that affect all different boards, so more testing should be done on different hardware.

Describe your preferred solution
Improved the Battery library by adding the ability to instantiate different batteries with different sets of parameters. Split power reading in sensors module into a separate class for readability.

Additional context
In the near future, support for digital power meters will be expanded, continuing the work done in #11755. This refactoring should make supporting this new source of battery data, as well as any future new sources, easier.

AmeliaEScott · 2019-07-24T14:50:04Z

@feemi Follow-up from our discussion in #12034: This PR will hopefully address the issues you were having before. Would you mind testing it with your setup?

hamishwillee · 2019-07-25T03:23:12Z

@ItsTimmy This will have a docs impact and possibly quite a complex QGC UI change. Please ping me when you think it is time to engage re docs. The main doc to change would be : http://docs.px4.io/master/en/config/battery.html

And presumably we may need to change the failsafe behaviour too: http://docs.px4.io/master/en/config/safety.html#low-battery-failsafe

dagar · 2019-07-29T17:26:42Z

This is looking good, but I think we should briefly take a step back and try to sketch out a rough overall design that factors in all the current requirements.

different battery types (analog bricks, digital bricks, smart batteries)
- battery_status message
multiple instances
- parameter instances?
overall system estimate (reporting, failsafe handling, etc)
system power architecture
- if you have multiple batteries what's the overall percentage?
refactoring to cleanup the mess scattered across each platform adc and the sensors module
- I've already touched some of this as I'm moving the ADC drivers to the new WQ (remaining work queue cleanup #12068 [WIP]: ADC move to WQ dagar/PX4-Firmware#12)

@ItsTimmy @davids5 let's schedule a call.

dagar · 2019-07-29T17:37:51Z

Related #12033

AmeliaEScott · 2019-08-09T12:42:24Z

Even though we don't have the multi-battery requirements worked out completely, I think this PR is still ready for review and merge. It does not affect the safety-critical decisions like low-battery failsafe behavior and reporting of overall system battery percentage. These are still open issues, but they can be addressed in a separate PR.

The only outstanding issue is that this PR is failing CI because it is overflowing the flash on px4_fmu-v5x_stackcheck. I welcome suggestions to fix this.

AmeliaEScott · 2019-11-28T16:41:04Z

I have updated this with 3 important differences:

Parameters are now instanced with YAML files
Refactored to decrease code size
Parameter migration has been improved: Old and new parameters are now kept in sync in both directions. If either parameter is changed, the other is immediately updated to reflect this. If the deprecated parameter was changed, then a warning is displayed.

I still have some cleanup and testing to do on this. There are still some relics of the old way this was written.

@hamishwillee I don't know if this is the final behavior of this parameter migration (@dagar will have to weigh in), but once we have it nailed down, where should it be documented? Is it sufficient to only document it in the parameter descriptions?

julianoes · 2019-11-28T16:50:54Z

src/lib/battery/battery.cpp

 	_warning(battery_status_s::BATTERY_WARNING_NONE),
 	_last_timestamp(0)
 {
+	if (index > 9 || index < 1) {
+		PX4_ERR("Battery index must be between 1 and 9 (inclusive). Received %d. Defaulting to 1.", index);


I guess the alternative would just be an assert.

src/lib/battery/battery_params_deprecated.c

src/lib/battery/CMakeLists.txt

src/lib/battery/battery.h

src/modules/simulator/simulator.h

src/modules/battery_status/analog_battery.cpp

AmeliaEScott · 2019-12-05T14:08:47Z

@dagar @julianoes @bkueng This is ready for review.

julianoes

Reviewed again, I would say this is good to go in.

AmeliaEScott · 2019-12-05T15:21:09Z

I did some more testing on this latest version. The following tests all worked as intended:

Plugged in 2 batteries (with different cell counts) to Pixhawk 4 with analog power module
Calibrated battery 1 with QGC
Calibrated battery 2 manually (using param set)
Set values of each BAT_* parameter and BAT1_* parameter (using param set) to check that migration worked correctly
Tested with only 1 battery, plugged into each power port on the Pixhawk
- When plugged into POWER1, it is calibrated with BAT1_* parameters
- In POWER2, it is calibrated with BAT2_* parameters

bkueng

Nice work.

How do you want to handle updating QGC to the new params?

src/lib/battery/battery.h

src/modules/battery_status/analog_battery.cpp

src/modules/simulator/simulator.h

bkueng · 2019-12-05T15:42:28Z

src/lib/battery/battery.h

 class Battery : public ModuleParams
 {
 public:
-	Battery();
+	Battery(int index = 1, ModuleParams *parent = nullptr);


I would not default to NULL for the parent, to ensure it's set.

AmeliaEScott force-pushed the pr-digital-battery branch from 5bc65f7 to 9803216 Compare July 24, 2019 15:21

AmeliaEScott force-pushed the pr-digital-battery branch 12 times, most recently from 66b4f3d to ed137a4 Compare July 26, 2019 13:48

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

Weekly Digest (21 July, 2019 - 28 July, 2019) #12569

Closed

dagar requested review from davids5 and dagar July 29, 2019 17:22

AmeliaEScott force-pushed the pr-digital-battery branch 3 times, most recently from db2b8ff to 48034ca Compare August 2, 2019 09:48

AmeliaEScott force-pushed the pr-digital-battery branch 2 times, most recently from 89f127b to 06d774f Compare August 9, 2019 08:12

AmeliaEScott marked this pull request as ready for review August 9, 2019 12:40

AmeliaEScott force-pushed the pr-digital-battery branch from 06d774f to 6346943 Compare August 9, 2019 13:15

AmeliaEScott mentioned this pull request Aug 9, 2019

Integrate INA226 with PX4 #12673

Merged

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #12682

Closed

AmeliaEScott force-pushed the pr-digital-battery branch from d00c2d2 to 612ae0c Compare November 28, 2019 16:49

julianoes reviewed Nov 28, 2019

View reviewed changes

bkueng reviewed Nov 29, 2019

View reviewed changes

src/lib/battery/CMakeLists.txt Show resolved Hide resolved

src/lib/battery/battery.h Outdated Show resolved Hide resolved

src/lib/battery/battery.h Outdated Show resolved Hide resolved

AmeliaEScott mentioned this pull request Nov 29, 2019

Added option for module config file in libraries #13633

Closed

AmeliaEScott force-pushed the pr-digital-battery branch 5 times, most recently from c1ebd6f to 638f4c9 Compare November 29, 2019 13:37

dagar reviewed Dec 4, 2019

View reviewed changes

src/modules/simulator/simulator.h Outdated Show resolved Hide resolved

dagar reviewed Dec 4, 2019

View reviewed changes

src/modules/simulator/simulator.h Outdated Show resolved Hide resolved

dagar reviewed Dec 4, 2019

View reviewed changes

src/modules/battery_status/analog_battery.cpp Outdated Show resolved Hide resolved

Timothy Scott added 2 commits December 5, 2019 13:17

Refactored battery library for multiple instances

b82a80e

Created new AnalogBattery class

2335917

AmeliaEScott force-pushed the pr-digital-battery branch 3 times, most recently from e78b8c7 to bfdc9fd Compare December 5, 2019 13:19

Refactored to work with new battery_status module

a3febb9

AmeliaEScott force-pushed the pr-digital-battery branch from bfdc9fd to a3febb9 Compare December 5, 2019 13:28

AmeliaEScott requested review from dagar, bkueng and julianoes December 5, 2019 14:08

julianoes approved these changes Dec 5, 2019

View reviewed changes

julianoes merged commit 993fa5b into PX4:master Dec 5, 2019

bkueng reviewed Dec 5, 2019

View reviewed changes

AmeliaEScott mentioned this pull request Dec 6, 2019

Code cleanup from #12551 #13694

Merged

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

Add calibration parameters for multiple batteries #12551

Add calibration parameters for multiple batteries #12551

AmeliaEScott commented Jul 24, 2019

AmeliaEScott commented Jul 24, 2019

hamishwillee commented Jul 25, 2019

dagar commented Jul 29, 2019 •

edited

Loading

dagar commented Jul 29, 2019

AmeliaEScott commented Aug 9, 2019 •

edited

Loading

AmeliaEScott commented Nov 28, 2019 •

edited

Loading

julianoes Nov 28, 2019

AmeliaEScott commented Dec 5, 2019

julianoes left a comment •

edited

Loading

AmeliaEScott commented Dec 5, 2019 •

edited

Loading

bkueng left a comment

bkueng Dec 5, 2019

Add calibration parameters for multiple batteries #12551

Add calibration parameters for multiple batteries #12551

Conversation

AmeliaEScott commented Jul 24, 2019

AmeliaEScott commented Jul 24, 2019

hamishwillee commented Jul 25, 2019

dagar commented Jul 29, 2019 • edited Loading

dagar commented Jul 29, 2019

AmeliaEScott commented Aug 9, 2019 • edited Loading

AmeliaEScott commented Nov 28, 2019 • edited Loading

julianoes Nov 28, 2019

Choose a reason for hiding this comment

AmeliaEScott commented Dec 5, 2019

julianoes left a comment • edited Loading

Choose a reason for hiding this comment

AmeliaEScott commented Dec 5, 2019 • edited Loading

bkueng left a comment

Choose a reason for hiding this comment

bkueng Dec 5, 2019

Choose a reason for hiding this comment

dagar commented Jul 29, 2019 •

edited

Loading

AmeliaEScott commented Aug 9, 2019 •

edited

Loading

AmeliaEScott commented Nov 28, 2019 •

edited

Loading

julianoes left a comment •

edited

Loading

AmeliaEScott commented Dec 5, 2019 •

edited

Loading