Added support for nodes handles with specific callbacks queues #701

alainsanguinetti · 2016-06-29T09:39:34Z

Hello,

Working with MoveIt in my company showed the need for the possibility to pass node handles with specific callback queues to the move_group interface and the current state monitor.
These are the modifications that I made to have it working.
Let me know if there is anything you think I should update more.

Regards,

v4hn · 2016-07-01T10:42:55Z

planning/kinematics_plugin_loader/src/kinematics_plugin_loader.cpp

+      kinematics_loader_.reset(); // we delete it manually
+      ROS_DEBUG( "KinematicsLoaderImpl destructor");
+  }
+


This commit has nothing to do with the matter addressed in the pull-request and is separately discussed in #702.
Please remove this commit from the request.

Sure, will do that.

v4hn · 2016-07-01T10:55:44Z

Hey @alainsanguinetti,
thanks for the request! Could you please address the comments?
Also, what is your use-case for which this change is neccessary?

As this is ABI breaking in a central class of MoveIt!, I do not like to see this merged into indigo.
Instead I would propose to merge this (after a clean up and with an explanation) into kinetic.

alainsanguinetti · 2016-07-01T13:20:48Z

Hello, I think I responded to all the very relevant comments.
The use case is the following:

One node is running, it instanciates objects and move them to separate threads. Each object has its own callback queue that it calls periodically. No one calls ros::spin / spinOnce because we want everything to be scoped to a thread.

The creation of the move group works fine because the move_group calls ros::spin / spinOnce (which is quite a strong assumption actually, maybe the presence of a callback queue should prevent those calls).
What does not work is the use of the current state monitor. Since nobody calls its callbacks which are in the global space, it is never updated and we cannot use it to read the current state of the end effector (It does work if we spin at some point but we don't want to have to do this.)

To solve this I tried to give the node handle with our callbacks options to the state monitor but then, the move_group interface waits for the callbacks of the node to be called which does not happen of course. This is why I added the calls to process the callbacks of the callback queue.
Maybe an even better option would be to remove those checks ?

I'm not really sure what ABI breaking means, I would like to highlight that this is an option so that it works fine with current code.
As for the release in which it gets merged, I do not really have any specific preferences. All I know is that we use Indigo on 14.04 because it is the only combination that was fully working and met our needs. There is something like a deprecated mongoDB driver from ubuntu 15 on that prevents running moveit properly.

At last, seen the issues with this PR, should I close this one and open a new clean one on a dedicated branch ? Would that make sense ?

v4hn · 2016-07-04T20:01:12Z

planning_interface/move_group_interface/include/moveit/move_group_interface/move_group.h

+      the wait time is unlimited.
+    *
+    * @param opt. An option structure, if you pass a node with a specific callback queue, it has to of type ros::CallbackQueue.
+    */


Please remove this comment change, you didn't change anything there.

Ok, let me change that comment: I don't see why option should get an @param line when everything else does not and I don't understand why you added the comment there.
This seems to be highly specialized to your application and it is pretty confusing to newcomers.
So I would prefer to see this change removed from the request.

It's just to highlight that the callback queue must be of type ros::CallbackQueue. It's highly unlikely that it happens but it seems to be possible from the way it is coded.

Then, could you please add something like "(This is the default case in ROS)" at the end of the comment, to make it less confusing for people reading the code for the first time?

v4hn · 2016-07-04T21:44:14Z

breaking ABI compatibility means that everyone using the library will have to recompile their code and will get segfaults and (in this case) missing symbol exceptions if they don't.
It's the small brother of API changes.
Currently MoveIt! does not provide sonames to notify users that they have to recompile their code, so
all we could do is to send a mail to the mailing lists to tell people.
Therefore, I try to keep ABI breakage (as changing a function signature in your case) away from indigo.

Could you please close this request and open a new one (cleaned up) targeting jade-devel?
I agree that the NodeHandle for the StateMonitor should be optionally specified by the user,
this is a shortcoming in the current code.

I'm not sure about your getCallbackQueue solution, but it results from poor design in roscpp - NodeHandle does not provide a way to spin its queues - ,
so if we want to have the sleeps there, we might have to live with it.
I don't like the idea to remove the connecting-loops, because this will result in errors if you call e.g. MoveGroup::pick() right away - Moving them might be an idea, but that would be a different pull-request.
@davetcoleman, @mikeferguson any comments on this construction?

alainsanguinetti · 2016-07-05T01:42:46Z

How about keeping the current function signature and adding the proposed one as a new one without optional arguments ?
The current one would simply call the new one with the default node handle.
I'm not so familiar with compiling and linking but this way we would keep the current signature for the function.

v4hn · 2016-07-05T08:23:10Z

How about keeping the current function signature and adding the proposed one as a new one without optional arguments ? The current one would simply call the new one with the default node handle.

Yes, that works for indigo-devel too. +1 for that method. Please go ahead and adjust the request to address the discussed changes. In case you're not that familiar with git/github: you can use `git rebase -i` (interactive rebasing) to get rid of unnecessary commits in the branch and change code in existing commits and `git push -f` to push the changed branch to your own github remote, replacing the current branch. The pull-request is bound to your branch "indigo-devel", so if you update something there, the request automatically changes.

alainsanguinetti · 2016-07-05T09:33:19Z

Modifications done.
Compiler warns me that using constructor delegation as I have done here is only c++11.
Alternatively I can keep both the current and new constructors but that does not seem very clean and maintenance-friendly to me.

v4hn · 2016-07-05T09:44:39Z

planning_interface/move_group_interface/include/moveit/move_group_interface/move_group.h

+      \param wait_for_server. Optionnal timeout for connecting to the action server. If it is not specified, the wait time is unlimited.
+      \param opt. A MoveGroup::Options structure, if you pass a ros::NodeHandle with a specific callback queue, it has to of type ros::CallbackQueue
+        (which is the default type of callback queues available in ROS)
+    */


"Optionnal" -> "Optional"
"it has to of type" -> "it has to be of type"

v4hn · 2016-07-05T11:44:38Z

The delegation constructor is fine by me. ROS has many places where it does not respect c++98
and MoveIt! doesn't compile with -Werror anyway at the moment...

pr#701 updated code PR moveit#701 typo fixes and overload for getSharedStateMonitor

alainsanguinetti · 2016-07-06T01:28:13Z

Hopefully this time I got it right!

v4hn · 2016-07-06T09:30:04Z

On Tue, Jul 05, 2016 at 06:28:14PM -0700, Alain Sanguinetti wrote:

Hopefully this time I got it right!

Hehe, nearly.
One last thing I just noticed is that you didn't add a comment anywhere
on why you replaced the ros::spinOnce() calls. The new construct
is pretty hard to understand without background knowledge, so
could you please add something like this right above the calls to callAvailable?

// explicit ros::spinOnce on the NodeHandle that manages the action client

Sorry for all the nit-picking...

Other than that +1. The patch works as expected on the tutorials,
so if another dev could +1 this I would like to merge it into indigo-devel and later.

alainsanguinetti · 2016-07-06T09:52:46Z

@v4hn I added comments, can you check if they are good ?
No problem with nit-picking ;-) I prefer clean, easy to read and robust.

v4hn · 2016-07-06T10:43:11Z

On Wed, Jul 06, 2016 at 02:52:47AM -0700, Alain Sanguinetti wrote:

@v4hn I added comments, can you check if they are good ?
No problem with nit-picking ;-) I prefer clean, easy to read and robust.

perfect!

@davetcoleman : the patch works as expected and preserves ABI, the only thing
I would like to have a +1 on is the downcast here.

davetcoleman · 2016-07-06T21:34:48Z

I've never seen this sort of thing:

( ( ros::CallbackQueue * ) node_handle_.getCallbackQueue())->callAvailable();

So I can't comment intelligently on it. I'm assuming it allows us to specify as specific node handle to spin instead of the generic ros::spinOnce()?

Sounds like you did a good review @v4hn, +1

alainsanguinetti · 2016-07-07T04:37:03Z

@davetcoleman the difference with this call is that
ros::spinOnce() always processes the available callbacks of the global callback queue while
( ( ros::CallbackQueue * ) node_handle_.getCallbackQueue())->callAvailable(); processes the callbacks of the node handle callback queue, which is often the global callback queue.

davetcoleman · 2016-07-07T17:50:58Z

Thanks for cherry-picking!

v4hn · 2016-07-07T18:38:29Z

Crap -.- I'm sorry... Last time I did it without a comment anywhere. Promised.

…t#701)

v4hn reviewed Jul 1, 2016
View reviewed changes

v4hn reviewed Jul 4, 2016
View reviewed changes

alainsanguinetti force-pushed the indigo-devel branch from bebe544 to bf236be Compare July 5, 2016 09:31

v4hn reviewed Jul 5, 2016
View reviewed changes

alainsanguinetti force-pushed the indigo-devel branch from bf236be to 02e7118 Compare July 6, 2016 01:25

alainsanguinetti pushed a commit to alainsanguinetti/moveit_ros that referenced this pull request Jul 6, 2016

Added support for nodes handles with specific callbacks queues

02e7118

pr#701 updated code PR moveit#701 typo fixes and overload for getSharedStateMonitor

Added support for nodes handles with specific callbacks queues

19f52b2

alainsanguinetti force-pushed the indigo-devel branch from 02e7118 to 19f52b2 Compare July 6, 2016 09:51

v4hn merged commit 9479afc into moveit:indigo-devel Jul 7, 2016

v4hn pushed a commit that referenced this pull request Jul 7, 2016

Added support for nodes handles with specific callbacks queues (#701)

9b6832c

v4hn mentioned this pull request Jul 7, 2016

Add nodehandle for specific params #707

Merged

rhaschke mentioned this pull request Jul 15, 2016

fix #442: race conditions when updating PlanningScene state #716

Merged

v4hn mentioned this pull request Aug 22, 2016

Reopen #85: Add capability to execute trajectory with a ROS action moveit/moveit#94

Merged

This was referenced Jan 26, 2017

Creating moveit::planning_interface::MoveGroup crashes when running in nodelet moveit/moveit#425

Closed

move_group.cpp: seg fault bug fix moveit/moveit#426

Merged

otamachan pushed a commit to otamachan/moveit_ros that referenced this pull request Oct 22, 2017

Added support for nodes handles with specific callbacks queues (movei…

1f46c5d

…t#701)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for nodes handles with specific callbacks queues #701

Added support for nodes handles with specific callbacks queues #701

alainsanguinetti commented Jun 29, 2016

v4hn Jul 1, 2016

alainsanguinetti Jul 1, 2016

v4hn commented Jul 1, 2016

alainsanguinetti commented Jul 1, 2016 •

edited

Loading

v4hn Jul 4, 2016

v4hn Jul 4, 2016

alainsanguinetti Jul 5, 2016

v4hn Jul 5, 2016

v4hn commented Jul 4, 2016

alainsanguinetti commented Jul 5, 2016 •

edited

Loading

v4hn commented Jul 5, 2016 via email

alainsanguinetti commented Jul 5, 2016

v4hn Jul 5, 2016

v4hn commented Jul 5, 2016

alainsanguinetti commented Jul 6, 2016

v4hn commented Jul 6, 2016

alainsanguinetti commented Jul 6, 2016

v4hn commented Jul 6, 2016

davetcoleman commented Jul 6, 2016

alainsanguinetti commented Jul 7, 2016

davetcoleman commented Jul 7, 2016

v4hn commented Jul 7, 2016 via email

Added support for nodes handles with specific callbacks queues #701

Added support for nodes handles with specific callbacks queues #701

Conversation

alainsanguinetti commented Jun 29, 2016

v4hn Jul 1, 2016

Choose a reason for hiding this comment

alainsanguinetti Jul 1, 2016

Choose a reason for hiding this comment

v4hn commented Jul 1, 2016

alainsanguinetti commented Jul 1, 2016 • edited Loading

v4hn Jul 4, 2016

Choose a reason for hiding this comment

v4hn Jul 4, 2016

Choose a reason for hiding this comment

alainsanguinetti Jul 5, 2016

Choose a reason for hiding this comment

v4hn Jul 5, 2016

Choose a reason for hiding this comment

v4hn commented Jul 4, 2016

alainsanguinetti commented Jul 5, 2016 • edited Loading

v4hn commented Jul 5, 2016 via email

alainsanguinetti commented Jul 5, 2016

v4hn Jul 5, 2016

Choose a reason for hiding this comment

v4hn commented Jul 5, 2016

alainsanguinetti commented Jul 6, 2016

v4hn commented Jul 6, 2016

alainsanguinetti commented Jul 6, 2016

v4hn commented Jul 6, 2016

davetcoleman commented Jul 6, 2016

alainsanguinetti commented Jul 7, 2016

davetcoleman commented Jul 7, 2016

v4hn commented Jul 7, 2016 via email

alainsanguinetti commented Jul 1, 2016 •

edited

Loading

alainsanguinetti commented Jul 5, 2016 •

edited

Loading