-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
RTL based on remaining flight time estimation #18646
Conversation
- Compute RTL time also during RTL - Calculate correct altitude when finding destination
ef8ce01
to
6664ffb
Compare
I rebased on top of master with the small conflict: https://github.com/PX4/PX4-Autopilot/pull/18645/files#diff-a58612b34e98d7c47e0b13294ccc0c5077dbab967450a7bc84acd596825a8151R86 |
uint64 timestamp # time since system start (microseconds) | ||
|
||
float32 rtl_time_s # how long in seconds will the RTL take | ||
float32 rtl_limit_fraction # what fraction of the allowable RTL time would be taken |
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.
@MaEtUgR Thanks for removing this, I always had trouble with this
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 it's fine to bring it in without major changes, though we should think about the following for the next iteration:
- I think it should be either the old battery state check triggering the warning/RTL/Land action, or this new range based RTL
- VTOL average current calculation needs some revising, probably it makes most sense if it only is updated in FW flight, and for the MC part of the flight the user sets reasonable safety margins. Or we add separate estimate for MC flight as well
- think about estimating power consumption or an unitless "state or charge per second" instead of current estimation (current increases towards end of flight)
// Try to trigger RTL | ||
if (main_state_transition(_status, commander_state_s::MAIN_STATE_AUTO_RTL, _status_flags, | ||
_internal_state) == TRANSITION_CHANGED) { | ||
mavlink_log_emergency(&_mavlink_log_pub, "Flight time low, returning to land"); |
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 about "Remaining" flight time low, return to land
?
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.
mavlink_log_emergency(&_mavlink_log_pub, "Flight time low, returning to land"); | |
mavlink_log_emergency(&_mavlink_log_pub, "Remaining flight time low, returning to land"); |
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 guess we should also add an event here and not only the legacy mavlink log?
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.
} else if (battery_at_home < _param_bat_low_thr.get()) { | ||
battery_range_warning = battery_status_s::BATTERY_WARNING_LOW; | ||
} else { | ||
mavlink_log_emergency(&_mavlink_log_pub, "Flight time low, land now!"); |
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.
mavlink_log_emergency(&_mavlink_log_pub, "Flight time low, land now!"); | |
mavlink_log_emergency(&_mavlink_log_pub, "Remaining flight time low, land now!"); |
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.
* @increment 1 | ||
* @group Return To Land | ||
*/ | ||
PARAM_DEFINE_INT32(RTL_TIME_MARGIN, 110); |
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.
Not that it really matters, but it's an interesting default, why not making it 100s or 2min?
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.
Taken over from some old real use case. Not sure how it ended up being this value. A round number makes more sense as default indeed.
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.
* | ||
* @unit s | ||
* @min 0 | ||
* @max 300 |
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.
300s sounds quite low as max, for vehicle flying multiple hours I would for sure want a bigger margin.
Would propose 3600 as max.
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.
What issues do you currently see?
I think this could make sense. If we can get a reasonable "state of energy" estimate in [Wh] then the accuracy might be improved since it would be independent from current increase towards the end. |
Let's say the VTOL uses about 10x as much energy for hover as for cruise. If you update the consumption average estimation independently of the flight phase, then the remaining flight times drops by a factor of 10 when you transition to hover, likely triggering a RTL if the vehicle is at some distance to home. But as I said, let's tackle that in a follow up. |
Thanks for the review! I'm quickly fixing the smaller issues @sfuhrer pointed out and then we can follow up with further improvements. I generally agree with all the points and am happy to discuss the details. |
Or may I merge this and create an immediate follow-up for the fixes? That would help the work flow 😇 |
Whatever you prefer |
I added the follow up here: #18711 Good input! |
@MaEtUgR What is the docs implication of this? I see Specifically, what should a user set or not set to use this? It appears to be triggered of a low battery - what battery level? What if there are multiple batteries? How is it enabled/disabled? |
Describe problem solved by this pull request
The current range-based RTL from #16399 needs to be configured with a flight time per battery which would need to be adjusted to the environment, flight speed, payload, ... every time it is used and results in very coarse estimate. It's IMO also less transparent for user feedback because the
rtl_limit_fraction
is not an intuitive graspable number.Describe your solution
The approach I implemented in summary uses the remaining battery time from the battery estimation #17828 and compares it against a precise estimate how long the RTL will take. It triggers the RTL a configurable margin before that such that there's enough time to return before the battery is empty. This results in a different return time depending on the average current consumption and distance the vehicle is away from where it wants to land again e.g. home.
Test data / coverage
This strategy was tested a lot before but not in the ported form of this pr.