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.

Sign up for GitHub

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

Jump to bottom

Winch and Gripper plugins #308

Merged
merged 7 commits into from
Jan 31, 2023
Merged

Winch and Gripper plugins #308

merged 7 commits into from
Jan 31, 2023

Conversation

potaito
Copy link
Contributor

@potaito potaito commented Jan 20, 2023 •
edited
Loading

Related PRs

Description

Message definitions for Winches and Grippers. This reflects the following mavlink messages:

  • MAV_CMD_DO_GRIPPER (211)
  • MAV_CMD_DO_WINCH (42600 )

and their submessages for the gripper and winch actions, as well as the winch status.

Please don't merge before I squashed the commits

@potaito potaito marked this pull request as draft January 20, 2023 10:38
This was referenced Jan 20, 2023
Merged
Merged
JonasVautherin
JonasVautherin requested changes Jan 22, 2023
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Looking nice! 🚀

protos/gripper/gripper.proto Outdated
Comment on lines 49 to 50
RESULT_COMMAND_DENIED_LANDED_STATE_UNKNOWN = 6; // Command refused because landed state is unknown
RESULT_COMMAND_DENIED_NOT_LANDED = 7; // Command refused because vehicle not landed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do those make sense for a gripper? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, sorry. I forgot to narrow these down. I copied the struct from actions.proto initially 🙈

I have to check which responses are possible to a vehicle command.

protos/gripper/gripper.proto Outdated
Comment on lines 52 to 55
RESULT_VTOL_TRANSITION_SUPPORT_UNKNOWN = 9; // Hybrid/VTOL transition support is unknown
RESULT_NO_VTOL_TRANSITION_SUPPORT = 10; // Vehicle does not support hybrid/VTOL transitions
RESULT_PARAMETER_ERROR = 11; // Error getting or setting parameter
RESULT_UNSUPPORTED = 12; // Action not supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here: does that happen for the gripper?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could if the gripper commands are not implemented, e.g. for an older PX4 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RESULT_UNSUPPORTED is one of the few that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to support all of the possible results that the command sender can return?

https://github.com/mavlink/MAVSDK/blob/444d6b693b2d76cd594b1df074f28f468cc78f47/src/mavsdk/core/mavlink_command_sender.h#L24-L37

In case of simple systems like winches and grippers, the results NoSystem, ConnectionError, and Timeout for example are basically the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, it seems that the grpc code auto-generation at least expects there to be a NoSystem result, so I can't remove that one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are different though.

protos/winch/winch.proto Outdated Show resolved Hide resolved
protos/winch/winch.proto Outdated Show resolved Hide resolved
JonasVautherin
JonasVautherin previously approved these changes Jan 24, 2023
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

🚀

@potaito
Copy link
Contributor Author

potaito commented Jan 24, 2023

@JonasVautherin @julianoes there are now the following possible results:

  • RESULT_UNKNOWN, as a default fallthrough should MAVSDK receive something it cannot handle
  • RESULT_SUCCESS, obvious
  • RESULT_NO_SYSTEM is required by MAVSDK
  • RESULT_BUSY corresponds to mavlink's MAV_RESULT_TEMPORARILY_REJECTED as far as I can tell. A winch or gripper might send this while it's already performing an operation. These can take 30 seconds or more.
  • RESULT_COMMAND_DENIED for supported commands with invalid parameters. For winches and grippers this is probably not that important, since there are only few parameters to begin with, and the winch mavlink message for example states, that parameters are simply ignored. I think I'm going to remove this result as well...
  • RESULT_TIMEOUT for when the winch or gripper is not responding to commands with an ACK.
  • RESULT_UNSUPPORTED makes sense to have. Particularly for winches, because there are currently 10 winch actions, and no winch I know off supports all of these, but only a subset of actions.
  • RESULT_FAILED for when an action was acknowledged and then attempted to be executed, but something went wrong. For example, the over-current protection kicked in because the winch's rope is caught somewhere.

@potaito potaito marked this pull request as ready for review January 24, 2023 08:28
@potaito potaito force-pushed the winch_plugin branch 2 times, most recently from 7b763ee to 74d72d8 Compare January 25, 2023 07:40
@potaito
Copy link
Contributor Author

potaito commented Jan 25, 2023

Just squashed the whole thing into a single commit

JonasVautherin
JonasVautherin previously approved these changes Jan 25, 2023
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

🚀

julianoes
julianoes reviewed Jan 30, 2023
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Nice, thanks for that!

protos/winch/winch.proto Outdated Show resolved Hide resolved
protos/winch/winch.proto Outdated Show resolved Hide resolved
@potaito
add documentation to winch status fields
e89837c 
Co-authored-by: Jonas Vautherin <jonas.vautherin@protonmail.ch>
JonasVautherin
JonasVautherin previously approved these changes Jan 31, 2023
View reviewed changes
@potaito
Copy link
Contributor Author

potaito commented Jan 31, 2023

Sorry, that's what happens when I use the github web GUI to make changes to a file 😅
CI is now fixed.

@JonasVautherin JonasVautherin merged commit 6d31e4a into mavlink:main Jan 31, 2023
@potaito potaito deleted the winch_plugin branch January 31, 2023 10:34
@potaito potaito mentioned this pull request Jan 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianoes julianoes julianoes left review comments

@JonasVautherin JonasVautherin JonasVautherin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@potaito @julianoes @JonasVautherin