ros-industrial / motoman

ROS-Industrial Motoman support (http://wiki.ros.org/motoman)
143 stars 194 forks source link

Tight loop causing delays in processing #524

Closed ted-miller closed 1 year ago

ted-miller commented 1 year ago

This loop has no sleep commands https://github.com/ros-industrial/motoman/blob/7860ff545106d98ed6a3fa0121f2fb60891f69bd/motoman_driver/MotoPlus/MotionServer.c#L1434

A user has reported that with three groups, one (or more) of the tasks can get locked out for 0.4 - 3.0 seconds. (Using mpStopWatch inside Ros_MotionServer_AddToIncQueueProcess)

I have advised him to try doing a sleep every 10 or 20 iterations of the loop. If he reports that this helps, I'll open a PR.

ted-miller commented 1 year ago

User reports that he added the sleeps in the loop and also increased the size of the internal increment queue. This appears to have resolved the issue.

ted-miller commented 1 year ago

Hmmm, found another possible tight-loop. https://github.com/ros-industrial/motoman/blob/e074519f7610621877e13d74a12877a41289fad0/motoman_driver/MotoPlus/Controller.c#L354

I'm not sure if mpSelect relinquishes the CPU when there's no timeout value. But I'm thinking a delay in this loop would be good.

gavanderhoorn commented 1 year ago

I would be surprised if VxWorks's select(..) does not simply suspend the process/thread calling it if there are no file descriptors ready.

It would be a rather poor implementation of select(..) if it used a busy loop. Having the scheduler suspend the task -- while keeping an eye on the FDs -- is what makes select(..) useful.

ted-miller commented 1 year ago

But we're specifying NULL for the timeout to get an immediate return. That's why I'm concerned that the task might not be getting suspended.

gavanderhoorn commented 1 year ago

According to POSIX, select(2):

If timeout is specified as NULL, select() blocks indefinitely waiting for a file descriptor to become ready.

now the only question would be whether VxWorks's select(..) follows POSIX.

According to the VxWorks 6.3 headers:

int select
    (
    int              width,     /* number of bits to examine from 0 */
    fd_set *         pReadFds,  /* read fds */
    fd_set *         pWriteFds, /* write fds */
    fd_set *         pExcFds,   /* exception fds */
    struct timeval * pTimeOut   /* max time to wait, NULL = forever */
    )
ted-miller commented 1 year ago

Oh... I'm an idiot. I had it in my head that the logic was reversed. You're correct.

gavanderhoorn commented 1 year ago

@ted-miller: would this still be something you'd want to address?

ted-miller commented 1 year ago

Let's close this for now. A followup email from this user indicated:

I wanted to revisit this. It seems sleeping in this loop - https://github.com/ros-industrial/motoman/blob/7860ff545106d98ed6a3fa0121f2fb60891f69bd/motoman_driver/MotoPlus/MotionServer.c#L1434 & increasing the queue size, did not resolve the issue.

Given that there have been no other complaints (over many years), I'm not confident that this is actually an issue.

Unfortunately, communication from the user has stopped and I do not know how the issue was ultimately resolved.