Skip to content
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

Copter: fix RTL_SPEED from affecting vehicle speed in Auto #10331

Merged
merged 9 commits into from
Jan 31, 2019

Conversation

rmackay9
Copy link
Contributor

@rmackay9 rmackay9 commented Jan 24, 2019

This is an alternative solution to this PR which solves this issue.

The issue is that with the proper sequence of mode changes, the RTL_SPEED could be used in Auto mode. This is how to reproduce the issue:

  • set RTL_SPEED = 200
  • create a mission with at least one waypoint
  • arm the vehicle and switch to Auto mode (the vehicle will fly at WPNAV_SPEED)
  • switch to RTL mode (the vehicle will slow to the RTL_SPEED)
  • switch back to Auto, the vehicle will incorrectly fly at RTL_SPEED instead of WPNAV_SPEED

The PR resolves the issue by

  • AC_WPNav::set_speed_xy is changed so it does not update the _wp_speed_cms variable (which is the WPNAV_SPEED parameter). Instead the target speed is only passed to AC_PosControl::set_max_speed_xy.
  • other places in AC_WPNav that used _wp_speed_cms (like the leash calculations) instead go to AC_PosControl to get the current target speed. Essentially AC_PosControl is trusted to hold the current target horizontal speed
  • Similar changes are made to AC_WPNav::set_speed_z() that suffer a similar problem in which a DO_SET_SPEED command (either sent within a COMMAND_LONG or as part of a mission) permanently affected the WPNAV_SPEED_UP and WPNAV_SPEED_DN which meant all autonomous modes would fly at the new vertical speed (the speeds should be reset to their defaults when the mode is changed)

These changes have been tested in SITL. For the horizontal test, a mission was created like below
sitl-test
The vehicle was flown in Auto, switched to RTL, then back to Auto. In master we see the vehicle flies at 2m/s the 2nd time it enters Auto (it should fly at 8m/s)
horizontal-master

Below is the after view and we can see the vehicle flies at full speed after entering Auto the 2nd time.
horizontal-after


To test the climb rate issue a vertical mission was created in SITL like below:
sitl-vert-mission

Below are before and after pics of the climb rate:
vert-before-graph
vert-after-graph

@rmackay9 rmackay9 requested a review from khancyr January 24, 2019 07:21
@rmackay9 rmackay9 changed the title Copter: Copter: fix RTL_SPEED from affecting vehicle speed in Auto Jan 24, 2019
Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on SITL and approved!

@khancyr khancyr mentioned this pull request Jan 24, 2019
@rmackay9
Copy link
Contributor Author

@khancyr, great, thanks very much! I'll give it another 12hrs in case any one else wants to comment and then I'll merge. txs again.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed an autotest onto this which fails on master but passes here.

@rmackay9
Copy link
Contributor Author

@peterbarker, cool, thanks!

@rmackay9
Copy link
Contributor Author

.. hmm.. it seems to keep failing travis but it's not clear to me from looking at the travis output why it's failing..

@peterbarker
Copy link
Contributor

peterbarker commented Jan 31, 2019 via email

@rmackay9
Copy link
Contributor Author

@peterbarker, thanks!

@rmackay9 rmackay9 merged commit 44ff214 into ArduPilot:master Jan 31, 2019
@rmackay9 rmackay9 deleted the azuredrones-rtl-speed branch January 31, 2019 10:34
@rmackay9 rmackay9 removed this from PRs in Copter 3.6 Backports Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants