Scripting: added command_int API for accessing MAV_CMD_xxx

[tridge]

This makes it possible to call most MAV_CMD_xxx calls from lua scripts, gaining us a lot of functionality with a small amount of flash
This was prompted by #27223 and the MAV_CMD_GUIDED_xxx commands, but is really very broadly useful

some questions:

• should we return the full MAV_RESULT enum? Is it useful?
• should we create a dedicated GCS_MAVLink channel for lua?

this also adds an example that tests DO_SET_MODE and DO_REPOSITION

[peterbarker]

* should we return the full MAV_RESULT enum? Is it useful?

It'd probably only be useful during script development - but it could be very useful there. Most of the time the script probably couldn't do anything useful with the more-specific result code.

* should we create a dedicated GCS_MAVLink channel for lua?

Probably not for now.... only problem i can think of is commands that have side-effects on the channel e.g. setting stream rates.

this also adds an example that tests DO_SET_MODE and DO_REPOSITION

The interface would be much, *much nicer with optional parameters, in the same way that run_cmd_int works in autotest. Some of those look like this: self.run_cmd_int(COMMAND_NUM, p1=xyzzy) which is nice.

I wonder if the lack of target component might cause problems somewhere. I can't think of anywhere that might matter, however.

[tridge]

@peterbarker did you read the example? It does have optional parameters

[IamPete1]

This #27219 allows this and lots more, but it will be a little more complex to deal with.

[tpwrules]

I think this introduces locking issues? Ordinarily commands are processed starting at the GCS's update_receive method from the main scheduler. But as I understand Lua runs in a different thread, so it could enter the command handler while received data is being processed and commands are being handled.

[tridge]

I think this introduces locking issues? Ordinarily commands are processed starting at the GCS's update_receive method from the main scheduler.

good catch, I've added a GCS mutex

[tridge]

This #27219 allows this and lots more, but it will be a little more complex to deal with.

a little more complex? It would be horribly complex to configure then construct mavlink messages, then parse the reply to work out if it succeeded. How would you even do it as a synchonous lua call? You'd need to wait for GCS processing by the main thread then wait for the reply, which means adding a state machine in the lua code.

[IamPete1]

a little more complex? It would be horribly complex to configure then construct mavlink messages, then parse the reply to work out if it succeeded. How would you even do it as a synchonous lua call? You'd need to wait for GCS processing by the main thread then wait for the reply, which means adding a state machine in the lua code.

I would hope we would update the examples to do the tricky stuff. The synchronous issue is still a problem for this PR, although in a much smaller way when you get a MAV_RESULT_IN_PROGRESS, how do you get the final result in that case?

I guess my point is really this: Are we going to add helpers for every MAVLink message that might be useful for a script to send to the vehicle? I agree that the commands do tick off lots of them, but going the direct MAVLink way gets everything. Not to mention all the messages going the other way that give information we may not have bindings for.

[tpwrules]

I think also such a state machine could be nicely encapsulated and appear synchronous to the user if coroutines made a return.

[tridge]

Are we going to add helpers for every MAVLink message that might be useful for a script to send to the vehicle?

with command_int we get access to a huge pile of functions with one API

I would hope we would update the examples to do the tricky stuff

it is still going to be:

• slow (waiting for main thread to process commands)
• much more complex to setup for user

when you get a MAV_RESULT_IN_PROGRESS, how do you get the final result in that case?

that is indeed tricky, but commands that do that are very rare (only one that comes to mind is airspeed cal), and as far as I know the final result is always a pass?

There certainly are some MAV_CMD_xxx that won't make sense. For example, the one that triggers sending of MAVCAN makes no sense. It will work, but will just cause the pkts to come out on USB

[timtuxworth]

I've tested this in SITL as a replacement for update_target_location() for plane follow.

[davidbuzz]

+1 to merge this.

[peterbarker]

I'm not sure if this is insufficent protection or too blunt.

It's too blunt if you're protecting against mavlink-command-ints being run at the same time - that should go within handle_command_int.

It might not be sufficient protection depending on where the main thread is up to when the script command is executed. Remembering, of course, that we process mavlink while we're sleeping (at least that's only likely when we're disarmed....)

[tridge]

I presume you mean in AP_Vehicle::scheduler_delay_callback() ? that calls gcs().update_receive() which is exactly the call the mutex in this PR protects.

[tpwrules]

Upon further thought I agree that this is insufficient locking. Consider, for example AP_Gripper_Servo::release. It can be called through GCS_MAVLINK::handle_command_do_gripper which is called by update_receive and is protected by that new lock. But on the same object AP_Gripper_Servo::grab can be called through AP_Gripper_Backend::update() which is called by the main scheduler on the main thread too and not protected by any lock.

The scheduler won't run two things at once (IIUC) but if Lua is releasing while the main thread decides to grab, who wins? We need another lock in the gripper methods too. Then we have to start being concerned about locking order and deadlocks, and look at locking in everything that can be done by the command int handler...

This problem for Gripper is already triggerable through the aux function handler and bindings in any case (though adding a generic lock there would be a lot more straightforward, though it would not fix this problem), but I don't think we want it to be worse.

[peterbarker]

I think also such a state machine could be nicely encapsulated and appear synchronous to the user if coroutines made a return.

These commands must run fast as they're processed on the main thread ATM when executed via mavlink input.

You may get handed back an "in progress" result... and that would require some clever handling in LUA...

[tpwrules]

I think also such a state machine could be nicely encapsulated and appear synchronous to the user if coroutines made a return.

These commands must run fast as they're processed on the main thread ATM when executed via mavlink input.

You may get handed back an "in progress" result... and that would require some clever handling in LUA...

I think also such a state machine could be nicely encapsulated and appear synchronous to the user if coroutines made a return.

These commands must run fast as they're processed on the main thread ATM when executed via mavlink input.

You may get handed back an "in progress" result... and that would require some clever handling in LUA...

Like I said this can be done elegantly with coroutines. I think we should study that possibility. It would require more complex scripting for sure and use up valuable scripting memory, but would need no new bindings and create no new problems.

[timtuxworth]

I think also such a state machine could be nicely encapsulated and appear synchronous to the user if coroutines made a return.

These commands must run fast as they're processed on the main thread ATM when executed via mavlink input.

You may get handed back an "in progress" result... and that would require some clever handling in LUA...

As far as I can see "you" (i.e. Lua) will never get an "in progress" result, it seems to be hidden from Lua as lua_GCS_command_int only returns true/false success/fail.

[tridge]

run_command_int instead

[timtuxworth]

For DO_REPOSITION param4 (Yaw) says "Yaw heading. NaN to use the current system yaw heading mode (e.g. yaw towards next waypoint, yaw to home, etc.). For planes indicates loiter direction (0: clockwise, 1: counter clockwise)"

So if I want to not set this value (yaw) and leave it as whatever it was before, should I not pass "Nil" from Lua? except that lua_GCS_command_int doesn't seem to support that.

[tpwrules]

You can generate a NaN in Lua with the expression 0/0. In SITL this appears to be the NaN encoded as 0xFFC00000.

[timtuxworth]

I'm trying to use DO_REPOSITION to set the vehicle target lat/lng and altitude. The vehicle successfully heads to the new target lat/lng but appears to ignore the altitude. This is the wrapper I use to call it, passing in target.alt as m.

local function set_vehicle_target_location(target)
   if not gcs:command_int(MAV_FRAME.GLOBAL, MAV_CMD_INT.DO_REPOSITION, { target.groundspeed or -1, target.options or 0, target.radius or 2, target.yaw or (0/0),
                           x = target.lat, y = target.lng, z = target.alt }) then
      gcs:send_text(MAV_SEVERITY.ERROR, SCRIPT_NAME_SHORT .. ": MAVLink DO_REPOSITION returned false")
   end
end

If I use vehicle:update_target_location(current_target_location, new_target_location) (with the new altitude in new_target_location) it works fine.

It seems like DO_REPOSITION calls plane.set_guided_WP(requested_position)
which in turn calls set_target_altitude_current(); which seems like it means DO_REPOSITION ignores the altitude parameter. I'm probably missing something, happy to be corrected.

here's a log if it helps https://www.dropbox.com/scl/fi/ze5688x4289k7vem91vs2/00000308.BIN?rlkey=pp2c156damrj0h3guldnbjjow&dl=0

[timtuxworth]

My plane follow script uses do_reposition to set the target lat/lng/alt to the location returned by AP_Follow, but it it gets too close to the point, it switches to GUIDED_CHANGE_HEADING to maintain heading alongside the target plane.

GUIDED_CHANGE_HEADING sets plane.guided_state.target_heading_type, to either COG or HEADING but there is no way to change it back to NONE, (mavlink doesn't seem to support setting it back to NONE) so once the script has set heading, it can't revert to position mode without switching out of GUIDED and back again.

I found I could get this to work by adding one line in handle_command_int_do_reposition() right before the plane.set_mode() at line 844

plane.guided_state.target_heading_type = GUIDED_HEADING_NONE;

[tridge]

You can generate a NaN in Lua with the expression 0/0

yep, I've added NaN() to the example and used it for yaw

[tridge]

This is the wrapper I use to call it, passing in target.alt as m.

please update to the current PR, this is the old syntax

[timtuxworth]

please update to the current PR, this is the old syntax

I love the new syntax - very clean.

[tridge]

GUIDED_CHANGE_HEADING sets plane.guided_state.target_heading_type, to either COG or HEADING but there is no way to change it back to NONE

as a separate PR we should add GUIDED_HEADING_DEFAULT to the enum in the HEADING_TYPE enum in the mavlink XML and then allow setting of that type in GUIDED_CHANGE_HEADING.

[peterbarker]

LGTM

[timtuxworth]

GUIDED_CHANGE_HEADING sets plane.guided_state.target_heading_type, to either COG or HEADING but there is no way to change it back to NONE

as a separate PR we should add GUIDED_HEADING_DEFAULT to the enum in the HEADING_TYPE enum in the mavlink XML and then allow setting of that type in GUIDED_CHANGE_HEADING.

That seems to require a PR in the mavlink repo am I right?
Is this right? mavlink/mavlink#2130

Here's the ArduPIlot PR #27428