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

common: add MAV_CMD to process custom actions on a waypoint #1516

Closed
wants to merge 2 commits into from

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Oct 20, 2020

A custom action on a waypoint is an action that is determined by a system processing a navigation behavior which metadata is stored and processed in that system in specific. An example of it can be a planner in the companion computer that processes a simple action as triggering a servo, or a set of commands, split to be executed in a specific time frame, which then result in a more complex action.

The usage of this command should be integrated into an extension of the mission microservice as a way of triggering actions that are not explicitly specified in the Mavlink spec, but rather a sequence of specific MAV_CMDs being triggered or even other associated to other complex non-linear decision making processes. The premise here is that whatever action this command triggers, the metadata associated with that action (example: if param 1 is set to 1, that triggers action 1 which is basically rotating a servo 45 deg at a fixed interval), is stored on the end system and it is accessible by the process that is going to execute (or even plan) the action execution. The format of that metadata, how is stored and how it is accessed is an implementation decision of the developer.

@TSC21
Copy link
Member Author

TSC21 commented Oct 20, 2020

@julianoes @jkflying

message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
<entry value="2700" name="MAV_CMD_CUSTOM_ACTION" hasLocation="true" isDestination="true">
<description>Custom action to execute at this specific waypoint. The definition of the action, or set of actions, to execute and associated metadata is stored in the system that is going to process the action. By default, the Ground Station will show the Vehicle as flying through this item, but an extension of the microservice might allow to bring the action metadata to the Ground Station for propper feedback.</description>
<param index="1" label="Action" minValue="0" increment="1">Number of the custom action that is goung to be executed. The metadata associated to this action is stored on the end system that is going to process this command, in order to understand what action should be trigger and what are the specifics regarding that action.</param>
<param index="2" label="Execution control" minValue="0" increment="1" maxValue="2">Used to control the way the navigation process is continued and how it gets to the next waypoint after the execution of the action is terminated. (0: using the timeout value for receiving the ACK for the execution termination; 1: using the same field value as a maximum execution time, i.e., even if the ACK is received, the navigation module will just move to the next waypoint after that amount of time as passed since the command was sent; 2: blocks the navigation while the execution termination ACK is not received).</param>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please simplify this, I don't understand it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per Julian. This description seems to target a specific implementation or concept.

Copy link
Member Author

Choose a reason for hiding this comment

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

@julianoes @auturgy I have reword this. Can you check if it makes more sense. It's not about the implementation itself but about the extension of the mission microrservice that we are trying to achieve.

If it makes more sense to break this into a separate param (since now we have param 4-7 free), please suggest it so I bring it in. Thanks!

message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
message_definitions/v1.0/common.xml Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator

What is the specific use case that is driving this request?

My concern with this is that it is very platform-specific behaviour and not very amenable to generic UIs in a GCS. Generally we try to standardise an interface for common behaviour - which is why things like set_gripper are preferred over "set servo".

@auturgy @julianoes Do you see this as a problem or do you think this would be a useful command once refined?

FYI @TSC21 MAVLink are discussing moving towards a model where we will be managing a "standard.xml" (implemented in two flight stacks) and things that are only of interest to one stack go into its dialect. We're in an in-between state, but what this means is that we want strong buy in from stakeholders before we add things.
Even so, I would expect that this would need WIP tag, and we wouldn't merge until you had an implementation.

@julianoes
Copy link
Collaborator

@hamishwillee I don't see a problem of having something like this that enables a custom action. It's similar to other things that we reserve for custom use.

An alternative would be to reuse https://mavlink.io/en/messages/common.html#MAV_CMD_WAYPOINT_USER_1.

@auturgy
Copy link
Collaborator

auturgy commented Oct 21, 2020

We already have user defined commands, and companion scripts can already execute based on whatever state/condition you want to use as a trigger. My assumption is that you want to move some logic out of a script so that they are command, not condition, triggered, so they are more portable. I see the use case, but will need some convincing that this should be in the standard. To me that use case should either have a specific command, or use the available user commands.
I'll add that ArduPilot is guilty of a similar message though: https://github.com/ArduPilot/mavlink/blob/c9cd33584a6826c21d2c64d8e4d4b2c185dee084/message_definitions/v1.0/ardupilotmega.xml#L277

@peterbarker
Copy link
Contributor

Looks like ArduPilot's MAV_CMD_SCRIPT with some arguments to pass to the script?

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

My concern with this is that it is very platform-specific behaviour and not very amenable to generic UIs in a GCS. Generally we try to standardise an interface for common behaviour - which is why things like set_gripper are preferred over "set servo".

@hamishwillee the idea here is to actually standardize the way we send a command that triggers a specific action on system component responsible for executing that action. That's why I didn't use the USER commands in the first place because they are not standard - you can set whatever you want in the params. The point here is that any GCS can have access and implement this command (which has a standard structure), while the component responsible to execute this command will read the param 1 (the action), and according to stored metadata, it will read the associated configurations for that action and execute them according to the predefined conditions (which can be static or dynamic, again depends of the configuration).

What is not being standardized here is the format of that metadata, how is stored and how it is accessed, since as I said that will be a concern for the implementer. The only thing standard here is the way that the command is handled - what happens after that again depends on the implementation existent on the system component. IMO this is abstract enough to not cause limitations or constrains on the way it is used in any autopilot.

We already have user defined commands, and companion scripts can already execute based on whatever state/condition you want to use as a trigger. My assumption is that you want to move some logic out of a script so that they are command, not condition, triggered, so they are more portable. I see the use case, but will need some convincing that this should be in the standard. To me that use case should either have a specific command, or use the available user commands.
I'll add that ArduPilot is guilty of a similar message though: https://github.com/ArduPilot/mavlink/blob/c9cd33584a6826c21d2c64d8e4d4b2c185dee084/message_definitions/v1.0/ardupilotmega.xml#L277

@auturgy I think I have added enough context on the answer I provided to Hamish above. This is not exactly to trigger a script, but rather to extend the mission microservice in a way that allows, for a specific waypoint, to execute more complex actions (doesn't mean you can't execute simple, it really depends on how you define the metadata), while maintaining flight operations. So this bring a standardized way of handling custom/user waypoints and at the same time does a bit more than triggering a script - it actually might require a sort of handshake on execution completion and might also result on blocking conditions in the navigation routine, depending on how you set param 2.

To add this doesn't target a specific use case: it might actually target many. The point here is to standerdize the way we interface with all those specific use cases (custom actions), without having to specific concrete parameters which are different from action to action.

<wip/>
<description>Custom action to execute at this specific waypoint. The definition of the action, or set of actions, to execute, as well as the associated metadata is stored in the system that is going to process the action.</description>
<param index="1" label="Action" minValue="0" increment="1">Number of the custom action that will be executed. The metadata associated with this action is stored on the system that is going to process this command.</param>
<param index="2" label="Execution control" minValue="0" increment="1" maxValue="2">Control when the Vehicle moves to the next waypoint. 0: Use ACK from component responsible for executing the action, to move to the next Waypoint. Otherwise, move when timeout (param 3) is reached; 1: Waits for an amount of time (param 3) before moving to the next waypoint, even if the ACK is received from the component responsible for executing the action; 2: Wait for the ACK component responsible for executing the action and blocks the navigation (just moves when an ACK is received).</param>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add an enum for this. It makes documenting it easier and leads to a cleaner implementation

Copy link
Member Author

Choose a reason for hiding this comment

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

It looked too short to me to justify an enum, but yes I guess I can do that. Thanks for the feedback @amilcarlucas!

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • What is difference between 0 and 3?
  • Which ACK? Success only? What if fail or in progress or whatever? The problem with an ACK is that typically something has to be sent back very quickly, while you might want to have the thing sitting there for ages, but not as long as your timeout.

Some real examples might help here.

@auturgy
Copy link
Collaborator

auturgy commented Oct 21, 2020

@TSC21 you want to be specifically non specific about specifically handling non specific behaviour? Something here doesn’t make sense.
Show me the reference implementation. This is clearly targeted at a particular implementation/use case, and you’re trying to obfuscate that. You need to be clear, open and honest about what you need to do, and why the existing mechanics don’t allow it.

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

@TSC21 you want to be specifically non specific about specifically handling non specific behaviour? Something here doesn’t make sense.
Show me the reference implementation. This is clearly targeted at a particular implementation/use case, and you’re trying to obfuscate that. You need to be clear, open and honest about what you need to do, and why the existing mechanics don’t allow it.

I am being as transparent as I can be. As I stated, the idea is the definition of a custom action waypoint. The custom action can be something as simple as triggering a servo or something more complex that involves non-linear decision making. The point here is that the abstraction being brought with this command allows to trigger both actions I just referred, without the need to use specific commands for each.

@auturgy
Copy link
Collaborator

auturgy commented Oct 21, 2020

Let’s take this to the dev call - but you haven’t explained why you can’t or won’t use a user command. All this is is a constrained implementation of a user command, and you’re not being open about why.

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

but you haven’t explained why you can’t or won’t use a user command.

Yes I have:

@hamishwillee the idea here is to actually standardize the way we send a command that triggers a specific action on system component responsible for executing that action. That's why I didn't use the USER commands in the first place because they are not standard - you can set whatever you want in the params.

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

All this is is a constrained implementation of a user command, and you’re not being open about why.

This is not a constrained implementation of a user command. It's an higher level abstraction that allows to have a more granular control over the mission interface. I could just have a command that sets the action number in param 1 and nothing else, but then I would lose the granularity of control I wanted in the mission interface.

@auturgy
Copy link
Collaborator

auturgy commented Oct 21, 2020

Sigh. You’re not losing any granularity: you’re just shifting the logic out of the receiver into the sender, and constraining the options available to it. You need to demonstrate how this is to be used, why a user command isn’t more appropriate, and why this shouldn’t live in a dialect. As above let’s talk it through at dev call next week.

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

Sigh. You’re not losing any granularity: you’re just shifting the logic out of the receiver into the sender, and constraining the options available to it. You need to demonstrate how this is to be used, why a user command isn’t more appropriate, and why this shouldn’t live in a dialect. As above let’s talk it through at dev call next week.

I will be working on the use case so to demonstrate the point here. Moving the option of stating how to control the mission pace to the sender looks reasonable in the sense that an architecture where there's a navigation module in the FMU which takes care of the waypoint processing, you might want to do the configuration before he actually forwards the command to the system component responsible for the actions. The other is that I am looking for a command where I can set in a param what is the action to be executed. USER commands just sounds like an approach that serves all, meaning that its usage doesn't result on a clean extension of the mission microservices.

Anyway, again, I will try to surface this in a use case that is visible and upstreamed, so you can also see that I am not trying to obfuscate anything here. Just want to make things cleaner architecturally wise.

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

and why this shouldn’t live in a dialect

Also to note that I am not saying that this shouldn't live in a dialect. It probably does. But we don't have that structure in place yet, at least a px4.xml.

<entry value="2700" name="MAV_CMD_CUSTOM_ACTION" hasLocation="false" isDestination="false">
<wip/>
<description>Custom action to execute at this specific waypoint. The definition of the action, or set of actions, to execute, as well as the associated metadata is stored in the system that is going to process the action.</description>
<param index="1" label="Action" minValue="0" increment="1">Number of the custom action that will be executed. The metadata associated with this action is stored on the system that is going to process this command.</param>

Choose a reason for hiding this comment

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

The action is a MAV_CMD, isn't it? So "Number" is a message id, right? If that's correct, I think that "ID" would make it clearer to me than "Number".

Suggested change
<param index="1" label="Action" minValue="0" increment="1">Number of the custom action that will be executed. The metadata associated with this action is stored on the system that is going to process this command.</param>
<param index="1" label="Action" minValue="0" increment="1">ID of the custom action that will be executed. The metadata associated with this action is stored on the system that is going to process this command.</param>

Copy link
Member Author

Choose a reason for hiding this comment

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

The action is a MAV_CMD, isn't it? So "Number" is a message id, right?

The action is something that gets executed by this MAV_CMD. Can even be another MAV_CMD, or a time split sequence of them, or something else. The number represents the action that will be executed. Maybe ID can still fit better though.

@JonasVautherin
Copy link

JonasVautherin commented Oct 21, 2020

What's not clear to me is the following: once the autopilot reaches the waypoint and realizes that somebody needs to execute an action now (on which the autopilot may or may not wait), how does that somebody know that now is time to execute the action?

Is it an (unreliable) broadcast message sent by the autopilot, and anybody can pick it up and execute the action? Or is it a targeted command sent by the autopilot to the right component, but then how does it know who the right component is?

@peterbarker How does Ardupilot deal with that with MAV_CMD_SCRIPT?

@TSC21
Copy link
Member Author

TSC21 commented Oct 21, 2020

What's not clear to me is the following: once the autopilot reaches the waypoint and realizes that somebody needs to execute an action now (on which the autopilot may or may not wait), how does that somebody know that now is time to execute the action?

Because it receives this same command coming from the autopilot and triggers the execution (can be just another MAV_CMD, or something more complex, it depends on the metadata you have on that "somebody").

Is it an (unreliable) broadcast message sent by the autopilot, and anybody can pick it up and execute the action? Or is it a targeted command sent by the autopilot to the right component, but then how does it know who the right component is?

It should target a component, yes. I was thinking in MAV_COMP_ID_MISSIONPLANNER, but I am not entirely sure that it can be used on something like this.

@auturgy
Copy link
Collaborator

auturgy commented Oct 21, 2020

@JonasVautherin mav_cmd_script is a bit different in that it is targeting ArduPilot’s onboard lua engine: the autopilot itself executes the script (https://ardupilot.org/copter/docs/common-lua-scripts.html).

@hamishwillee
Copy link
Collaborator

It should target a component, yes. I was thinking in MAV_COMP_ID_MISSIONPLANNER, but I am not entirely sure that it can be used on something like this.

@JonasVautherin @TSC21 Depends on whether the component accepting commands is a separate mavlink entity with its own heartbeat. You need this if the "custom action processor" component needs to be separately addressed, needs separate "proof of life". You don't need this if there is only ever going to be one of these in a system and it will always be part of an overall MAV_COMP_ID_MISSIONPLANNER component.

If you do need this then you'll need to have a MAV_TYPE for the type of component and you should have at least one component ID for the type. Note that if you know there will be multiples of these, you'd declare a couple up front.

I don't know what you need, which is why I think we need an implementation to shake down these details before merging.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 21, 2020

@julianoes @TSC21

I don't see a problem of having something like this that enables a custom action. It's similar to other things that we reserve for custom use.

An alternative would be to reuse MAV_CMD_WAYPOINT_USER_1.

The difference for me is that in MAV_CMD_WAYPOINT_USER_1 we're

  • stating how the GCS should represent the behaviour at the waypoint. That needs to be thought about.
  • catering for users who want to do their own thing - ie we're explicitly washing our hands and saying use this in your own scripts, but don't expect any other system to handle it other than as a generic waypoint.

If someone includes this in their mission they are going to expect a GCS that supports it to be able to do sensible things, wheras for USER1 I think we expect nothing (or at least we have stated the expectation in the command). I'm not sure we have thought this through enough yet.

Further, I am wary of this being used as a mechanism to bypass having to standardise behaviour - ie I can't be bothered creating a new waypoint type for a fixed wing landing pattern so I'll just use a custom action. Yet another TUNNEL.

Let's continue to iterate and discuss in dev call. Despite comments above, I do kind of like the idea, just would prefer there was an implementation we could test against and that this was starting in px4.xml.

@julianoes
Copy link
Collaborator

this was starting in px4.xml.

I understand what you mean but at the current moment where we don't have a px4.xml we can't ask for it. So either we have a process and the mechanisms supporting it, or we don't have it. This in-between state is stupid (for a lack of a better word).

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Oct 22, 2020

To me this is exactly what USER commands are for. They are for the vehicle side to implement something totally custom. The flavors of the USER commands tell the GCS how to represent it on a flight map (if at all). It's the whole reason they exist.

This command is mish-mashing together a black box implementation for a fully custom command and some randomly exposed behavior which may or may not be interesting to the custom command implementor: timeouts, acks and so forth. I don't see any reason to expose any of that in the mavlink spec. It's way too specific.

@hamishwillee
Copy link
Collaborator

Note, action from last dev call was for Nuno to rethink this; in particular how to manage cases where the script is just run at the waypoint vs when the vehicle has to wait before moving, and reporting of failure cases. Some talk around process IDs

@stale
Copy link

stale bot commented Jan 10, 2021

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jan 10, 2021
@hamishwillee
Copy link
Collaborator

Anything happening with this or can it be closed?

@stale stale bot removed the stale label Jan 14, 2021
@JonasVautherin
Copy link

@TSC21 is actively working on an implementation, which I think was the missing part to continue the discussion. So I wouldn't close it yet 🙂.

@TSC21
Copy link
Member Author

TSC21 commented Jan 14, 2021

@TSC21 is actively working on an implementation, which I think was the missing part to continue the discussion. So I wouldn't close it yet 🙂.

Yep correct. Thanks for the heads-up here @JonasVautherin!

@hamishwillee
Copy link
Collaborator

@TSC21 (& @JonasVautherin) I'm going to close this. Open too long without action.

Feel free to reopen with a PR to push this into development.xml (this is our new model for things we intend to develop and have broad support for). At that point ping me and we'll make sure everyone is still on board with the proposed approach.

@julianoes
Copy link
Collaborator

@auturgy I was just thinking about custom actions again, and then looked closer at the ArduPilot scripting.
It looks like the new command introduced in ArduPilot#228 is pretty similar to what was suggested here.

Do you think it would make sense to move the scripting commands into common and use them for scripts that can run on a companion, e.g. using MAVSDK (which is where my use case comes from)?

@auturgy
Copy link
Collaborator

auturgy commented Jan 22, 2023

Does the ArduPilot script command generalise sufficiently for that use case? It's fairly widely used with ArduPilot, but my assumption was that the "api" it introduces isn't really rich enough to cover offboard/mavsdk (for ArduPilot or PX4).

@julianoes
Copy link
Collaborator

The API it introduces is richer than the one @TSC21 originally drafted here, from what I can see.

@peterbarker
Copy link
Contributor

The API it introduces is richer than the one @TSC21 originally drafted here, from what I can see.

It's hardly a "standard", however. It's "hey mavlink system, do something with these parameters, get back to me when you're done!".

Compare and contrast with MAV_CMD_NAV_WAYPOINT - there's general agreement on how that should work, knowledge on how that command is interpreted, and how the vehicle will move in response to it.

I'm not saying that the thing isn't useful, or that you shouldn't add some sort of dispatcher to ease consumption. Just that calling it a "standard" is a bit strange as the thing is just an opaque MAV_CMD parameter transport. If anything it would move away from standardisation as anything vaguely hard will become "use the scripting mavlink command with these magic parameters", rather than thinking through a message set.

Again, not saying it's a bad idea. I doubt we need or want a specific command set for specific aerobatic maneuvers, for example :-)

@auturgy
Copy link
Collaborator

auturgy commented Jan 23, 2023 via email

@JonasVautherin
Copy link

I do agree with @peterbarker 😊. It does feel like there is some need, though (aerobatic maneuvers is one example, another one could be "deliver a parcel and tell me when it's done", which could involve an operator action). I guess the hope then would be that people standardize what makes sense instead of using this feature for everything (which is not a given: people often take the lowest friction path).

Plus the fact that it exists both in PX4 and Ardupilot in some form sounds like there is a need 🙈

@julianoes
Copy link
Collaborator

@peterbarker I'm not trying to come up with a standard here by bumping this thread. I was interested in getting what ArduPilot has into common, so that I could use it too.

@hamishwillee
Copy link
Collaborator

@peterbarker @julianoes Not much to add.

To me this is a bit like the generic actuator command: a standard way to to trigger a generic "thing". In this case a standard way to trigger a script. Of course you're right that with this you can no longer write generic missions (for the same reason). As noted by others above, that's not a good reason to block this if the use case is something we all need and we can't see more-specific ways to achieve it.

With respect to the message itself my understanding is that:

  • this is a mission item not intended as a command (right?)
  • it contains a recipient-specific command, two parameters, and a timeout.
  • The mission engine would accept the item following the timeout (proceed to the next item).
  • There is no other feedback from the script.

Is that all right?

FWIW Looks OK.

  • Would be good to define behaviour if sent as a command.
  • You might define a mechanism for a script to indicate completion of a particular command - so you don't have to wait for timeout before knowing it is ready to accept more commands.
  • Or to put ^^^ another way, this implies (I think) that a script will be used in a particular way. We should outline those limits for script designers.

I agree with @auturgy - like any other item moved to common we should verify that the moved item does indeed meet the broad needs of the use case. We could discuss in mav call if you want.

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

8 participants