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

AP_Scripting/AP_Vehicle/Copter/Rover: add set_desired_speed for use in scripting #21115

Merged
merged 5 commits into from Jul 7, 2022

Conversation

yuri-rage
Copy link
Contributor

@yuri-rage yuri-rage commented Jul 4, 2022

Adds a Lua binding for set_desired_speed(float) to Copter and Rover.

Allows scripting to update the nav speed in all autopilot assisted modes.

Also includes an extremely minor update to the rover-MinFixType.lua example, per Pete's request (was not incorporated in the initial commit last week).

bool Copter::set_desired_speed(float speed)
{
// exit if vehicle is not in auto mode
if (flightmode != &mode_auto) {
Copy link
Contributor

@rmackay9 rmackay9 Jul 5, 2022

Choose a reason for hiding this comment

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

I think we could use the is_autopilot() method here which would allow it to work in RTL, SmartRTL, etc as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at doing so and generally agree, but I was not confident that wp_nav->set_speed_xy(speed * 100.0f) would function predictably if the mode was not AUTO. If I was simply being overly conservative, then I'm happy to make the change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made per request. Rebase/squashed commit containing Copter.cpp

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 5, 2022

In general this looks good to me and I think we could merge it if the above suggested change is made. thanks!

@yuri-rage
Copy link
Contributor Author

@rmackay9, I'd also like to kindly request a backport to 4.2-stable. Particularly for Rover users, I think there will be some hesitance to upgrade to 4.3 with the major nav controller changes, so I anticipate a lot of "hangers on" to 4.2 for a while. I know several of us have an immediate use case for the set_desired_speed() binding.

local SBY_INTERVAL_MS = 500 -- slower interval when detection is disabled

-- tuning values
local SCR_USER_SPEED_PARAM = 'SCR_USER1' -- (m/s) set rough terrain speed here
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to load a custom param name. It would be good to be able to remove the scirpting user params at some point.

Copy link
Contributor Author

@yuri-rage yuri-rage Jul 5, 2022

Choose a reason for hiding this comment

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

Per Discord discussion, will make this change.

Copy link
Contributor Author

@yuri-rage yuri-rage Jul 5, 2022

Choose a reason for hiding this comment

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

@IamPete1, custom param set added per discussion. Might be a good example to point folks toward when discussing scripting params, since I think I did a pretty decent job of writing a short function that takes care of it.

Also added Lua docs to AP_Scripting commit.

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Looks good. Needs lua docs.

I wonder if we should accept speed changes in any mode? For example I want to start my mission at speed X. Currently the script would have to keep trying until the request was accepted.

MAVLink do_change_speed works in any mode.

@yuri-rage
Copy link
Contributor Author

yuri-rage commented Jul 5, 2022

I wonder if we should accept speed changes in any mode? For example I want to start my mission at speed X. Currently the script would have to keep trying until the request was accepted.

MAVLink do_change_speed works in any mode.

@rmackay9, would it be acceptable to simply remove the is_autopilot() / is_autopilot_mode() checks to accomplish this intent?

@yuri-rage yuri-rage force-pushed the Lua-do_change_speed-binding branch from baf654e to 5ed5910 Compare July 5, 2022 22:38
@rmackay9
Copy link
Contributor

rmackay9 commented Jul 5, 2022

@yuri-rage, yes, I think it's ok to remove the is_autopilot() and is_autopilot_mode() checks. It should do no harm to set the value even if AR_WPNav or AC_WPNav aren't being used.

@yuri-rage
Copy link
Contributor Author

yuri-rage commented Jul 6, 2022

yes, I think it's ok to remove the is_autopilot() and is_autopilot_mode() checks. It should do no harm to set the value even if AR_WPNav or AC_WPNav aren't being used.

@rmackay9, in the case of Rover, I'm not sure we'd fully accomplish @IamPete1's intent if we just remove the check. Should we instead default to mode_auto->set_desired_speed(speed) if is_autopilot_mode() returns false?

EDIT: see my next comment - behavior is not impacted by these changes.

@yuri-rage yuri-rage force-pushed the Lua-do_change_speed-binding branch from 5ed5910 to e7750cb Compare July 6, 2022 15:50
@yuri-rage
Copy link
Contributor Author

@rmackay9, @IamPete1,

Removing the mode checks does not result in the DO_CHANGE_SPEED behavior that Pete describes (starting an auto mission at the requested speed after set_desired_speed() is called from any other mode). Instead, the vehicle uses cruise/waypoint speed from the params (even if mode_auto.set_desired_speed() is explicitly called in the method).

For Rover, I have removed the conditional mode check statement anyway, since control_mode->set_desired_speed() returns false for non-autopilot modes, which is likely more efficient than evaluating the condition.

For Copter, recommend no changes to the implementation as written.

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 7, 2022

@IamPete1 you happy with this? If "yes" maybe put an approved on it?

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@rmackay9 rmackay9 merged commit 565f757 into ArduPilot:master Jul 7, 2022
@rmackay9
Copy link
Contributor

rmackay9 commented Jul 7, 2022

Merged, thanks!

@yuri-rage
Copy link
Contributor Author

Excellent! Thank you!

@rmackay9, not sure if you saw my request for a backport to 4.2, but that would be extremely helpful. Please let me know what I can do to help!

@rmackay9 rmackay9 added this to Pending in Copter 4.2 Jul 18, 2022
@rmackay9
Copy link
Contributor

This has been included in Copter/Rover-4.2.3 except for the last commit, "AP_Scripting: rover-MinFixType example param caching fix" because of a merge conflict.

@rmackay9 rmackay9 moved this from Pending to Copter-4.2.3-rc1 in Copter 4.2 Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Copter 4.2
Copter-4.2.3-rc1
Plane 4.2
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants