Added support for nodes handles with specific callbacks queues

[alainsanguinetti]

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]

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]

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]

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]

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]

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]

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]

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...

[alainsanguinetti]

Hopefully this time I got it right!

[v4hn]

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]

@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]

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]

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]

@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]

Thanks for cherry-picking!

[v4hn]

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