-
Notifications
You must be signed in to change notification settings - Fork 25
[RSDK-10923] Add IsHoldingSomething to Gripper API #454
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
[RSDK-10923] Add IsHoldingSomething to Gripper API #454
Conversation
f726daf
to
aba2ec8
Compare
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.
Request to add a test and also fix compilation failures, otherwise lgtm!
Gripper::holding_status MockGripper::is_holding_something(const ProtoStruct&) { | ||
Gripper::holding_status res; | ||
res.is_holding_something = false; | ||
res.meta = {}; | ||
return res; | ||
} | ||
|
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.
can we set res.meta
to the protoStruct that's passed into the function, and then write a test of this method in test_gripper
?
src/viam/sdk/components/gripper.hpp
Outdated
@@ -5,6 +5,7 @@ | |||
|
|||
#include <string> | |||
|
|||
#include <viam/sdk/common/proto_convert.hpp> |
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.
You can probably just include proto_value.hpp
here.
else { | ||
holding_status.meta = {}; | ||
} |
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 you actually need this else?
holding_status.is_holding_something = response.is_holding_something(); | ||
if response | ||
.has_meta() { | ||
holding_status.meta = from_proto(response.meta()); |
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 file can include the proto_convert.hpp
header.
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.
LGTM
5c32ba3
into
viamrobotics:workflow/update-protos
No description provided.