Proposer: Sachin Chitta
Present at review:
Question / concerns / comments
- This is a set of messages for kinematics computation. Please review these messages and let me know if there are things that should be specified but are missing.
- Note that the service IKServiceWithCollision has changed but the Wiki documentation has not auto-updated yet. The new service:
# A service call to carry out an inverse kinematics computation # while checking collisions. # The inverse kinematics request kinematics_msgs/IKRequest data # The state of the robot - use this to specify positions for the rest of the joints on the robot motion_planning_msgs/RobotState robot_state # A set of ordered collision operations to determine the resultant collision matrix to use motion_planning_msgs/OrderedCollisionOperations ordered_collision_operations # Maximum allowed time for IK calculation float64 timeout --- # The returned solution # (in the same order as the list of joints specified in the IKRequest message) float64 solution
Random thoughts - my apologies if they are confused or mis-informed:
- The package seems focused on inverse kinematics. Should the package name reflect more of that? Kinematics is pretty general?
It is focused on inverse kinematics right now but there is no reason other kinematics related messages cannot be added in later. This review will not lock down the package to future additions.
In "WithCost", I have no idea how cost would be defined? In general, cost functions could contain lots of things. Why just a list of links?
You are right, the particular implementation was to get a collision cost for a set of links.
- Why doesn't the plain service have a robot state? The request message contains the list of joint to solve, but how do you set the other joints?
The assumption is that all the information you need to compute the IK is within this message itself. The robot_state is intended for joints that are not part of the IK computation itself
- Or is it helpful to explicitly define a starting frame? If not, I assume it's in a world frame and you would need the robot state to localize? Or, wait are you using the stamp frame in pose?? Documentation would help.
The starting frame is defined in the header of the message. The link_name defines the link that IK is being performed for while the frame_id in the header defines the frame in which the pose for that link is defined.
- (In the plain service, w/o robot state), are you assuming all joints between the stamp frame and the tip frame? Can you leave some out?
Yes. There is no strictness in the message itself but the way the nodes are implemented, you have to provide starting positions for all the joints that may be used in the IK computation
- Rather than naming "link_name" in the request, why not use a frame_id? Seems more consistent with the rest? Possible just a Pose, Base_frame_id, Tip_frame_id?
The term frame_id has been widely used throughout our stacks and an additional name with frame_id in it may lead to more confusion
- I'd personally like not to have the Request message. Seems that is just to collect some items that are shared? But other shared items (timeout, robot state) are not collected? I'd rather just spell it out in the service call.
Its a base ik request - its all the information you would require just to compute IK if you had a pose in the root frame of the chain, you don't need a timeout, robot state for it.
- In the fkin, how do you specify the base frame?
In the header
In general, it seems these are basically service requests for a pretty specialized code base = one service call for one algorithm? I'm a little worried that for general solvers, there may be other parameters (cost functions, tolerances, etc). So is it worth making this a package by itself, or just keep the service definitions with the code? And then, when we have multiple solvers and service calls we can see where the commonality lies?
These packages are supposed to be a starting API spec, not a complete and final API spec. They will evolve as we put in more use-cases but they seem to be sufficient for what we have done so far. Also, the requests themselves are universal, any custom configuration for individual solvers could be done through the ros parameter server.
- IKServiceWithCosts - even after reading the discussion here, I don't understand what cost function this is minimizing. If it is a collision cost, it seems that a list of links is still underspecified (does this include self collision, self-collision between adjacent links, obstacle collision, etc.?)
- What is the header in FKService used for? Does it specify what frame to use for the return value?
I'm worried that with the current approach we will start requiring a combinatorial number of service calls for IK with XXX and IK with yyy, and that this will become a maintenance nightmare. I'm especially worried about this together with the current naming, because once we have some other type of costs that we want to use, or once we want to add some new parameter to the cost function, it will become very unclear what the meaning of the different service calls are, and keeping the interfaces easy for people to understand may get harder and harder. One alternative would be to have service calls configured with those parameters before-hand, so that requests are all the same. We could do something like expand IKQuery to include a set of key-value pairs, so that it could flexibly represent many different IK algorithms into the future, but to keep the request itself simple and get rid of WithCollision, WithCost, etc. We could even do something like the PolledImage approach, where a user would ask for a service call that is configured the way they want it configured, that would dynamically create and advertise it, and then they could just call it without passing around lots of options.
- suggest renaming IKQuery as it can be easily taken as meaning "an actual query to compute IK". How about IKInformationRequest, IKNodeInformationRequest, etc.
why do IKServiceWithCollision and IKServiceWithCost have a RobotState field, but IKService does not?
is there a default option, where if RobotState is empty, or default, the current state of the robot is used?
Package status change mark change manifest)
Action items that need to be taken.
Major issues that need to be resolved
Take out IKWithCost - DONE
Move service response into a separate message as well - response is minimal and contained within two messages - one for solution and other for error code
Add an enum for the IKService response - DONE
Change the name of the IKService call - GetPositionIK - DONE
Change the name of the FKService call - GetPositionFK - DONE
Change IKRequest name to PositionIKRequest - DONE
Change IKRequest data to PositionIKRequest request - DONE
Change positions to seed_joint_positions in PositionIKRequest - DONE
Change link_name to ik_link_name - DONE
Change float64 timeout to ros::Duration timeout - DONE
Change float64 solutions to motion_planning_msgs/JointState - DONE - RobotState
FKRequest -change joint positions to JointState - DONE
FKRequest should be for an array of links - DONE
change link_name - fk_link_names - DONE
return will be array of PoseStamped messages + array of strings + enum for errors - dONE