-
Notifications
You must be signed in to change notification settings - Fork 55
Suggestion for new dynamic detachable joint plugin #533
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
base: gz-msgs12
Are you sure you want to change the base?
Suggestion for new dynamic detachable joint plugin #533
Conversation
| // TODO: Add optional header data | ||
| // gz.msgs.Header header = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to add or remove this, otherwise If we enable in the future this we will break ABI and it couldn't be backported
| // TODO: Add optional header data | ||
| // gz.msgs.Header header = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
|
Can you fix the DCO on this commit? |
b81297a to
b959d86
Compare
|
I have signed it off and also addressed the comments. Kindly let me know once u test it |
caguero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the message!
|
@Mergifyio rebase |
Signed-off-by: Adarsh Karan Kesavadas Prasanth <[email protected]>
Signed-off-by: Adarsh Karan Kesavadas Prasanth <[email protected]>
Signed-off-by: Adarsh Karan Kesavadas Prasanth <[email protected]>
Signed-off-by: Adarsh Karan Kesavadas Prasanth <[email protected]>
✅ Branch has been successfully rebased |
8ae6452 to
d43f7ad
Compare
azeey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I have a few comments, but hopefully, they should not be difficult to address.
| string child_link_name = 2; | ||
|
|
||
| /// \brief Command to attach or detach | ||
| string command = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is a limited list of commands here, I suggest we change this to an enum, see
gz-msgs/proto/gz/msgs/user_cmd.proto
Line 36 in 989f1fd
| enum Type |
| message AttachDetachResponse | ||
| { | ||
| /// \brief True if the operation was successful | ||
| bool success = 1; | ||
|
|
||
| /// \brief Additional message or error description | ||
| string message = 2; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it could be a generic Result type. How about:
in proto/gz/msgs/result.proto
message Result
{
/// \brief 0 if the operation was successful
int error_code = 1;
/// \brief Additional message or error description
string message = 2;
}| /// \interface DynamicDetachableJoint | ||
| /// \brief Message for dynamic detachable joint | ||
|
|
||
| import "gz/msgs/header.proto"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header is imported here, but it's not being used.
| package gz.msgs; | ||
|
|
||
| option java_package = "com.gz.msgs"; | ||
| option java_outer_classname = "DynamicDetachableJointProtos"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move the AttachDetachResponse to Result, this will just be AttachDetachRequest
|
|
||
| message AttachDetachRequest | ||
| { | ||
| /// \brief Name of the child model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add header here?
🎉 New feature
Closes -
Summary
Added a new protobuf defn to support the
dynamic_detachable_jointsystem discussed in Issue gazebosim/gz-sim#2362NOTE: the
headerdata part hasn't been implemented yetTest it
Adding the plugin to robot model URDF/SDF
GZ Service call command
ROS2 Service call command
Checklist
codecheckpassed (See contributing)Generated-by: Remove this if GenAI was not used.
Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.