vtol_type: Added position feedback to VTOL backward transition #23731
Conversation
| // to make overshooting the transition waypoint less likely in the presence of tracking errors | ||
| const float dec_sp = _param_vt_b_dec_mss.get() * 1.2f; | ||
| // Add 20% factor to maximum allowed deceleration | ||
| const float default_dec_sp = _param_vt_b_dec_mss.get() * 1.2f; |
There was a problem hiding this comment.
@oravla5 Maybe we can leave away this magic factor and see how it goes with it.
| // Add position feedback to the deceleration setpoint calculation | ||
|
|
||
| // Compute backtransition end-point in local reference frame body x-direction -> dist_body_forward | ||
| MapProjection map_proj{_attc->get_local_pos()->ref_lat , _attc->get_local_pos()->ref_lon}; |
There was a problem hiding this comment.
Maybe also check if ref_lat, ref_lon are valid
|
|
||
| // Check if the deceleration setpoint is within limits | ||
| if (!PX4_ISFINITE(dec_sp) || (dec_sp > max_dec_sp) || (dec_sp < 0.0f)) { | ||
| dec_sp = default_dec_sp; |
There was a problem hiding this comment.
@oravla5 I think instead of resetting to the default deceleration you can constrain the value to be within the limits.
oravla5
force-pushed
the
pr-vtol-backtransition-pos-feedback
branch
from
1462703
to
a99d585
Compare
There was a problem hiding this comment.
Doing all this position setpoint handling directly in the VTOL module (that is supposed to be an "attitude" controller) is quite ugly and could lead to many unforeseen corner cases. Have you thought about doing it instead in the FlightTaskTransition?
|
Thanks for the feedback @sfuhrer! I agree with your observation and indeed the logic to compute the acceleration setpoint logic, which in turn is used to define the pitch setpoint, could be moved somewhere else, i.e. the |
|
|
||
| float pos_sp_x, pos_sp_y = 0.f; | ||
| map_proj.project(_attc->get_pos_sp_triplet()->current.lat, _attc->get_pos_sp_triplet()->current.lon, pos_sp_x, | ||
| pos_sp_y); |
There was a problem hiding this comment.
Does this mean we will not be able to back transition without a valid global position estimate? How would this be handled?
There was a problem hiding this comment.
Hey @Jaeyoung-Lim , this is handled 7 lines above, in the if condition:
if (_attc->get_pos_sp_triplet()->current.valid && _attc->get_local_pos()->xy_valid)
If the available global position is not valid, the new logic defaults to the parameter value, as before these changes 👌
There was a problem hiding this comment.
I might be wrong, but your local position setpoint can be valid, but this doesn't mean you have a valid global position estimate(the reference of the local position might be wrong). Wouldn't this introduce problems with the map projection below?
|
Hey @Jaeyoung-Lim , thanks for the feedback!
I understand your concerns. The changes I implemented check that the computed deceleration setpoint is a finite value, if not, it is set to the default constant acceleration setpoint (defined by the existing parameter). The resulting value is also guaranteed to be within 0 and the maximum deceleration setpoint value (defined as a function of the decelaration setpoint parameter). Could you be more specific about other edge cases that may not be covered here? |
For me, this is the concern. The fact that we have a cascaded control structure (att control, position control), but you are using the position as part of the logic makes it unclear how the two loops interact. I see that to some degree this is already the case, but I am not so happy to see more extensions in this direction is being made. @oravla5 You mention the "bigger" change that is needed to change this. Why not just do it as part of this PR, so we can improve the quality of the implementation with this PR? I fear that we merge this as a quick fix without any concrete plans, along side many more quick fixes, and it will be at the same state when we arrive at the end of this year. |
|
@sfuhrer @oravla5 @Jaeyoung-Lim I think Silvan's input of trying to move this logic to the Transition flighttask is good. @Jaeyoung-Lim Keep in mind that @oravla5 has just started working with PX4 a couple of months and might not always be aware of the existing "messes". So it might be more useful to make concrete suggestions, e.g. providing a link to the transition flighttask. |
|
@Jaeyoung-Lim So just to be clear, your concerns are not about uncovered edge cases, but about the code structure. As I said above, I agree with these concerns. However, adhering to atomic PR's that, when possible, address a single topic / concern is considered a good practice that eases the review process and reduces the risk of missing out things. In this case, the topic is the deceleration setpoint computation logic. This logic was already there, and these changes just try to bring an improvement over the previous implementation. The changes were tested in flight and we were able to observe a better back transition behavior of the vehicle compared to the previous logic. The second topic that you and @sfuhrer brought is about the code structure of the deceleration setpoint computation logic code structure. I agree the way it was and continues to be is not ideal, and I am happy to address it in a different PR. |
|
|
||
| float dec_sp = default_dec_sp; | ||
|
|
||
| if (_attc->get_pos_sp_triplet()->current.valid && _attc->get_local_pos()->xy_valid) { |
There was a problem hiding this comment.
Does the position setpoint triplet become invalid if you are triggering back transition from a manual flight mode? If not, why is it relevant that the validity of the current setpoint matters?
I think you misunderstood my concerns. The changes in this PR, is effectively adding dependency to a position setpoint triplet, which uses global position. While dependency on the position setpoint triplet existed before, THIS PR uses a map projection from global to local, into vtol attitude control. I have added my comments regarding concerns of the edge cases with validity of global position and local position estimates in the code. However, I think the bigger concern is that because now that the attitude control and position control have a circular dependency it is hard to even identify what corner cases it would have for the future. I simply don't understand the hesitancy to improve the structure within this PR, which will save you a lot of time, since it would reduce the amount of testing you would need to do to consider all the different edge cases. As you mention the flight test, has this been tested in cases where the back transition is triggered from manual flight modes, loss of global position estimates while the local position estimate is valid (e.g. manual flight modes)? And has there been any investigation that these two would be the only corner cases? If the "clean" structure was already in place to fix this problem, these edge case testing would not have been necessary. However, if we want to make this change without improving the structure of the code, these tests are necessary and otherwise we will compromise ourselves to even bigger problems in the future. I agree with small atomic changes are better than refactoring the whole code base without solving problems. BUT if the small atomic changes are a potential liability for problems that didn't exist previously, I don't think this is worth fixing in my opinion. We already have so many corner cases in the fw_pos_control module, because we don't test enough to fix a small problem. |
Solved Problem
VTOL back-transition logic was open-loop on position, which resulted in usual over / undershoots w.r.t. the back-transition end-point.
Solution
Added position feedback to the VTOL back-transition logic. The distance between the vehicle and the back-transition end The acceleration setpoint is then computed using a rectilinear kinematic model.
Testing
A first version of this changes (not including some of the sanity checks) was tested on a Standard VTOL, see associated logs here