-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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: Rely less on battery capacity based estimate when voltage is low #22761
Conversation
src/lib/battery/battery.cpp
Outdated
@@ -239,10 +239,10 @@ float Battery::calculateStateOfChargeVoltageBased(const float voltage_v, const f | |||
void Battery::estimateStateOfCharge() | |||
{ | |||
// choose which quantity we're using for final reporting | |||
if (_params.capacity > 0.f && _battery_initialized) { | |||
if ((_params.capacity > 0.f) && _battery_initialized && (_state_of_charge_volt_based > _params.emergen_thr)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This jump could be a bit weird if you're on the line? Maybe it needs to latch or have a big hysteresis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Maybe it's enough to go with the filter adjustments below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This jump could be a bit weird if you're on the line?
I agree but it's only there to save the user from crashing the vehicle after he already has a seriously misconfigured battery estimation.
We can try to be super smart and make it unnoticeable that the capacity was set wrong but what was the point of having the parameter in the first place? I expected we can rely on it being correct if it's set.
Maybe it's enough to go with the filter adjustments below?
If the capacity is set completely wrong like in some logs I saw it will still react too slow and the vehicle might crash even though "it can be seen from the voltage that it was too low".
Well it is also a problem when somebody flies with a valid Ah value for the battery and power cycles between flights with the logic before the change |
// if battery capacity is known, fuse voltage measurement with used capacity | ||
// The lower the voltage the more adjust the estimate with it to avoid deep discharge | ||
const float weight_v = 3e-4f * (1 - _state_of_charge_volt_based); | ||
const float weight_v = 3e-2f * (1 - _state_of_charge_volt_based); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to give voltage more weight, especially as it's anyway only fed in when it gets depleted.
Where does this magic number 0.0003 resp 0.03 now come from? Worth a comment in my eyes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These come from testing I did back at Yuneec for the H520 where the previous weight was used on the product because it resulted in the best user experience. With the fine-tuned battery parameters for the single type of battery that was sold with the product the SOC was decreasing pretty linearly until the very end of the flight with light or heavy load, the battery was connected when not full and when the battery already aged a bit.
The 100 times higher weight produced more fluctuations and non-linear decreasing of the SOC but since I have not seen a single user or company spending the extra time at analyzing data and tuning these parameters I suspect it leads to overall better satisfaction.
I should have kept and uploaded the way I was analyzing this, it's been a while but I showed examples here: #8153
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should I put that in a comment? I mean I'm open to any weighting.
Ideally we'd have a python script that gives you parameter suggestions based on flight logs and some margin setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// weighting was determined heuristically
I meant something like that. Just to be clear that this is not linked to any mathematical or physical property.
You mean that the init of the SoC based on voltage is too crude? What better option do we have? |
I was trying to emphasize that previously it was a problem even if a valid Capacity was set for the battery and the user powercycles between flights with the same battery the SOC would be off and would higher the odds of a crash. So this would just be an additional problem solved. |
No, if the parameters were set correctly it all worked. It was adjusting to the correct SOC even if the initialy voltage-based one was not so accurate. It's in the product exactly like that, I spent about a week tuning the parameters for the specific battery and they sold multiple thousands after that and battery estimation while not as good as with a smart battery was never an issue also not with aged batteries. See also #22761 (comment) |
Tested here on VTOL: 20Ah Lipo (16Ah usable) Fly until battery warning at 15% while monitoring Battery voltage Verdict: Looks like this safety net works. Right at 21.6V (Battery empty) it triggered the battery failsafe aligning with 15% SOC. Battery on Ground resting at 25%, Powercycling, Battery at 29% https://logs.px4.io/plot_app?log=dabfd0bc-b025-4e74-93c3-a228e5515701 |
Thanks for the testing @jongell ! Given on these results it seems like the new filter tuning does the job, and we could get rid of the additional hard voltage threshold. |
This is a minimal change to make it harder to crash a vehicle with an empty battery if the capacity was set wrong. The disadvantage is that the state of charge estimate will fluctuate more under load. We need better documentation and improvements to the estimation.
7ae28f6
to
fb8b612
Compare
@jongell Thanks a lot for providing the test data! In the test data it never got into the hard voltage threshold which could cause other problems like e.g. sudden landing in the middle of nowhere. So I removed that part again for now and rebased the weight change on main. |
Solved Problem
When analyzing test vehicle crashes I found that it happens regularly that people set a "random" battery capacity and then still rely on the battery estimate which is in that case completely wrong.
Solution
This is a minimal change to make it harder to crash a vehicle with an empty battery if the capacity was set completely wrong.
Changelog Entry
Alternatives
We need better documentation and improvements to the estimation.
Test coverage
Not tested yet. I'll see if there's a simple simulation test to verify.Check this comment: #22761 (comment)
Context
FYI @jongell @Dani3L9H