On Fri, Jan 28, 2011 at 9:32 AM, Blaise Gassend <
blaise@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