On Sat, Feb 4, 2012 at 5:54 PM, Jack O'Quin <
jack.oquin@gmail.com> wrote:
> Your patch is a big help. I have no previous experience with those interfaces.
>
> The patch creates a separate thread to run the diagnostics updater,
> but only for the nodelet implementation. The camera1394_node does not
> create this thread or call the updater. I presume that is just a minor
> oversight due to the slightly strange camera1394 source file structure
> that has evolved.
Yeah, I was not too sure where I should put the initialization.
> Multi-threading is tricky in this driver, see the comments in
> Camera1394Driver::poll(). I am wondering if we could just call the
> updater there in the poll() thread, instead of creating another
> thread. Is there any large overhead in calling the updater?
The updater is publishing in /diagnostics at a fixed rate so no it is probably
not that time consumming. However, it is still a shame to slow down the
whole node.
> Also, I am still a little confused about how the diagnostics updater
> learns the actual frame rate of the camera. Shouldn't there be a
> common data structure update? Wouldn't that require a mutex lock?
The patch is missing a line: topic_diagnostics_.tick() should be called
each time a message is published. This function is thead safe so we can
safely publish diagnostics in one thread and tick in the main thread.
--
Thomas Moulard
(text/plain)