Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Battery: make battery states not sticky when vehicle is not armed #13470

Merged
merged 2 commits into from
Nov 29, 2019

Conversation

RomanBapst
Copy link
Contributor

Problem solved this this PR:
Hot swapping the battery was not supported until now if the battery was already in a state <= warning.
Furthermore, hot swapping using a battery with a lower voltage always lead to the system being in the emergency battery states.

This PR removes the stickiness of the battery levels when the vehicle is NOT armed.

Open Question:
If folks think this is a safety issue then we could make this a parameter. By default the battery warning levels could be sticky always (current state). Then we can introduce to have them sticky only when armed and as a third option to never have them sticky.
I'm saying this because it's not the first time I hear somebody complaining about battery states being sticky.

@LorenzMeier
Copy link
Member

We shouldn't be adding more parameters to the system, in particular not for important stuff like this. So we need to have decent defaults. I think we're a point in complexity where parameters must be taken away and any addition needs to be carefully justified. It makes testing impossible and makes it impossible to make PX4 easy to work with.

@dagar
Copy link
Member

dagar commented Nov 14, 2019

I don't think each "Battery" should have any knowledge of the vehicle arming state.

Why does the Battery warning level need to latch? In commander the battery failsafe is triggered on severity increase and only when armed. Couldn't the Battery warning level simply be allowed to reset to BATTERY_WARNING_NONE?

https://github.com/PX4/Firmware/blob/6369ae858a79cc410c1963c1c0046bfd8a59f2a7/src/modules/commander/Commander.cpp#L3971-L3977

@RomanBapst
Copy link
Contributor Author

@dagar Yeah that might be good as well. Let me look at it and update this.

@RomanBapst
Copy link
Contributor Author

@dagar I updated the PR.

bkueng
bkueng previously approved these changes Nov 20, 2019
@RomanBapst
Copy link
Contributor Author

@PX4/testflights Could you please test the following on a multicopter:

  • make sure you have battery failsafe enabled, e.g. RTL
  • make sure you have your battery threshold setup correctly, e.g. threshold for warning, rtl, land.
    For the sake of your battery I would maybe set the thresholds relatively high so that you don't actually harm your battery, especially when testing the emergency threshold.

Please fly the vehicle and make sure that the failsafe behavior is correct, e.g.

  1. System gives a warning at battery low threshold
  2. System triggers RTL at battery critical threshold
  3. System triggers LAND at battery emergency threshold

Thanks!

@dannyfpv
Copy link

dannyfpv commented Nov 25, 2019

Tested on Pixhawk 4 v5 f-450

Battery threshold warning at 40%, RTL 35%, LAND 30%

Battery warn level: 40%: Battery warning alert
Battery failsafe level: 35%: Failsafe triggered
Battery emergency level: 30%: Land

log:
https://review.px4.io/plot_app?log=4e58c09c-106c-4e30-b628-482035c1a048

Tested on PixRacer v4 F-450

battery warn level: 40%
battery failsafe level: 35%
battery emergency level: 30%
no issue

log:
https://review.px4.io/plot_app?log=8484dfee-c871-4a94-8b28-8112a69b6e76

@Junkim3DR
Copy link

Junkim3DR commented Nov 26, 2019

Tested on NXP FMUK66 v3

Tested on Pixhawk Pro v4

@jorge789
Copy link

jorge789 commented Nov 26, 2019

Tested on PixRacer V4 Master Branch Log Comparison

Modes Tested
Position Mode: Good.
Altitude: Good
Stabilized: Good
RTL (Return To Land): Good.

Procedure
Adjust vehicle Battery Safety settings so that vehicles trigger safety procedures.
Make sure the vehicle triggers the "Low Battery" message and RTL.

Master Branch Log:
https://review.px4.io/plot_app?log=9e3c0e80-ac88-4b20-a102-133269dc7a81

Master Branch Log:
https://review.px4.io/plot_app?log=8cebc31f-a48c-422a-9bcd-cc870cec41c1

@RomanBapst
Copy link
Contributor Author

@PX4/testflights Thanks for testing guys!

@bkueng @dagar I noticed in the logs from the test team that the battery warning state was toggling between two states (now that it's not sticky anymore) which caused the commander spamming battery warning / RTL messages (see figure below).
I now added logic to the commander which makes the internal battery warning state sticky while the vehicle is armed. This prevents the effects we see in the log but still allows the battery state to recover when the vehicle is disarmed.
I first thought about adding a hysteresis but looking at the log I think in most cases battery estimates are just too unreliable to make this work nicely.
Please have a look.
image

Signed-off-by: RomanBapst <bapstroman@gmail.com>
Signed-off-by: RomanBapst <bapstroman@gmail.com>
@RomanBapst
Copy link
Contributor Author

@PX4/testflights Yould you please give this another test flight? You don't necessary need to fly this on all platforms, it's sufficient if you test on Pixhawk 4 v5 f-450. Thanks!

@Junkim3DR
Copy link

Junkim3DR commented Nov 27, 2019

Tested on Pixhawk 4 v5

@RomanBapst RomanBapst merged commit 133a6e3 into master Nov 29, 2019
@RomanBapst RomanBapst deleted the pr-battery-hot-swap branch November 29, 2019 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants