[ros-users] need help: set_parameters service callback very sluggish with nodelet-based camera1394 driver

[Jack O'Quin]

On Fri, Jan 28, 2011 at 9:32 AM, Blaise Gassend <blaise at willowgarage.com> wrote:
>> Thanks for the detailed example. I will probably do something similar
>> but simpler. I want to avoid big changes with the initial Diamondback
>> release coming up right away.
>
> Let me know what you come up with. I have not been fully pleased with
> how complex that multi_interface_roam snippet is, so if you have
> something that achieves the same thing more simply I'll probably
> imitate it.

Here's a simple solution that I believe works for my restricted case
(two threads, one needing to preempt the other). Note that the
original scoped_lock solution "worked" except for unacceptable
latency.

The idea is to keep the lock, but add a bool class member for when the
reconfig callback needs to interrupt the poll() thread. The poll()
loop is almost the same:

  /** device poll */
  void Camera1394Driver::poll(void)
  {
    // Do not run concurrently with reconfig().
    //
    // The mutex lock should be sufficient, but the Linux pthreads
    // implementation does not guarantee fairness, and the reconfig()
    // callback thread generally suffers from lock starvation for many
    // seconds before getting to run.  So, we avoid acquiring the lock
    // if there is a reconfig() pending.
    bool do_sleep = true;
    if (!reconfiguring_)
      {
        boost::mutex::scoped_lock lock(mutex_);
        do_sleep = (state_ == Driver::CLOSED);
        if (!do_sleep)
          {
            // driver is open, read the next image still holding lock
            sensor_msgs::ImagePtr image(new sensor_msgs::Image);
            if (read(image))
              {
                publish(image);
              }
          }
      }

    if (do_sleep)
      {
        // device was closed or poll is not running, sleeping avoids
        // busy wait (DO NOT hold the lock while sleeping)
        cycle_.sleep();
      }
  }

The reconfig() member must set reconfiguring_ true while running:

  /** Dynamic reconfigure callback
   *
   *  Called immediately when callback first defined. Called again
   *  when dynamic reconfigure starts or changes a parameter value.
   *
   *  @param newconfig new Config values
   *  @param level bit-wise OR of reconfiguration levels for all
   *               changed parameters (0xffffffff on initial call)
   **/
  void Camera1394Driver::reconfig(Config &newconfig, uint32_t level)
  {
    // Do not run concurrently with poll().  Tell it to stop running,
    // and wait on the lock until it does.
    reconfiguring_ = true;
    boost::mutex::scoped_lock lock(mutex_);

    // lengthy device reconfiguration...

   reconfiguring_ = false;             // let poll() run again
  }

This is not as general as your approach, but I think it a safer change
for camera1394 so late in the release. For me it's a small change
(including re-indentation):

  https://code.ros.org/trac/ros-pkg/changeset/35610

I've thought about portability to machines with weak storage ordering,
but I think the scoped_lock should always invoke any memory fences
required for the underlying processor architecture, so it should be
OK.

I believe an extra Driver::STREAMING state would work about the same,
but I do not want to add another state right now, the effect would be
too global.
-- 
 joq