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

DriverNodelet's Poll Timeout Handling #213

Closed
jimmyw404 opened this issue Feb 1, 2019 · 3 comments
Closed

DriverNodelet's Poll Timeout Handling #213

jimmyw404 opened this issue Feb 1, 2019 · 3 comments

Comments

@jimmyw404
Copy link

jimmyw404 commented Feb 1, 2019

In the DriverNodelet::devicePoll() function if the VelodyneDriver::poll() call fails, the devicePoll thread will exit. This happens if there is a network disconnect, the puck's power cycles etc.

This was requiring me to restart the velodyne Driver nodelet whenever I had a timeout.

Is the intent that there is some other mechanism to restart the DriverNodelet when it is detected it exits? What is the recommended way to do this?

For now I've commented out the break; line. My DriverNodelet::devicePoll() function looks like this:

/** @brief Device poll thread main loop. */
void DriverNodelet::devicePoll()
{
  while(ros::ok())
    {
      // poll device until end of file
      running_ = dvr_->poll();
      if (!running_)
      {

      	ROS_ERROR("W: DriverNodelet:devicePoll poll failed, who cares?");
//        break;
      }
    }
  running_ = false;
}

And now it persists through disconnects and works great, but I'm guessing this is the inappropriate way to do this.

@JWhitleyWork
Copy link
Contributor

The ROS_ERROR (or, ROS_ERROR_THROTTLE would probably be better in this case) is the correct way to handle this. I would make the message something more... "professional" like "DriverNodelet::devicePoll - Failed to poll device" for the real message. Would you mind submitting a PR with this change?

@JWhitleyWork
Copy link
Contributor

@jimmyw404 - Check the branch maint/poll_timeout_handling. Let me know if it works and I'll merge it if it does.

@jimmyw404
Copy link
Author

@JWhitleyAStuff Thank you for that code change. It will work just fine.

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

No branches or pull requests

2 participants