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

ardupilot: added NAV_SCRIPT_TIME mission item #228

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

tridge
Copy link
Collaborator

@tridge tridge commented Oct 25, 2021

this allows for scripts that execute arbitrary navigation commands
before handing back control

link to related flight code PR: ArduPilot/ardupilot#19039

Copy link

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

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

In many ways this seems the same as NAV_GUIDED_ENABLE so as discussed on the main flight code PR I'd like to discuss why we need both before this is merged.

@tridge
Copy link
Collaborator Author

tridge commented Oct 26, 2021

In many ways this seems the same as NAV_GUIDED_ENABLE

there are key differences:

  • NAV_GUIDED_ENABLE doesn't have any way for the offboard or script control to hand back to continue the mission. Doing a "set waypoint" to continue is full or race conditions
  • there NAV_GUIDED_ENABLE takes no parameters. I want the script to know which command is wanted. So the mission can do a loop, then a roll, then an immelman turn, then a spin, then a figure 8 etc, all with normal waypoints to get lined up for the aerobatic manoeuvrer in between

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.

I don't think the timeout is useful.

It is too linked to the other feilds. For example, I use a script to re-implement loiter turns. I use arg 1 as a radius and arg 2 as a number of turns. I then have to go and workout how long that should take in order to set the the timeout correctly.

A timeout offers no protection against the script crashing. The vehicle will just carry on doing whatever the last thing it was asked to until the timer runs out. If the expected maneuver is more than a few 10's of seconds the timout results in a large distance the vehicle could potentially cover before being 'saved' by the timeout.

Instead I suggest using the existing guided functionality. The user could optionally use MAV_CMD_DO_GUIDED_LIMITS to set limits, these could either be set once at the start or several times throughout the mission. This gives possibility both a timeout and a geo-fence from the start of the command.

We implement GUID_TIMEOUT param on plane. Rather than a time out for the whole maneuver this is a timeout on the guided command callback. This means you timeout in the case that the script dies. For scripting guided in the past I have set this to 0.1 seconds with the script running at 100hz. This gives a very fast recovery from a script crash.

@IamPete1
Copy link
Member

IamPete1 commented Nov 1, 2021

  • NAV_GUIDED_ENABLE doesn't have any way for the offboard or script control to hand back to continue the mission. Doing a "set waypoint" to continue is full or race conditions

This command does not give any method to hand back to mission control either. It has a timeout, but NAV_GUIDED_ENABLE has that too vis MAV_CMD_DO_GUIDED_LIMITS. I suggest a new method to allow both this new scripting command and NAV_GUIDED_ENABLE to allow the item to be marked as complete and the mission continued.

Maybe a command long, NAV_GUIDED_COMPLETE a companion computer could send it over mavlink and scripting could bypass the message and set the bit it would flip once support has been added to mission.

@rmackay9
Copy link

rmackay9 commented Nov 1, 2021

@IamPete1,

I generally agree with your comments but looking at the flight code PR the "nav_scripting_update()" allows the script to return a "done" boolean.

That same method also allows the script to return other controls but this is where you and I agree that instead the script should use the existing controls:

singleton AP_Vehicle method set_target_location boolean Location
singleton AP_Vehicle method get_target_location boolean Location'Null
singleton AP_Vehicle method set_target_pos_NED boolean Vector3f boolean float -360 +360 boolean float'skip_check boolean boolean
singleton AP_Vehicle method set_target_posvel_NED boolean Vector3f Vector3f
singleton AP_Vehicle method set_target_posvelaccel_NED boolean Vector3f Vector3f Vector3f boolean float -360 +360 boolean float'skip_check boolean
singleton AP_Vehicle method set_target_velaccel_NED boolean Vector3f Vector3f boolean float -360 +360 boolean float'skip_check boolean
singleton AP_Vehicle method set_target_velocity_NED boolean Vector3f
singleton AP_Vehicle method set_target_angle_and_climbrate boolean float -180 180 float -90 90 float -360 360 float'skip_check boolean float'skip_check

@IamPete1
Copy link
Member

IamPete1 commented Nov 1, 2021

I generally agree with your comments but looking at the flight code PR the "nav_scripting_update()" allows the script to return a "done" boolean.

Right, but that is specific to the implementation. It is not a factor of the proposed mavlink message. We could just as easily implement that method for NAV_GUIDED_ENABLE

@rmackay9
Copy link

rmackay9 commented Nov 1, 2021

I generally agree with your comments but looking at the flight code PR the "nav_scripting_update()" allows the script to return a "done" boolean.

Right, but that is specific to the implementation. It is not a factor of the proposed mavlink message. We could just as easily implement that method for NAV_GUIDED_ENABLE

I suppose but how would we know if the mission command is to be sent to an offboard computer or a script? .. or maybe you're suggesting that the vehicle code doesn't need to know?

It's separate from this PR but if we were to implement NAV_GUIDED_COMPLETE we would need to send a dynamic magic number to the offboard computer within the NAV_GUIDED_ENABLE command and then have the offboard computer return that magic nuimber in the NAV_GUIDED_COMPLETE reply to avoid race conditions.

For me, I don't mind adding a new set of NAV_SCRIPT_xxx mission commands but I think the implementation should be closer to the NAV_GUIDED_ENABLE feature than this PR is.

@IamPete1
Copy link
Member

IamPete1 commented Nov 1, 2021

It's separate from this PR but if we were to implement NAV_GUIDED_COMPLETE we would need to send a dynamic magic number to the offboard computer within the NAV_GUIDED_ENABLE command and then have the offboard computer return that magic nuimber in the NAV_GUIDED_COMPLETE reply to avoid race conditions.

I don't think that is that use-full unless you also start gate keeping that all the guided commands also come from the same place.

For the moment anyway, I think we have to assume that there is only one thing sending guided commands at a time, either scripting or companion.

this allows for scripts that execute arbitrary navigation commands
before handing back control
@tridge tridge changed the title ardupilot: added NAV_SCRIPTING mission item ardupilot: added NAV_SCRIPT_TIME mission item Nov 5, 2021
@tridge
Copy link
Collaborator Author

tridge commented Nov 5, 2021

I discussed with @rmackay9 and agreed on NAV_SCRIPT_TIME name and fields

@tridge
Copy link
Collaborator Author

tridge commented Nov 5, 2021

It is too linked to the other feilds. For example, I use a script to re-implement loiter turns. I use arg 1 as a radius and arg 2 as a number of turns. I then have to go and workout how long that should take in order to set the the timeout correctly.

using this in practice the timeout has been useful. You can also set it to zero for no timeout. For distance limits I want to use the geofence, or pilot takeover

@tridge tridge merged commit 6bcbd85 into ArduPilot:master Nov 5, 2021
@tridge tridge deleted the pr-nav-scripting branch November 5, 2021 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants