search

LCAS / topological_navigation

The topological navigation framework
Apache License 2.0
32 stars 36 forks source link

The dwa param `max_trans_vel` in kinetic has been changed to `max_vel_trans` in melodic. #10

Closed adambinch closed 4 years ago

adambinch commented 4 years ago

The kinetic version appears in the dwa params that can be reconfigured by toponav.

gpdas commented 4 years ago

@Jailander @marc-hanheide This needs a fix (execute_policy has this parameter setting in rasberry).

This parameter is not set in navigate.py for each edge, but is set in execute_policy_server.py.
I am not sure whether there is any reason for this.

We could either not set max_vel_trans or split into master (for melodic) and kinetic-devel branches.

marc-hanheide commented 4 years ago

Can't we just ensure both are set to avoid having to maintain different releases?

gpdas commented 4 years ago

It will throw an error as the parameter max_trans_vel does not exist and configured for reconfig.

  File "/opt/ros/melodic/lib/python2.7/dist-packages/dynamic_reconfigure/client.py", line 185, in update
_configuration                                                                                          
    raise DynamicReconfigureParameterException('don\'t know parameter: %s' % name)
DynamicReconfigureParameterException: don't know parameter: max_trans_vel
marc-hanheide commented 4 years ago

Nothing a try...except block can't handle. I'd still make it so that it's backwards compatible and can be done in one single branch.

gpdas commented 4 years ago

I made a PR with a temp fix. Ideally we shouldn't hardcode the parameters as we are doing now, to make it compatible with different local planners. @adambinch may agree with this, as he had trouble to get teb_local_planner working.

marc-hanheide commented 4 years ago

I made a PR with a temp fix. Ideally we shouldn't hardcode the parameters as we are doing now, to make it compatible with different local planners. @adambinch may agree with this, as he had trouble to get teb_local_planner working.

Very true!