microsoft / Azure_Kinect_ROS_Driver

A ROS sensor driver for the Azure Kinect Developer Kit.
MIT License
304 stars 226 forks source link

Fixed node not shutting down, if sensor stream breaks #183

Closed jmachowinski closed 3 years ago

jmachowinski commented 3 years ago

Currently the node will break silently if the camera stops sending data. This happens as it blocks infinite in get_capture. This commit reduces the timeout to 5 times the expected time it would normally take for a new frame to arrive. For unknown reasons, the first frame takes way longer to arrive, therefore we wait up to four seconds for the first frame.

Fixes #181

Description of the changes:

-

-

Required before submitting a Pull Request:

I tested changes on:

Test was performed on an AMD EPYC System with one camera, multiple stalls occurred, node was shut down as expected. (And the restarted by roslaunch)

RoseFlunder commented 3 years ago

Just a small optimization idea computation wise: You could initialize two variables instead of your bool:

auto waitTime = std::chrono::milliseconds(4 * 1000);
auto regularWaitTime = std::chrono::milliseconds(1000 * 5 / params_.fps);

Then after getting a capture you just assign waitTime to the shorter interval: waitTime = regularWaitTime

So in the first iteration it takes the initial value and in all following iterations the smaller value. Of you course you still have this one assignment every loop but its less than having to branch with a bool value and do the assigning inside the branches.

ooeygui commented 3 years ago

@jmachowinski Thank you for your contribution. We do need to keep both the ROS1 and ROS2 branches in sync, could you submit a pull request for the ROS2 build as well?

jmachowinski commented 3 years ago

@ooeygui I don't have an ros2 installation and currently also no time, to set it up. I could cherry pick the commit on the foxy branch, and just open the pull request with the request for someone to try it out though...

jmachowinski commented 3 years ago

@RoseFlunder I created a short version : https://github.com/jmachowinski/Azure_Kinect_ROS_Driver/blob/timeout_fix2/src/k4a_ros_device.cpp but is it still as readable and understandable as the initial patch ?

I normally prefer readability above minor performance optimizations...

amelhassan commented 3 years ago

Thank you for your contributions @jmachowinski , I made a ROS2 equivalent of your change, PR #205 . It would be great if you could update your PR with the same format for the changes. This takes into consideration @RoseFlunder suggestions but also keeps the readability in tact as you suggested.

jmachowinski commented 3 years ago

okay, I'll try to do this next week, thanks @amelhassan

jmachowinski commented 3 years ago

Updated, as requested, rebased on latest head. Note, I also added two comments to make the code more obvious, and removed the TODO.

amelhassan commented 3 years ago

Looks good!