-
Notifications
You must be signed in to change notification settings - Fork 12
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
Move arm handover #1023
base: master
Are you sure you want to change the base?
Move arm handover #1023
Conversation
self.robot_name = robot_name | ||
self.arm_name = arm_name | ||
|
||
def handover_to_human(self, timeout=10): |
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 definitions handover_to_human and handover_to_robot are now defined twice (here and in the arm class). I think they can be removed here if the functionality (return exchange_with_human) is shifted to the arm class.
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.
Actually, this and #1024 are the start of cleaning out the much of the Arm class by decomposing its code into a set of smaller interfaces. this and #1024 are the first steps in that.
However, since that is a complicated operation on a crucial part of the robot, I am taking baby steps, using knowledge gained in #1011. As such, your suggestion is in the opposite direction of what is being aimed for :)
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 like that you are taking small steps and I think it is not a big issue to have a little bit of duplicate code, but I guess that in the end we want to get rid of this. But for now I guess this is fine.
""" | ||
return self._exchange_with_human(True, timeout) | ||
|
||
def _exchange_with_human(self, to_robot, timeout=10): |
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.
Good idea to combine this! I like it!
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 as in #1024, if/once it is tested it is good to go!
First step of arm splitting, move the handover function to its own class.