vtol_type: Added position feedback to VTOL backward transition #23731

oravla5 · 2024-09-27T08:13:54Z

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

RomanBapst · 2024-09-27T10:55:12Z

src/modules/vtol_att_control/vtol_type.cpp

-	// 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;


@oravla5 Maybe we can leave away this magic factor and see how it goes with it.

src/modules/vtol_att_control/vtol_type.cpp

sfuhrer

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?

oravla5 · 2024-10-04T07:04:31Z

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 TranstiionFlightTask.
However, I think this would be a different, way bigger change in the code, outside of the scope of this PR. The goal of this PR was just replacing the acceleration setpoint computation logic (from a constant value to a dynamic one). In my view, moving that logic somewhere else in the codebase would be a different task. What would you think about addressing the latter in a different PR?

Jaeyoung-Lim

@oravla5 I agree with @sfuhrer, and I am not sure if all the edge cases are being handled for this change to be able to be merged in upstream.

Jaeyoung-Lim · 2024-10-04T08:12:58Z

src/modules/vtol_att_control/vtol_type.cpp

+
+		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);


Does this mean we will not be able to back transition without a valid global position estimate? How would this be handled?

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 👌

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?

oravla5 · 2024-10-04T08:48:33Z

Hey @Jaeyoung-Lim , thanks for the feedback!

I am not sure if all the edge cases are being handled for this change to be able to be merged in upstream.

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?

Jaeyoung-Lim · 2024-10-04T08:56:11Z

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.

RomanBapst · 2024-10-04T09:17:25Z

@sfuhrer @oravla5 @Jaeyoung-Lim I think Silvan's input of trying to move this logic to the Transition flighttask is good.
The more logic that is related to creating attitude setpoints we can move out of this module and into the flighttask, the better. I can help @oravla5 to make the required changes (assuming for now that it's feasible).

@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.

oravla5 · 2024-10-04T09:22:08Z

@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.

Jaeyoung-Lim · 2024-10-04T12:06:13Z

src/modules/vtol_att_control/vtol_type.cpp

+
+	float dec_sp = default_dec_sp;
+
+	if (_attc->get_pos_sp_triplet()->current.valid && _attc->get_local_pos()->xy_valid) {


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?

Jaeyoung-Lim · 2024-10-05T10:32:04Z

@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.

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 cutting corners in my opinion. We already have so many corner cases in the fw_pos_control module, because we don't test enough.

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.hpp

…tpoint computation

oravla5 · 2024-10-23T14:45:10Z

@Jaeyoung-Lim @sfuhrer
During the last week we have addressed the major concerns you shared about the initial implementation of these changes and performed some extensive testing. Our observation during testing was that the risk of overshooting during back-transition was significantly reduced. In the few cases we experienced overshooting, (situations with tail- or side-wind), its magnitude was noticeably reduced.

Find below links to some of the test flights we performed with a standard VTOL.

sfuhrer

Cool feature but I have a couple of comments about the implementation.

use matrix::Vector2f for geometrical calculations, then you get handy functions like .dot() and .abs() and it makes it easier to follow the code
protect against invalid global position (local_position.xy_global)

sfuhrer · 2024-11-01T09:46:43Z

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp

@@ -100,6 +75,13 @@ bool FlightTaskTransition::activate(const trajectory_setpoint_s &last_setpoint)
 	return ret;
 }

+bool FlightTaskTransition::updateInitialize()


Why did you move these function definitions around? It creates a lot of unnecessary diff, or at least makes an efficient reviewing impossible.. Please separate it into pure refactor commits and then commits that change functionality.

I used the opportunity to do some minor refactoring of the class. If definitions were moved around, it's probably because they were not following the same order as in the header file. I hope this is ok since the amount of changes is extremely minor.

It's good that you take care of refactors like that, I just ask you to put it in different commits next time.

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp

oravla5 · 2024-11-04T15:27:00Z

@sfuhrer I implemented the requested changes. I'm getting local build errors due to flash (roughly 700 bytes). What would be the suggested course of action in this regard. FYI @RomanBapst

…comments Signed-off-by: Silvan Fuhrer <silvan@auterion.com>

#23731) Instead of tracking a fixed deceleration setpoint during the backtransition we added here position feedback, such that the vehicle comes to a stop latest at the current position setpoint. This reduces the risk of overshooting the landing point. If no position feedback/position setpoint is present the old logic still applies. It further moves the braking controller to the FlightTaskTransition instad of doing it in the VTOL attitude mode. * vtol_type: added position feedback to backward transition * FlgithTaskTransition: refactored backtransition deceleration/pitch setpoint computation * FlightTaskTransition: minor improvements * FlightTaskTransition: use .dot() consistently and remove unnecessary comments Signed-off-by: Silvan Fuhrer <silvan@auterion.com> --------- Signed-off-by: Silvan Fuhrer <silvan@auterion.com> Co-authored-by: Silvan Fuhrer <silvan@auterion.com>

vtol_type: added position feedback to backward transition

8fc808d

RomanBapst reviewed Sep 27, 2024

View reviewed changes

src/modules/vtol_att_control/vtol_type.cpp Outdated Show resolved Hide resolved

RomanBapst reviewed Sep 27, 2024

View reviewed changes

src/modules/vtol_att_control/vtol_type.cpp Outdated Show resolved Hide resolved

vtol_type: addressed PR comments

a99d585

oravla5 force-pushed the pr-vtol-backtransition-pos-feedback branch from 1462703 to a99d585 Compare September 27, 2024 15:19

oravla5 requested a review from RomanBapst September 27, 2024 15:19

sfuhrer reviewed Oct 1, 2024

View reviewed changes

Jaeyoung-Lim requested changes Oct 4, 2024

View reviewed changes

oravla5 requested a review from Jaeyoung-Lim October 4, 2024 08:53

Jaeyoung-Lim reviewed Oct 4, 2024

View reviewed changes

oravla5 requested review from Jaeyoung-Lim and sfuhrer October 15, 2024 08:52

oravla5 force-pushed the pr-vtol-backtransition-pos-feedback branch 2 times, most recently from c4a062c to 2888b21 Compare October 15, 2024 08:57

RomanBapst reviewed Oct 15, 2024

View reviewed changes

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp Outdated Show resolved Hide resolved

RomanBapst reviewed Oct 15, 2024

View reviewed changes

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp Outdated Show resolved Hide resolved

RomanBapst reviewed Oct 15, 2024

View reviewed changes

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.hpp Outdated Show resolved Hide resolved

RomanBapst reviewed Oct 15, 2024

View reviewed changes

src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.hpp Outdated Show resolved Hide resolved

oravla5 force-pushed the pr-vtol-backtransition-pos-feedback branch from 2888b21 to 5cf5217 Compare October 15, 2024 09:05

FlgithTaskTransition: refactored backtransition deceleration/pitch se…

fa6f14b

…tpoint computation

oravla5 force-pushed the pr-vtol-backtransition-pos-feedback branch from 5cf5217 to fa6f14b Compare October 15, 2024 09:07

oravla5 added 2 commits October 15, 2024 16:44

addressed PR comments

43c7aa8

FlightTaskTransition: fly test changes

4b5b3f2

oravla5 added 5 commits October 23, 2024 15:17

FlightTaskTransition: fixed double radian conversion

932af0c

FlightTaskTransition: minor improvements

1a243fe

FlightTaskTransition: reduced max_deceleration_sp factor

1e59565

FlightTaskTransition: fixes and improvements

44b6270

FlightTaskTransition: reverted some dev changes and applied format

dde15e6

Merge branch 'main' into pr-vtol-backtransition-pos-feedback

e39685b

sfuhrer requested changes Nov 1, 2024

View reviewed changes

oravla5 added 2 commits November 4, 2024 05:26

Merge branch 'main' into pr-vtol-backtransition-pos-feedback

cdf616c

FlightTaskTransition: refined implementation

e592e1a

FlightTaskTransition: use .dot() consistently and remove unnecessary …

eb2553b

…comments Signed-off-by: Silvan Fuhrer <silvan@auterion.com>

sfuhrer approved these changes Nov 7, 2024

View reviewed changes

Merge branch 'main' into pr-vtol-backtransition-pos-feedback

7a85323

sfuhrer merged commit 079b756 into PX4:main Nov 11, 2024
51 of 53 checks passed

MaEtUgR mentioned this pull request Dec 18, 2024

Clean up Transition Flighttask #24126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vtol_type: Added position feedback to VTOL backward transition #23731

vtol_type: Added position feedback to VTOL backward transition #23731

oravla5 commented Sep 27, 2024 •

edited

Loading

RomanBapst Sep 27, 2024

sfuhrer left a comment

oravla5 commented Oct 4, 2024

Jaeyoung-Lim left a comment

Jaeyoung-Lim Oct 4, 2024 •

edited

Loading

oravla5 Oct 4, 2024

Jaeyoung-Lim Oct 4, 2024 •

edited

Loading

oravla5 commented Oct 4, 2024 •

edited

Loading

Jaeyoung-Lim commented Oct 4, 2024 •

edited

Loading

RomanBapst commented Oct 4, 2024

oravla5 commented Oct 4, 2024

Jaeyoung-Lim Oct 4, 2024

Jaeyoung-Lim commented Oct 5, 2024 •

edited

Loading

oravla5 commented Oct 23, 2024

sfuhrer left a comment

sfuhrer Nov 1, 2024

oravla5 Nov 4, 2024

sfuhrer Nov 6, 2024

oravla5 commented Nov 4, 2024


		float dec_sp = default_dec_sp;

		if (_attc->get_pos_sp_triplet()->current.valid && _attc->get_local_pos()->xy_valid) {

vtol_type: Added position feedback to VTOL backward transition #23731

vtol_type: Added position feedback to VTOL backward transition #23731

Conversation

oravla5 commented Sep 27, 2024 • edited Loading

Solved Problem

Solution

Testing

RomanBapst Sep 27, 2024

Choose a reason for hiding this comment

sfuhrer left a comment

Choose a reason for hiding this comment

oravla5 commented Oct 4, 2024

Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

Jaeyoung-Lim Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

oravla5 Oct 4, 2024

Choose a reason for hiding this comment

Jaeyoung-Lim Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

oravla5 commented Oct 4, 2024 • edited Loading

Jaeyoung-Lim commented Oct 4, 2024 • edited Loading

RomanBapst commented Oct 4, 2024

oravla5 commented Oct 4, 2024

Jaeyoung-Lim Oct 4, 2024

Choose a reason for hiding this comment

Jaeyoung-Lim commented Oct 5, 2024 • edited Loading

oravla5 commented Oct 23, 2024

sfuhrer left a comment

Choose a reason for hiding this comment

sfuhrer Nov 1, 2024

Choose a reason for hiding this comment

oravla5 Nov 4, 2024

Choose a reason for hiding this comment

sfuhrer Nov 6, 2024

Choose a reason for hiding this comment

oravla5 commented Nov 4, 2024

oravla5 commented Sep 27, 2024 •

edited

Loading

Jaeyoung-Lim Oct 4, 2024 •

edited

Loading

Jaeyoung-Lim Oct 4, 2024 •

edited

Loading

oravla5 commented Oct 4, 2024 •

edited

Loading

Jaeyoung-Lim commented Oct 4, 2024 •

edited

Loading

Jaeyoung-Lim commented Oct 5, 2024 •

edited

Loading