-
Notifications
You must be signed in to change notification settings - Fork 85
RSDK-11061 move GetPose from the Motion service API to robot API #713
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: main
Are you sure you want to change the base?
Conversation
proto/viam/robot/v1/robot.proto
Outdated
message GetPoseRequest { | ||
string name = 1; | ||
// the component whose pose is being requested | ||
common.v1.ResourceName component_name = 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.
if we want to avoid a breaking change later, maybe just make this a string?
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.
though making it a string could be risky if we don't end up doing the other project
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.
I'm good with either. I'll leave this up to you to decide, since you have the best understanding of whats going on
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.
I think it'll be better as a string - we can use a helper to only check the ShortNames() on a robot, and if there was a collision the framesystem wouldn't build properly anyway
proto/viam/robot/v1/robot.proto
Outdated
@@ -425,3 +435,22 @@ message GetVersionResponse { | |||
|
|||
string api_version = 3; | |||
} | |||
|
|||
message GetPoseRequest { | |||
string name = 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.
name should be unneeded here
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.
Do I understand correctly this isn't needed because this isn't on a resource anymore? If so, nice catch I missed this
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.
yep
This PR moves the GetPose method from the Motion service to the Robot itself.