-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
commander: fix battery failsafe without GPS #13294
Conversation
@PX4/testflights please test this on the cheapest frame as follows: For these tests I suggest to set the battery failsafe thresholds higher in QGC -> Settings -> Safety. (e.g. warning at 60%, failsafe at 50%, emergency at 20%. That way you don't need to risk damaging batteries. Test 1: Land with GPS
Test 2: Land without GPS -> only when no wind!
Test 3: RTL with GPS
Test 4: RTL without GPS -> only when no wind!
Thanks and let me know if you have any questions! |
@julianoes Thanks for providing a really good test plan. You're setting a very good example with that and a lot of PRs are lacking that. |
This fixes the battery failsafe for the following corner cases: - Battery failsafe set to Return but we can't do RTL because we don't have a global position or home position. In this case we now switch to Land. Land might end up in Descend in the failsafe state machine later. - Battery failsafe set to Land but we can't land because we don't have a local position. In this case we switch to land anyway and then fall back to descend in the failsafe state machine later. The "fix" involves ignoring using the main_state_transition and implementing the guards in place. This is a hack for now but should cover the corner case until a more thorough refactor. The different failsafe state machines have involved over time from requirements and learnings based on developed solutions and products. The implementations in various places will need to get consolidated in the future. Tested in SITL for Return and Land with and without GPS.
f3505fe
to
e3783a3
Compare
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.
To me it is now clearer what happens when RTL or similar are executing without GPS.
Tested on Pixhawk v4pro f-450 Test 1: Land with GPS Test 2: Land without GPS Test 3: RTL with GPS Test 4: RTL without GPS |
@dannyfpv Thanks a lot for testing and the logs. I'm investigating the problem where it does not auto-disarm on landing. From the log I can see that landing is detected but then disarm does not happen. I can't reproduce this behaviour in SITL, so I'm wondering what might be different. One thing that caught my eye is the weird noise/spikes in the mag values. It's something we need to investigate. I don't know if this has something to do with the fact that auto-disarm does not work. |
FC: PixhawkPro |
@julianoes the landing was detected, once the vehicle triggered the failsafe landing was detected. The reason why we switched to manual is to disarm the vehicle by using the kill switch. We can give it another go on Monday. Get more data to see if we can figure out what could be the issue. |
@@ -1194,13 +1195,9 @@ void battery_failsafe(orb_advert_t *mavlink_log_pub, const vehicle_status_s &sta | |||
|
|||
// FALLTHROUGH | |||
case LOW_BAT_ACTION::LAND: | |||
if (TRANSITION_DENIED != main_state_transition(status, commander_state_s::MAIN_STATE_AUTO_LAND, status_flags, |
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.
@julianoes Sorry, I can't comment on a line which you did not change but don't you need to put the LOW_BAT_ACTION::RETURN_OR_LAND case before the LOW_BAT_ACTION::RETURN case, as you have done for the critical battery level section? As I understand it will otherwise fall through to landing and never RTL.
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.
I think this is correct because in the emergency case we want to land, so we fallthrough to land.
In the critical (non-emergency) case we still do RTL.
Tested on Pixhawk4 v5 f-450 Test 1: Land with GPS Test 2: Land without GPS Test 3: RTL with GPS Test 4: RTL without GPS @julianoes the same behavior in all the tests the vehicle did not disarm upon touching the ground. |
@dannyfpv when you do a normal RTL with the same vehicle on master: does auto-disarm work? |
@dannyfpv I think your logs got a bit messed up above. The first two both seem to have GPS. |
Does that mean that the failsafe was done or that landing was actually detected and the props were spinning slower (but not disarmed)? |
From the logs I can tell that the problem seems to be the land estimator not triggering, rather than auto-disarm not working. @bresch I'm looking at this log and I can't figure out why the land detector doesn't trigger: |
Test 2: Land without GPS Test 3: RTL with GPS |
|
tested on pixhawk4 v4 f-450 tested on pixhawk v4pro f-450 |
Ok, I'll merge this given the land detection issues are unrelated. |
This fixes the battery failsafe for the following corner cases:
The "fix" involves ignoring using the main_state_transition and implementing the guards in place. This is a hack for now but should cover the corner case until a more thorough refactor.
The different failsafe state machines have involved over time from requirements and learnings based on developed solutions and products. The implementations in various places will need to get consolidated in the future.
Tested in SITL for Return and Land with and without GPS.
Fixes #13291.