Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: why are the controller, recovery behavior and planner bundled in one node? #15

Closed
reinzor opened this issue Nov 28, 2017 · 10 comments
Labels

Comments

@reinzor
Copy link
Contributor

reinzor commented Nov 28, 2017

What is the benefit of having one server that exposes these three functionalities instead of having three (or n) servers exposing these separate functionalities?

@corot
Copy link
Collaborator

corot commented Nov 28, 2017

Main is to share between all the plugins a very expensive resource as are the costmaps (we use several recovery behaviors that use them).

Secondary is to provide backward compatibility: in a single node we can just copy paste (well, almost) the code of the current move_base action that do all together, plan, control and recovery. True we could have done an external FSM doing the same and offering this same move_base action, but first approach was simpler and faster.

But anyway, I don't see any significant advantage on having separated nodes. The current single-node code is well organized so it's not more difficult to handle that separated nodes.

@corot corot added the question label Nov 28, 2017
@reinzor
Copy link
Contributor Author

reinzor commented Nov 28, 2017

Thanks for your quick comment. I see the benefit of sharing these resources.

@reinzor reinzor closed this as completed Nov 28, 2017
@reinzor reinzor reopened this Nov 28, 2017
@reinzor
Copy link
Contributor Author

reinzor commented Nov 28, 2017

I was browsing a little bit more through your code and I don't understand your reasoning for the following. You mention in your README.md the following: "This interface allows external executives, e.g. SMACH, or Behavior Trees, to run highly flexible and complex navigation strategies.". Why would you then still define the MoveBase.action in your interfaces and implement the state machines in the abstract server:

https://github.com/magazino/move_base_flex/blob/c8a6eff839b5520bad7bab9d6bfcd3bcd9f97ce3/move_base_flex/include/move_base_flex/abstract_server/impl/abstract_navigation_server.tcc#L745

Why not remove this action from the ROS interfaces and implement the abstract_navigation_server statemachine (that does the move base state machine behavior) in the implementation specific version of the navigation server? So the mbf_costmap_2d_nav.

@corot
Copy link
Collaborator

corot commented Nov 28, 2017

Perfectly legitimate request; tracked on #16

@reinzor
Copy link
Contributor Author

reinzor commented Nov 28, 2017

Ok, I will close this issue. Thanks for being so responsive!

@reinzor reinzor closed this as completed Nov 28, 2017
@corot
Copy link
Collaborator

corot commented Nov 28, 2017

Welcome; but don't be surprised; we are now on a rush to settle most open issues before releasing

@reinzor
Copy link
Contributor Author

reinzor commented Nov 28, 2017

I would like to contribute to get involved with the development, are you open for pull requests of do you have contribution guidelines?

@corot
Copy link
Collaborator

corot commented Nov 28, 2017

sure, and not yet

but note that all development is now going on on advanced_naming branch, and it will soon change again with all the changes we have discussed so far.

Any particular feature you are interested to contribute? Will be happy to review and integrate any contribution! 😬 👍

@reinzor
Copy link
Contributor Author

reinzor commented Nov 28, 2017

I think your vision of MBF is great but I see that some ideas are not that clearly implemented and/or documented. So maybe it's good that you share your TODO list or plan so I can help you working on a more stable release?

@corot
Copy link
Collaborator

corot commented Nov 28, 2017

A roadmap is already in the repo README, at the bottom. Feel free to propose other features!

For bugs and necessary short-term improvements we have the issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants