-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add NodeletLazy abstract class for lazy transport #45
Conversation
Maintainers, could you please review this? cd test_nodelet_topic_tools
catkin build --this
rostest test/test_nodelet_lazy.launch --text |
Any comments, please/ |
sorry for the late reply, will review this shortly. |
Friendly ping. |
ping |
finally got back decent internet, will review this today, sorry for the very long wait |
// timer to warn when no connection in a few seconds | ||
ever_subscribed_ = false; | ||
timer_ever_subscribed_ = nh_->createWallTimer( | ||
ros::WallDuration(5), |
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.
Would it make sense to expose this timer as a ros param ?
if (!lazy_) | ||
{ | ||
boost::mutex::scoped_lock lock(connection_mutex_); | ||
ever_subscribed_ = true; |
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.
set flag after subscribe succeeded
{ | ||
if (!ever_subscribed_) | ||
{ | ||
ever_subscribed_ = true; |
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 above
|
||
/** @brief | ||
* Advertise a topic and watch the publisher. Publishers which are | ||
* created by this method. |
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.
Rephrase this as a single sentence? the meaning of "Publishers which are created by this method" is unclear to me. Did you mean "update the list of Publishers created by this method" ?
@wkentaro Code looks good to me overall, a few nicktip comments to address/clarify but all of them are cosmetics. |
e82dc40
to
33e29aa
Compare
User can disable this feature by setting -1 to the param.
33e29aa
to
50c0a10
Compare
Thank you for the reviews. I fixed the code. |
great! thanks for iterating on this |
Thanks! |
This class is to crete nodelet for 'lazy' transport.
Class for Python node is already merged in kinetic-devel branch of topic_tools. ros/ros_comm#713
Lazy transport is an essential feature for perception nodelets that handle image and point cloud, because
the processing should run only when it is necessary to reduce the CPU load.
FYI, the similar nodelet abstract class is used in ros-perception/opencv_apps package. https://github.com/ros-perception/opencv_apps/blob/indigo/include/opencv_apps/nodelet.h