search

ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
154 stars 181 forks source link

Long delay before starting trajectory streaming #191

Closed simonschmeisser closed 5 years ago

simonschmeisser commented 6 years ago

https://github.com/ros-industrial/industrial_core/blob/d14029bdf0b403ef4412ef8a7b16bbb43a0a7627/industrial_robot_client/src/joint_trajectory_streamer.cpp#L170

We wondered about long delays before trajectory streaming starts and there is actually a (up to) 250ms sleep before the sending thread goes into action. Is there any reason for it? I guess the idea was to avoid busy looping but that could also work with a shorter sleep.

I'll try to write a PR that uses a condition variable instead. Are we on C++98/03 still?

gavanderhoorn commented 6 years ago

250ms is not really that long (the robot probably takes longer to start moving, even if there was 0 ms sleep time). Are you sure you're not running into ros-industrial/abb#142?

simonschmeisser commented 6 years ago

Well, true, but it adds up in the end. We're currently looking at the old/default fanuc driver and admittedly there are bigger issues but 250ms wasted is already a big issue in itself. A typical cycle of our application right now consists of 8 trajectories so that is 2sec (worst case ...) wasted already.

gavanderhoorn commented 6 years ago

Sure. Lowering the sleep to 10ms or something would be fine.

simonschmeisser commented 6 years ago

It looks like industrial_core is not yet on c++11, am I correct on that? Would it be possible to switch? This would allow a non-polling version based on std::condition_variable

gavanderhoorn commented 6 years ago

No, we're not using C++11 for this package yet.

The minimal patch would be to lower the period. CPU usage increase should be minor (if at all really).

If you really want to, Boost has condition variables as well, so you could use those.

davetcoleman commented 6 years ago

I ran into the no C++11 thing yesterday. It seems to me a simple no-brainer to modify the CMakeList.

pbeeson commented 5 years ago

I recall lowering the 250ms period in a previous PR and people didn't like that and didn't merge the PR. We found 250ms to be prohibitive. If you have Let's say 20 trajectories in an industrial tasks, that's 5 seconds to the cycle time (worst case). In my fork, we made it 100ms and see no increase in overall CPU usage but a measurable difference in tasks that use many trajectories, but think 10ms should be fine.

pbeeson commented 5 years ago

Sorry it was PR #174 where it was set to 100ms. There are other changes here too, but this PR was never accepted.