adding strands_navfn to update goal tolerance before generating global plans

[bfalacerda]

This is my proposal to solve https://github.com/strands-project/strands_navigation/issues/187, as I described there.

It adds a new strands_navfn package which is simply a copy of the navfn package with strands_* prefixes added everywhere to avoid conflicts with navfn, plus checking the tolerance param before making plans. The only real code changes are in commit https://github.com/strands-project/strands_movebase/commit/cf3b8a3aac4e13221e9227992d56063f97438133.

It also updates the move_base configuration yaml to use strands_navfn as the global planner.

I will make a PR upstream with the changes in https://github.com/strands-project/strands_movebase/commit/cf3b8a3aac4e13221e9227992d56063f97438133 but that will probably take some time to be merged so I think we're better off using the new package for now.

If this is merged, then we just need to have topological navigation checking if the target node of the edge to be executed is an intermediary waypoint in the route plan, and if so change the default_tolerance param according to the influence area of the target node.

[bfalacerda]

Also, are the changes in https://github.com/strands-project/strands_movebase/commit/cf3b8a3aac4e13221e9227992d56063f97438133 the proper way to get the params in cpp?

[marc-hanheide]

still DDoS?

[marc-hanheide]

retest this please

[marc-hanheide]

OK, so now we have a proper failed build:

[ 87%] Building CXX object monitored_vcs/strands_navfn/test/CMakeFiles/strands_path_calc_test.dir/strands_path_calc_test.cpp.o
/var/lib/jenkins/workspace/pr-indigo-strands_movebase/monitored_vcs/strands_navfn/test/strands_path_calc_test.cpp:57:45: error: variable or field 'print_neighborhood_of_last_path_entry' declared void
 void print_neighborhood_of_last_path_entry( navfn::NavFn* nav )  
                                             ^
/var/lib/jenkins/workspace/pr-indigo-strands_movebase/monitored_vcs/strands_navfn/test/strands_path_calc_test.cpp:57:45: error: 'navfn' has not been declared
/var/lib/jenkins/workspace/pr-indigo-strands_movebase/monitored_vcs/strands_navfn/test/strands_path_calc_test.cpp:57:59: error: 'nav' was not declared in this scope
 void print_neighborhood_of_last_path_entry( navfn::NavFn* nav )  

[marc-hanheide]

9877376 is much appreciated, I'm sure. @Jailander @cdondrup @santosj @gestom can we give this a try on Linda, please, before we merge it?

On question: Wouldn't it in this case better to use dynamic_reconfigure?

[marc-hanheide]

I'll assign to @Jailander for now to report test results.

[cdondrup]

I just wanted to try something on Linda any way. I can just run this as well. I assume, I just pull it and it runs the strands_navfn instead of the normal one?

[bfalacerda]

there wont be any noticeable changes in the robot behaviour unless you play with the /move_base/NavfnROS/default_tolerance param.

Regarding dynamic_reconfigure, imo it's an overkill in many cases. For this case, it is much easier to just read from the param server in certain places, instead of adding the whole dynamic reconfigure infrastructure. We could do it to put it in tune with the rest of move_base, but that's already too much cpp for me

[bfalacerda]

@cdondrup yes, it's just pulling and running strands_movebase.launch. It's already using strands_navfn as the global planner.

[cdondrup]

If the navigation is started without the chest can, it doesn't load the parameter files from strands_movebase.

Otherwise it seems to work like before. Can't really make a more intelligent statement since I don't know what is supposed to be different.

[marc-hanheide]

Yes, these two places for parameters are very annoying... @bfalacerda any suggestion?

[bfalacerda]

Why are we pulling params from scitos_2d_nav?

[Jailander]

https://github.com/strands-project/scitos_2d_navigation/issues/55

[bfalacerda]

ok, i understand why we have the scits_2d_nav repo, just to keep a place with vanilla scitos g5 params, what I don't understand is why we are pulling params from there. I would have the repos completely separated instead, and have someone (I dont mind doing it, and there's an issue open there for it already) in charge of updating scitos_2d_nav to mirror strands_movebase once in a while

[Jailander]

yes I agree the main parameters should be in strands movebase I don't mind doing it either

[bfalacerda]

@nilsbore does this make sense? Would we have to copy stuff from scitos_2d_nav to here?

[nilsbore]

@bfalacerda Yes, we would have to move the parameters for navigating without the chest camera from scitos_2d_navigation. The amcl configuration is also in scitos_2d_navigation, not sure if that should be moved as well.

[nilsbore]

In general though, do we really want to have the option to navigate without the chest camera given what happened in Vienna? We should at least add a warning for when it is not enabled or when there is nothing published on /chest_xtion/depth/points.

[nilsbore]

In effect it is two whole movebase configurations (voxel vs grid) that we have to maintain while we really only want to use one of them.

[cdondrup]

I guess we mainly want to keep the 2D one for people having a scitos robot without a chest cam who are not from the project.

[Jailander]

and simulation

[nilsbore]

Yeah, simulation is a good point. We should add a chest camera there whenever that get's possible.

[Jailander]

yes we should, however, we should also keep the 2D nav, I can't imagine simulating 3D obstacle avoidance and people perception for example working on every PC

[bfalacerda]

cant we usel voxel costmaps regardless of the chest cam being used or not? If we do that, we just need to add more sensor sources when chest_camera is set in the launch file, otherwise we launch it like in sictos_2d_nav but with a voxel configuration?

[nilsbore]

@bfalacerda Yes, that's probably a good option.

[bfalacerda]

cool. I'll open an issue as this was a huge digression from this particular PR. Is this mergeable @cdondrup ?

[marc-hanheide]

Another problem: We need to make strands_movebase/package.xml depend on this, as otherwise the Debian deps will be wrong.

[marc-hanheide]

please address the above before merging

[cdondrup]

Yes, please merge after the making the changes @marc-hanheide suggested.