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

add topicname args to navigation launch #137

Conversation

sktometometo
Copy link
Contributor

I have added topic name arguments to navigation launch files so that navigation topics can be remapped to other topics. ( like remapping /odom to /odom_visual )

@cjds cjds requested a review from a team February 21, 2020 18:52
Copy link
Contributor

@cjds cjds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SRTM

Copy link
Contributor

@cjds cjds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SRTM

@@ -11,6 +11,12 @@
<arg name="move_base_include" default="$(find fetch_navigation)/launch/include/move_base.launch.xml" />
<arg name="amcl_include" default="$(find fetch_navigation)/launch/include/amcl.launch.xml" />

<!-- set topics -->
<arg name="scan_topic" default="base_scan" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove tab

@@ -11,6 +11,12 @@
<arg name="move_base_include" default="$(find fetch_navigation)/launch/include/move_base.launch.xml" />
<arg name="amcl_include" default="$(find fetch_navigation)/launch/include/amcl.launch.xml" />

<!-- set topics -->
<arg name="scan_topic" default="base_scan" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove tab

@erelson
Copy link
Collaborator

erelson commented Mar 2, 2020

A question for others in general (incl. @cjds @narora1 ): Does it make sense to be parameterizing everything in the base launch files?

Where I'm coming from: My impression is that the launch files originally captured best practices (5 years ago), but I'm not personally familiar with other robots' equivalent launch files or changes in practice in the past 5 years. I am wondering if these changes (including similar in previous PRs) match the spirit of current best/common practices.

That said, this generally LGTM 👍 . I just could imagine a fully parameterized launch file being a bit less readable...

@mikeferguson
Copy link
Contributor

@erelson I think it makes sense that as you have more people doing more use-cases, you'll end up with a few more parameters. I can certainly understand scan and odom getting parameterized (since you might install a better/different set of sensors).

Parameterizing the default map seems kind of unnecessary (I'm not aware of any other robot platform that uses a default map not named "map" - it's pretty much the standard). Same goes for cmd_vel - while different robots may have a different name for this, the entire fetch/freight research system uses a consistent naming convention throughout the real robot config (that's apps like nav here, as well as drivers/bringup) and the simulation.

@knorth55
Copy link
Contributor

In our lab, map can be changed when a robot moves to different level and manages each level map as /map/1f, /map/2f and so on.
Also, cmd_vel_topic can be changed when we make base controller to do motion planning with base.

@knorth55
Copy link
Contributor

knorth55 commented Apr 4, 2020

kindly ping.
can we need to modify this PR? or can you merge this PR?

@erelson erelson merged commit f384288 into ZebraDevs:indigo-devel Apr 5, 2020
@erelson
Copy link
Collaborator

erelson commented Apr 5, 2020

Thanks for the ping. Merged both indigo and melodic PRs 👍

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

Successfully merging this pull request may close these issues.

6 participants