Skip to content
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

Make error message in PositionIKRequest less confusing #2517

Closed
felixvd opened this issue Feb 13, 2021 · 4 comments · Fixed by #2544
Closed

Make error message in PositionIKRequest less confusing #2517

felixvd opened this issue Feb 13, 2021 · 4 comments · Fixed by #2544
Labels
good first issue If you want to contribute work for the first time, look at this moveit day candidate This issue can likely be resolved in less than a day

Comments

@felixvd
Copy link
Contributor

felixvd commented Feb 13, 2021

Description

As described in this ROS Answers post, calling the compute_ik service with an empty robot_state field displays the error Found empty JointState message because the field is parsed in _robotStateMsgToRobotStateHelper after passing through here. Something like "No hint for IK provided - starting from current robot state" should be displayed before the IK is called, imo.

I believe the error message normally appears when an empty message arrives on /joint_states, and that behavior should persist. But I can't find where exactly it happens (presumably in planning_scene_monitor/current_state_monitor), so I might just keep the error print in utils and add the extra information.

This would be a good first-timer issue since it only requires small changes in a few files.

Steps to reproduce

Call the compute_ik service with an empty robot_state field in the PositionIKRequest message.

@felixvd felixvd added moveit day candidate This issue can likely be resolved in less than a day good first issue If you want to contribute work for the first time, look at this labels Feb 13, 2021
@FabianSchuetze
Copy link

I have been toying with this issue and would happily try sending a patch at WMD. If anymore would also want to work on this during WMD, I'd be happy to do it together (I'm working at EU hours)!

@v4hn
Copy link
Contributor

v4hn commented Mar 10, 2021

Sorry @FabianSchuetze , I just saw this issue.

The mentioned error message is an artifact of #499 . Before this pr, robot state messages were always interpreted relative to the current robot state. I just filed #2544 to remove the error.

I believe it is perfectly fine behavior to have the service use the current state as seed, if nothing else is provided.
If anything should be changed here, it should probably be the weird long comment in PositionIKRequest that asks users to always fill in the field.

@FabianSchuetze
Copy link

Thanks @v4hn for your detailed an kind comment - it provided very valuable background info for me!

Do we want to modify the comment for the PositionIKRequest message? We could say something such as: "If the robot state is set explicitly, the IKService will use this state as basis for the IK calculation. If not, the service uses the current state.". I think this could help users but is of course just a very minor thing.

@v4hn
Copy link
Contributor

v4hn commented Mar 10, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue If you want to contribute work for the first time, look at this moveit day candidate This issue can likely be resolved in less than a day
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants