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

remove undeserved error log for empty JointState.position #64

Merged
merged 1 commit into from Nov 16, 2016

Conversation

sbarthelemy
Copy link
Contributor

as per the JointState message doc, the position member must be
either empty OR have the same size as the name member.

Some background here http://answers.ros.org/question/244796/jointstate-messages-with-heterogeneous-position-and-velocity-sensors/

@wjwwood
Copy link
Member

wjwwood commented Oct 17, 2016

@tfoote what do you think about this?

@tfoote
Copy link
Member

tfoote commented Oct 17, 2016

The raw joint state message without position is not invalid in of itself, but I don't think that the robot_state_publisher can do anything without the position message. I think that publishing the two different Joint State messages is the best thing to do as it encapsulates the raw data. And you're right that the error in the joint_state_listener is overly strong. However I think that the code should probably be downgraded to a warning, and the message clarified that the message cannot be handled, instead of saying that it's invalid.

Note that if this is downgraded to a warning (it should probably be throttled too) it should still ignore the message that it cannot publish.

@sbarthelemy
Copy link
Contributor Author

I think that publishing the two different Joint State messages is the best thing to do as it encapsulates the raw data.

In the end (and since I'm sticking to indigo right now), I circumvented the issue by publishing two JointState messages, in two topics ("joint_states" and "wheel_states"). robot_state_publisher only listens to "joint_states".

But I'm still interested in fixing this erroneous spammy log.

The raw joint state message without position is not invalid in of itself, but I don't think that the robot_state_publisher can do anything without the position message. (...) However I think that the code should probably be downgraded to a warning, (...)

Well I personally don't see a use for this warning and it complicates the matter a bit since we'll need to choose a throttle period.

Similarly, there is no warning for empty JointState messages (not names nor position) which are also both valid and useless (ie. not handled by robot_state_publisher), and I don't see why there should be any.

But anyway, both solutions (no log or a throttled warning) would be an improvement, and I'll be happy to submit the one you prefer.

I just pushed the second one for review.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR I just had a two suggestions to improve the debuggability for new users.

@@ -86,7 +86,11 @@ void JointStateListener::callbackFixedJoint(const ros::TimerEvent& e)
void JointStateListener::callbackJointState(const JointStateConstPtr& state)
{
if (state->name.size() != state->position.size()){
ROS_ERROR("Robot state publisher received an invalid joint state vector");
if (state->position.empty()){
ROS_WARN_THROTTLE(300, "Robot state publisher ignored a JointState message whose position member was empty");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to list the names, and also mention that this message will not reappear for 300 seconds. For those who don't know that this is a throttled output.

As per the JointState message doc, the ``position`` member must be
either empty OR have the same size as the ``name`` member.

In case it is empty, the message is valid, but we will still warn
we won't use it.
@sbarthelemy
Copy link
Contributor Author

@tfoote: Hi, I updated the PR following your suggestions, could you have a look? Thx!

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks that's nice and clear now.

@wjwwood wjwwood merged commit 9992dd3 into ros:kinetic-devel Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants