VTOL parameter cleanup #8967

RomanBapst · 2018-02-26T16:36:22Z

moved many generic parameters out of standard.cpp into vtol_type class so that not every vtol_type needs to define the same parameter again
cleaned up usage of hrt_elapsed_time (thanks @bkueng for suggestions)

Needs CAREFUL review and fair amount of testing!

Next: Do the same thing for tiltrotor.cpp and tailsitter.cpp

SITL testing:

Standard VTOL Transitions

Tiltrotor Transitions

Tailsitter Transitions

dagar · 2018-02-26T22:12:08Z

src/modules/vtol_att_control/standard.cpp

@@ -277,39 +233,38 @@ void Standard::update_vtol_state()
 void Standard::update_transition_state()
 {
 	float mc_weight = 1.0f;
+	float time_now = (float)hrt_absolute_time();


Why are you keeping this as a float?

@dagar Are you only asking about time_now or also about time_since_trans_start below?
You are right, I don't need to have this as a float, I can cast it to a float in the computation of time_since_trans_start.
Not sure if you were debating about storing values as float vs integers.

I mean keep it as a uint64 if working in microseconds, or convert to seconds and keep it as a float. For everything in this VTOL controller seconds is good enough, and easier to work with (fits with parameters).

RomanBapst · 2018-02-27T10:19:40Z

src/modules/vtol_att_control/standard.cpp


 	/* duration of a forwards transition to fw mode */
-	param_get(_params_handles_standard.front_trans_dur, &v);
-	_params_standard.front_trans_dur = math::constrain(v, 0.0f, 20.0f);


@dagar What do you think about these things? If we want them we need to add them to the vtol_att_control_main.cpp which handles most params now. The questions is if this makes sense in general?

The intent is good, but I don't like that your actual parameter value isn't used. In many cases these constraint aren't necessary because the param metadata prevents invalid user data. At a minimum I'd want to see the effective parameter value committed.

I agree to remove the constrain as UI will make clear not to go beyond that

RomanBapst · 2018-02-27T10:22:26Z

src/modules/vtol_att_control/standard.cpp

 			// Handle throttle reversal for active breaking
-			float thrscale = (float)hrt_elapsed_time(&btrans_start) / (_params_standard.front_trans_dur *
-					 1000000.0f);
+			float thrscale = (time_now - btrans_start) / (_params_standard.pusher_ramp_dt * 1e6f);


@sanderux I need your input here. You were using the parameter for the front transition duration for this calculation related to active breaking. I introduced a parameter for the pusher ramp up time so that we don't need to abuse the front transition time param anymore. Can you plz check if I used the correct param?

RomanBapst · 2018-02-28T08:07:58Z

@dagar We decided to move away from computing elapsed time in microseconds and compute elapsed time in seconds instead. It avoids all the conversions as all our parameters are already in seconds. Can you have a look?

RomanBapst · 2018-02-28T09:04:45Z

src/modules/vtol_att_control/tiltrotor.cpp

-					  (_params_tiltrotor.airspeed_trans - _params_tiltrotor.airspeed_blend_start);
+		if (!_params->airspeed_disabled && _airspeed->indicated_airspeed_m_s >= _params->airspeed_blend) {
+			_mc_roll_weight = 1.0f - (_airspeed->indicated_airspeed_m_s - _params->airspeed_blend) /
+					  (_params->transition_airspeed - _params->airspeed_blend);


Need to put the check somewhere to make sure that transition_airspeed and airspeed_blend are not equal.

Did you do that already?

@bkueng Yes, I put in a constraint which holds for all three vtol types.

dagar · 2018-02-28T13:06:19Z

src/modules/vtol_att_control/standard.cpp

@@ -277,39 +232,36 @@ void Standard::update_vtol_state()
 void Standard::update_transition_state()
 {
 	float mc_weight = 1.0f;
+	float time_since_trans_start = (float)(hrt_absolute_time() - _vtol_schedule.transition_start) * 1e-6f;


float time_since_trans_start = hrt_elapsed_time(&_vtol_schedule.transition_start) * 1e-6f

dagar · 2018-02-28T13:09:19Z

src/modules/vtol_att_control/standard.cpp

+		} else if (_params->airspeed_disabled &&
+			   time_since_trans_start < _params->front_trans_time_min &&
+			   time_since_trans_start > _params->front_trans_time_min / 2.0f) {
+			mc_weight = 1.0f - ((time_since_trans_start -  _params->front_trans_time_min / 2.0f) /


I believe this simplifies to
float weight = 1.0f - (time_since_trans_start / _params->front_trans_time_min);

@dagar Not really, I think the intent is to start the weight from zero when time_since_trans_start is half of front_trans_time_min.
By the way, I mostly wanted to keep this PR as pure refactoring and not change any functional stuff.

I'm talking about keeping it the same functionally, but in a somewhat comprehensible form.

const float weight = 1.0f - (time_since_trans_start / _params->front_trans_time_min); _mc_throttle_weight = math::constrain(2.0f * weight, 0.0f, 1.0f);

dagar · 2018-02-28T13:12:52Z

@dagar We decided to move away from computing elapsed time in microseconds and compute elapsed time in seconds instead. It avoids all the conversions as all our parameters are already in seconds. Can you have a look?

Yes that's fine, the problem I saw earlier was using microseconds in a float.

RomanBapst · 2018-03-01T15:11:25Z

@dagar I'd like to get this in as soon as possible. Do you think we should ask the flight test team to give it a try?

RomanBapst · 2018-03-01T15:39:09Z

@dagar I verified that #8988 is not related to this because it also is happening on master. So I'm pretty confident about this.

dagar · 2018-03-01T16:40:41Z

It all looks reasonable and likely works with the defaults. The only concern is that we miss some trivial param thing because there's almost no error handling. That's one of the reasons I wanted to go to BlockParam.

bkueng

Nice, this is much more readable!

bkueng · 2018-03-01T17:39:45Z

src/modules/vtol_att_control/tailsitter.cpp

-		_mc_pitch_weight = 1.0f * (float)hrt_elapsed_time(&_vtol_schedule.transition_start) /
-				   (_params_tailsitter.back_trans_dur * 1000000.0f);
+		// smoothly move control weight to MC
+		_mc_roll_weight = _mc_pitch_weight = 1.0f * time_since_trans_start / _params->back_trans_duration;


1.0f * can be dropped

bkueng · 2018-03-01T17:43:29Z

src/modules/vtol_att_control/tiltrotor.cpp

-					  (_params_tiltrotor.airspeed_trans - _params_tiltrotor.airspeed_blend_start);
+		if (!_params->airspeed_disabled && _airspeed->indicated_airspeed_m_s >= _params->airspeed_blend) {
+			_mc_roll_weight = 1.0f - (_airspeed->indicated_airspeed_m_s - _params->airspeed_blend) /
+					  (_params->transition_airspeed - _params->airspeed_blend);


Did you do that already?

bkueng · 2018-03-01T17:47:26Z

posix-configs/SITL/init/ekf2/standard_vtol

@@ -57,7 +57,7 @@ param set SYS_AUTOSTART 13006
 param set SYS_MC_EST_GROUP 2
 param set SYS_RESTART_TYPE 2
 param set VT_MOT_COUNT 4
-param set VT_TRANS_THR 0.75
+param set VT_F_TRANS_THR 0.75


Do we need some logic to automatically update existing values from VT_TRANS_THR to VT_F_TRANS_THR for people who have set VT_TRANS_THR?

This change can cause crashes on airspeedsensorless vtols
Can we default to 100% ?

@sanderux I defaulted the param to 100%

bkueng · 2018-03-01T18:03:27Z

src/modules/vtol_att_control/standard.cpp

-					    ((_params_standard.front_trans_time_min / 2.0f) * 1000000.0f));
+		} else if (_params->airspeed_disabled) {
+			mc_weight = 1.0f - (time_since_trans_start -  _params->front_trans_time_min) / _params->front_trans_time_min;
+			mc_weight = math::constrain(2.0f * mc_weight, 0.0f, 1.0f);


If I see this correct, this changes the logic:

m := _params_standard.front_trans_time_min t : = time_since_trans_start previous: if t < m && t > m/2: w = 1 - (t - m/2)/(m/2) = 1 - t/m*2 + 1 = 2 - 2 * t/m new: w = 1 - (t-m)/m = 1 - t/m + 1 = 2 - t/m w = constrain(2 * w, 0, 1) --> 4 - 2*t/m

Is that the intention?

#8967 (comment)

sanderux · 2018-03-01T18:26:28Z

src/modules/vtol_att_control/standard.cpp


 	/* duration of a forwards transition to fw mode */
-	param_get(_params_handles_standard.front_trans_dur, &v);
-	_params_standard.front_trans_dur = math::constrain(v, 0.0f, 20.0f);


I agree to remove the constrain as UI will make clear not to go beyond that

Trying to correct my review

RomanBapst · 2018-03-05T17:00:48Z

@PX4/testflights Could you please test this on a standard VTOL?

r0gelion · 2018-03-05T22:36:57Z

@RomanBapst we're going to the flying field tomorrow and we will test this PR.

santiago3dr · 2018-03-06T22:08:26Z

flight on standard delta-quad running pixhawk 1 (v2):
https://logs.px4.io/plot_app?log=d3b71a4f-aa03-44fe-8afb-a6d65ec13240

compared to current master:
https://logs.px4.io/plot_app?log=a0fe8530-da7f-4f21-ae4c-4fdeaa9b5658

good flight performance; nothing out of the ordinary

…_elapsed time - standard vtol was implementing many custom parameters although they are generic and should be shared between the vtol types - removed heavy usage of hrt_elapsed_time() which is a system call and could be computationally expensive

Signed-off-by: Roman <[email protected]>

…sing microseconds - seconds is more intuitive - avoids tons of divisions by 1e6f Signed-off-by: Roman <[email protected]>

- do not call hrt_elapsed_time since it's expensive - remove P2 front transition phase (was not even used) Signed-off-by: Roman <[email protected]>

- more intuitive - avoids tons of divisions Signed-off-by: Roman <[email protected]>

calculation Signed-off-by: Roman <[email protected]>

Signed-off-by: Roman <[email protected]>

…ion airspeed Signed-off-by: Roman <[email protected]>

Signed-off-by: Roman <[email protected]>

- the main idea is to prevent airspeed less systems to stall after a transition Signed-off-by: Roman <[email protected]>

dagar reviewed Feb 26, 2018

View reviewed changes

RomanBapst commented Feb 27, 2018

View reviewed changes

RomanBapst mentioned this pull request Feb 27, 2018

Pr tiltrotor param cleanup #8974

Merged

RomanBapst force-pushed the pr-standard-param-cleanup branch from 9950dcb to 71c3726 Compare February 27, 2018 14:37

This was referenced Feb 27, 2018

tailsitter: removed usage of hrt_elapsed and small cleanup #8977

Closed

tailsitter: removed common vtol parameters #8976

Closed

RomanBapst force-pushed the pr-standard-param-cleanup branch from b6eaac0 to e7a8d50 Compare February 27, 2018 15:29

RomanBapst force-pushed the pr-standard-param-cleanup branch from e7a8d50 to f015809 Compare February 28, 2018 08:57

RomanBapst commented Feb 28, 2018

View reviewed changes

dagar reviewed Feb 28, 2018

View reviewed changes

RomanBapst force-pushed the pr-standard-param-cleanup branch 2 times, most recently from 15940a3 to 21e6e35 Compare March 1, 2018 08:19

RomanBapst changed the title ~~[WIP] Pr standard param cleanup~~ VTOL parameter cleanup Mar 1, 2018

bkueng reviewed Mar 1, 2018

View reviewed changes

sanderux requested changes Mar 1, 2018

View reviewed changes

sanderux previously approved these changes Mar 1, 2018

View reviewed changes

RomanBapst force-pushed the pr-standard-param-cleanup branch from 21e6e35 to 390b2fc Compare March 2, 2018 14:15

RomanBapst added 16 commits March 7, 2018 11:14

ROMFS: updated VT_TRANS_THR parameter name change

8710c9d

Signed-off-by: Roman <[email protected]>

sitl configs: updated VT_TRANS_THR parameter name change

0716a3e

Signed-off-by: Roman <[email protected]>

vtol_att_control: use airspeed_disabled flag instead of airspeed mode

4cad515

Signed-off-by: Roman <[email protected]>

tiltrotor: removed common vtol parameters

bc23fef

Signed-off-by: Roman <[email protected]>

sitl tiltrotor config: front transition throttle parameter name change

364310a

Signed-off-by: Roman <[email protected]>

tailsitter: removed common vtol parameters

38d5330

Signed-off-by: Roman <[email protected]>

standard vtol: use time in second since transition start instead of u…

2906a86

…sing microseconds - seconds is more intuitive - avoids tons of divisions by 1e6f Signed-off-by: Roman <[email protected]>

tailsitter: removed usage of hrt_elapsed and small cleanup

4f492f2

- do not call hrt_elapsed_time since it's expensive - remove P2 front transition phase (was not even used) Signed-off-by: Roman <[email protected]>

tailsitter: represent time since transition in seconds

c704e80

- more intuitive - avoids tons of divisions Signed-off-by: Roman <[email protected]>

tiltrotor: represent time since transition in seconds

de13540

- more intuitive - avoids tons of divisions Signed-off-by: Roman <[email protected]>

vtol_att_control: switch from division to multiplication for variable

78a8db7

calculation Signed-off-by: Roman <[email protected]>

tailsitter: avoid doing same calculation twice

8014671

Signed-off-by: Roman <[email protected]>

standard vtol: simplify blending weight calculation

c9eec01

Signed-off-by: Roman <[email protected]>

standard vtol: fix mc weight calculation

5f4de0d

Signed-off-by: Roman <[email protected]>

vtol_att_control: make sure blending airspeed is smaller than transit…

681b6ce

…ion airspeed Signed-off-by: Roman <[email protected]>

tailsitter.cpp: small cosmetic fix

0aa0869

Signed-off-by: Roman <[email protected]>

RomanBapst force-pushed the pr-standard-param-cleanup branch from 6962fa5 to 0aa0869 Compare March 7, 2018 10:23

vtol_att_control: default front transition throttle to 100%

28b9e8e

- the main idea is to prevent airspeed less systems to stall after a transition Signed-off-by: Roman <[email protected]>

RomanBapst merged commit ac2b3cc into master Mar 7, 2018

RomanBapst deleted the pr-standard-param-cleanup branch March 7, 2018 13:01

RomanBapst mentioned this pull request Mar 7, 2018

vtol_att_control parameter consolidation #8965

Closed

mrpollo mentioned this pull request Jun 5, 2018

v1.8 Changelog scratchpad #9593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VTOL parameter cleanup #8967

VTOL parameter cleanup #8967

RomanBapst commented Feb 26, 2018 •

edited

Loading

dagar Feb 26, 2018

RomanBapst Feb 27, 2018

dagar Feb 27, 2018

RomanBapst Feb 27, 2018

dagar Feb 27, 2018

sanderux Mar 1, 2018

RomanBapst Feb 27, 2018

RomanBapst commented Feb 28, 2018

RomanBapst Feb 28, 2018

bkueng Mar 1, 2018

RomanBapst Mar 2, 2018

dagar Feb 28, 2018

dagar Feb 28, 2018

RomanBapst Feb 28, 2018

dagar Feb 28, 2018

dagar commented Feb 28, 2018

RomanBapst commented Mar 1, 2018

RomanBapst commented Mar 1, 2018

dagar commented Mar 1, 2018

bkueng left a comment

bkueng Mar 1, 2018

bkueng Mar 1, 2018

bkueng Mar 1, 2018

sanderux Mar 1, 2018

RomanBapst Mar 7, 2018

bkueng Mar 1, 2018

dagar Mar 1, 2018

sanderux Mar 1, 2018

RomanBapst commented Mar 5, 2018

r0gelion commented Mar 5, 2018

santiago3dr commented Mar 6, 2018

VTOL parameter cleanup #8967

VTOL parameter cleanup #8967

Conversation

RomanBapst commented Feb 26, 2018 • edited Loading

SITL testing:

Standard VTOL Transitions

Tiltrotor Transitions

Tailsitter Transitions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanBapst commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Feb 28, 2018

RomanBapst commented Mar 1, 2018

RomanBapst commented Mar 1, 2018

dagar commented Mar 1, 2018

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanBapst commented Mar 5, 2018

r0gelion commented Mar 5, 2018

santiago3dr commented Mar 6, 2018

RomanBapst commented Feb 26, 2018 •

edited

Loading