Fix Battery Parameter Migration #15616

MaEtUgR · 2020-08-25T18:02:43Z

Describe problem solved by this pull request
@cmic0 tried to change BAT1_V_EMPTY. Parameter was changed, it differed from BAT_V_EMPTY and after a reboot (starting simulation again) it was back to the old value.

Battery parameter migration from BAT_* to BAT1_* had the following issues:

In simulation the SimulatorBattery broke the chain of ModuleParams parent/child tree which the logic is relying on. Additionally it overwrote the parameters with hardcoded values internally.
The parameter migration prefers to migrate old over new if the values differ on boot but the flag _first_parameter_update was initialized false and hence always false. This didn't have an effect most of the time because the previous_old_val is zero on initialization and if the new parameter isn't zero it went the right path.
Some drivers using the battery class e.g. INA266 didn't clear the parameter_update subscription once they checked it's updated.
Some parts were hard to read.

Describe your solution

Remove SimulatorBattery, it only did harm.
Initialize _first_parameter_update with true.
Copy subscription data to a dummy to reset after it was updated.
Refactoring.

Test data / coverage
SITL testing.

It loads the battery parameters but then overwrites them with hardcoded values and it breaks the ModuleParams parent/child hierarchy. Both is undesired.

src/drivers/power_monitor/voxlpm/voxlpm.cpp

dagar · 2020-08-25T20:06:47Z

src/lib/battery/battery.cpp

@@ -186,7 +186,8 @@ Battery::updateBatteryStatus(hrt_abstime timestamp, float voltage_v, float curre
 void
 Battery::publish()
 {
-	orb_publish_auto(ORB_ID(battery_status), &_orb_advert, &_battery_status, &_orb_instance);
+	int dummy; // We're not interested in the instance ID we get
+	orb_publish_auto(ORB_ID(battery_status), &_battery_status_pub, &_battery_status, &dummy);


If you're going to change this how about replacing it with a uORB::PublicationMulti<battery_status_s>?

I thought about that but don't know what's the equivalent of this auto publication. Don't you have to specify the ID in advance? Would you just use the const int _index; of the battery for that?

I tried to add it. Works with one battery (SITL). Is that the correct use?
Does it automatically choose the next free instance?

dagar · 2020-08-25T20:08:16Z

Nice work @MaEtUgR. The battery param migration was a real mess that I'm looking forward to purging once the QGC side is in place.

dagar · 2020-08-26T00:23:30Z

ROMFS/px4fmu_common/init.d-posix/rcS

@@ -119,7 +119,7 @@ if [ $AUTOCNF = yes ]
 then
 	param set SYS_AUTOSTART $REQUESTED_AUTOSTART

-	param set BAT_N_CELLS 3
+	param set BAT_N_CELLS 4


The BatterySimulator is hardcoding this value now so it's what we're already using in SITL atm: https://github.com/PX4/Firmware/blob/4088c2581f38333a306d3cd55aa66b617a51eb93/src/modules/simulator/battery_simulator/BatterySimulator.hpp#L88

IMHO 4S is far more common than 3S these days which was not the case when this was originally set up.

It doesn't matter, the simulated battery generates a voltage that considers the cell count so voltages "just look more familiar" 🤷‍♂️

dagar · 2020-08-26T00:24:56Z

src/lib/battery/battery.h

@@ -230,6 +214,5 @@ class Battery : public ModuleParams
 	uint8_t _warning;
 	hrt_abstime _last_timestamp;

-	orb_advert_t _orb_advert{nullptr};
-	int _orb_instance;
+	orb_advert_t _battery_status_pub{nullptr};


Suggested change

orb_advert_t _battery_status_pub{nullptr};

uORB::PublicationMulti<battery_status_s> _battery_status_pub{ORB_ID(battery_status)};

MaEtUgR · 2020-08-26T09:03:01Z

@dagar Thanks for the quick review!!

fixed analog_battery.cpp include (made CI fail)
fixed syslink_main namespace bracketing (made CI fail)
removed unnecessary parameter_update struct (review comment)
reworded some things (wasn't happy)

Diff: https://github.com/PX4/Firmware/compare/e7c302c4c9eb0122864b10b5310257b39ea5e3cd..dc1870d2dd414d7765a7611289a30cbb96b790e5

MaEtUgR · 2020-08-26T09:43:16Z

I added a commit to switch to PublicationMulti like suggested in #15616 (comment) and #15616 (comment)

src/modules/battery_status/analog_battery.cpp

src/lib/battery/battery.h

The subscription to parameter updates has to get copied otherwise the change detection will not get reset for next time.

This was not a problem before because battery.h included the adc driver and implicitly snprintf was defined through there.

MaEtUgR · 2020-08-26T12:52:30Z

@julianoes Thanks for the review! I hope I addressed the comments to your liking. If you want to discuss migration cases I'm happy to do so.

dagar · 2020-08-28T22:34:03Z

Looks good to me. Let's do an extra round of manual testing here because I'm going to cherry-pick this as the last change for v1.11.

MaEtUgR · 2020-09-01T08:51:19Z

@dagar Thanks for reviewing!
Release port like discussed: #15654

BatterySimulator: remove SimulatorBattery

7977ace

It loads the battery parameters but then overwrites them with hardcoded values and it breaks the ModuleParams parent/child hierarchy. Both is undesired.

MaEtUgR added the bug label Aug 25, 2020

MaEtUgR requested a review from dagar August 25, 2020 18:02

MaEtUgR self-assigned this Aug 25, 2020

dagar reviewed Aug 25, 2020

View reviewed changes

src/drivers/power_monitor/voxlpm/voxlpm.cpp Outdated Show resolved Hide resolved

dagar reviewed Aug 25, 2020

View reviewed changes

dagar reviewed Aug 26, 2020

View reviewed changes

MaEtUgR force-pushed the battery-param-migration-fix branch from e7c302c to 9770e67 Compare August 26, 2020 09:01

MaEtUgR force-pushed the battery-param-migration-fix branch 2 times, most recently from 12bdef1 to dc1870d Compare August 26, 2020 09:30

MaEtUgR force-pushed the battery-param-migration-fix branch from a153aa5 to 233c2c9 Compare August 26, 2020 10:06

julianoes reviewed Aug 26, 2020

View reviewed changes

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

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

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

MaEtUgR added 6 commits August 26, 2020 14:41

battery: fix parameter migration and clarify

f9a32ca

ina226/voxlpm: make sure parameter sub is reset

6c98649

The subscription to parameter updates has to get copied otherwise the change detection will not get reset for next time.

syslink_main: remove empty lines and struct keyword

5e648e0

commander_params: remove some double spaces

1628cbb

analog_battery: fix missing stdio include

4562bb3

This was not a problem before because battery.h included the adc driver and implicitly snprintf was defined through there.

battery: switch to PublicationMulti for battery_status

6cb5644

MaEtUgR force-pushed the battery-param-migration-fix branch from 233c2c9 to 6cb5644 Compare August 26, 2020 12:48

dagar added this to the Release v1.11.0 milestone Aug 28, 2020

dagar approved these changes Aug 28, 2020

View reviewed changes

MaEtUgR merged commit 1d562aa into master Sep 1, 2020

MaEtUgR deleted the battery-param-migration-fix branch September 1, 2020 08:25

MaEtUgR mentioned this pull request Sep 1, 2020

[Port v1.11] Fix Battery Parameter Migration #15654

Merged

bkueng mentioned this pull request Oct 16, 2020

Disable secondary power source by default, fix battery param migration (take 2) #15985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Battery Parameter Migration #15616

Fix Battery Parameter Migration #15616

MaEtUgR commented Aug 25, 2020

dagar Aug 25, 2020

MaEtUgR Aug 26, 2020

MaEtUgR Aug 26, 2020 •

edited

Loading

dagar commented Aug 25, 2020

dagar Aug 26, 2020

MaEtUgR Aug 26, 2020

dagar Aug 26, 2020

MaEtUgR commented Aug 26, 2020 •

edited

Loading

MaEtUgR commented Aug 26, 2020

MaEtUgR commented Aug 26, 2020

dagar commented Aug 28, 2020

MaEtUgR commented Sep 1, 2020

	orb_advert_t _battery_status_pub{nullptr};
	uORB::PublicationMulti<battery_status_s> _battery_status_pub{ORB_ID(battery_status)};

Fix Battery Parameter Migration #15616

Fix Battery Parameter Migration #15616

Conversation

MaEtUgR commented Aug 25, 2020

dagar Aug 25, 2020

Choose a reason for hiding this comment

MaEtUgR Aug 26, 2020

Choose a reason for hiding this comment

MaEtUgR Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

dagar commented Aug 25, 2020

dagar Aug 26, 2020

Choose a reason for hiding this comment

MaEtUgR Aug 26, 2020

Choose a reason for hiding this comment

dagar Aug 26, 2020

Choose a reason for hiding this comment

MaEtUgR commented Aug 26, 2020 • edited Loading

MaEtUgR commented Aug 26, 2020

MaEtUgR commented Aug 26, 2020

dagar commented Aug 28, 2020

MaEtUgR commented Sep 1, 2020

MaEtUgR Aug 26, 2020 •

edited

Loading

MaEtUgR commented Aug 26, 2020 •

edited

Loading