-
Notifications
You must be signed in to change notification settings - Fork 66
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
Publish sensors tfs as static, and allow configuring odom frequency #202
base: indigo-devel
Are you sure you want to change the base?
Conversation
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.
Thanks for your PR. I went ahead and created a review with some minor comments. It generally looks good; we would need to make some changes in the future to address the use case of a moving sensor (proposed by #201), but this is not in the scope of the current PR.
@@ -150,6 +151,9 @@ namespace stdr_robot { | |||
//!< ROS tf transform broadcaster | |||
tf::TransformBroadcaster _tfBroadcaster; | |||
|
|||
//!< ROS tf2 static transform broadcaster | |||
tf2_ros::StaticTransformBroadcaster static_broadcaster; |
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.
Prepend underscore to comply with the coding styleguide (I know it's ugly...).
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.
right
stdr_robot/src/stdr_robot.cpp
Outdated
_tfTimer = n.createTimer( | ||
ros::Duration(0.1), &Robot::publishTransforms, this, false, false); | ||
ros::Duration(1.0/odometry_rate), &Robot::publishTransforms, this, false, false); |
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.
Could we put the default to 100Hz and not use the parameter? If you use the parameter you would need to check for division by zero. I also don't know what would be the computational impact if we increase to 100Hz though...
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.
Impact is small, I get similar same CPU usage (~85% vs. ~90% in my machine). What is normal as the function is quite light. But 50 Hz would be equally fine.
getName(), | ||
_sensors[i]->getFrameId())); | ||
} | ||
_odomPublisher.publish(odom); |
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.
In this function the same time can be used when publishing odometry and the pose tf, instead of ros::Time::now()
twice, in order to avoid the issue that you mentioned in your ROS Answer.
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.
true. In fact, using the same timestamp also for the sensors made things better (though using static tf is much much better anyway and the reasonable thing to do)
As explained in this ROS answer, the 10Hz at witch STDR publishes odom and sensors TFs causes important delays when processing sensor readings. So solution has two parts: