cleanup all globally hardcoded remaps for multirobots by SteveMacenski · Pull Request #1214 · ros-navigation/navigation2

SteveMacenski · 2019-10-10T18:49:07Z

Fixes #746

This PR cleans up all the hardcoded global scope topics across the stack so were not unnecessarily remapping basic topics.

Passes tests but I think @orduno is in the best position to give this the thumbs up that it resolves all the issues he needed the remaps for.

Now the remaps are just for things genuinely needed (TF) where there isn't (yet) an option for a prefix

SteveMacenski · 2019-10-10T18:51:00Z

Also @orduno I see you have a multirobot system test PR, you're more than welcome to pull my commit into that branch and I can close this PR. It was just a quick cleanup job

codecov · 2019-10-10T20:11:30Z

Codecov Report

Merging #1214 into master will decrease coverage by 0.79%.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           master   #1214     +/-   ##
========================================
- Coverage      39%   38.2%   -0.8%     
========================================
  Files         244     244             
  Lines       11033   11033             
  Branches     4565    4565             
========================================
- Hits         4303    4215     -88     
- Misses       3874    3990    +116     
+ Partials     2856    2828     -28

Flag	Coverage Δ
#project	`38.2% <0%> (-0.8%)`	⬇️

Impacted Files	Coverage Δ
nav2_bt_navigator/src/bt_navigator.cpp	`20.83% <ø> (ø)`	⬆️
...map_2d/test/integration/test_collision_checker.cpp	`51.33% <0%> (ø)`	⬆️
...av2_costmap_2d/test/integration/obstacle_tests.cpp	`33.33% <0%> (ø)`	⬆️
..._tests/src/localization/test_localization_node.cpp	`68% <0%> (ø)`	⬆️
nav2_costmap_2d/src/costmap_2d_ros.cpp	`22.22% <0%> (ø)`	⬆️
nav2_controller/src/nav2_controller.cpp	`22.05% <0%> (ø)`	⬆️
...v2_costmap_2d/test/integration/inflation_tests.cpp	`29.35% <0%> (ø)`	⬆️
...cpp/nav2_msgs/srv/clear_entire_costmap__struct.hpp	`0% <0%> (-100%)`	⬇️
...dl_generator_cpp/nav2_msgs/action/wait__struct.hpp	`9.52% <0%> (-57.15%)`	⬇️
...dl_generator_cpp/nav2_msgs/action/spin__struct.hpp	`9.09% <0%> (-54.55%)`	⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd239d6...45fdac6. Read the comment docs.

nav2_bringup/bringup/launch/nav2_tb3_simulation_launch.py

nav2_bringup/bringup/params/nav2_params.yaml

orduno · 2019-10-16T22:29:18Z

Thanks for making these changes Steve.

I pulled your branch and applied #1243 on top. Tests pass! (however, we're not testing "dynamic" obstacles -- see above)

Here's the branch in case you're interested.

Besides my comment above, all looks good.

SteveMacenski · 2019-10-16T23:36:31Z

Seems to me best action is to have the / added back in front of the configuration file as a configuration file is meant to configure for a specific use case. For the case of multirobot, I think its reasonable to have an overloading configuration file with the total namespace for these nodes, ei nav_params.yaml and nav_params_multirobot.yaml

…into multi_cleanup

SteveMacenski · 2019-10-23T20:09:21Z

@crdelsey @orduno can I get reviews?

orduno · 2019-10-23T23:47:29Z

For the case of multirobot, I think its reasonable to have an overloading configuration file with the total namespace for these nodes, ei nav_params.yaml and nav_params_multirobot.yaml

Are you planning on adding this to this PR?

SteveMacenski · 2019-10-24T00:35:22Z

I can. What namespace(s) would you like?

orduno · 2019-10-24T16:42:36Z

<robot_namespace>/scan to match what we have on nav2_namespaced_view.rviz

SteveMacenski · 2019-10-24T17:01:17Z

What I'm asking is what is that <robot_namspace> this is a yaml file so you need to specify a specific namespace string for me to use

orduno · 2019-10-24T17:12:29Z

we'll need one per robot, so robot1/scan and robot2/scan

SteveMacenski · 2019-10-24T17:19:33Z

I think you mean /robot1/... so that we're going out to the global namespace?

SteveMacenski · 2019-10-24T17:29:30Z

@orduno done.

This also has the added benefit of allowing a user to launch multiple heterogenerous (ei completely different robots) if they require unique configuration files.

I usually hate eval but this is a sweet sweet use of it

Edit: Forgot to update the multirobot tests to do this -- one moment

SteveMacenski · 2019-10-25T03:13:05Z

This PR should be squash merged, battling small python flake issues that only appear in CI. I'll force push a squashed last commit from this garbage but in general this is a small change and shouldn't need more than 1

…ti_cleanup

SteveMacenski · 2019-10-25T21:07:27Z

@orduno this is ready again for review.

I'm not seeing anything subscribed/published to the global /scan. The test is failing for me locally from signal failure drops in the costmaps, but otherwise it seems fine (for what little that means). I see travis succeeded but it uses the run test suite script that doesn't include multirobot. I added the multirobot test back to travis to see if it passes off my machine. The multirobot CircleCI is failing for the spawn_robot bug so its hard for me to know that this isworking.

So this is probably going to have to sit here until the spawn_robot CI bug is fixed or I can get the stack running on my computer again. That's fine

SteveMacenski · 2019-10-25T21:51:58Z

Travis also fails on the spawn_robot problem, reverted that commit.

Edit: Thanks to @crdelsey that's all handled now, passing tests 👍

…ti_cleanup

SteveMacenski · 2019-10-28T23:34:21Z

Can I get a couple reviews? I'm passing the multi-robot CI & I'd love it if I didn't have to fix another 5 merge conflicts on this one PR 😿

ghost · 2019-10-30T20:34:14Z

nav2_bringup/bringup/launch/nav2_multi_tb3_simulation_launch.py

            source_file=rviz_config_file,
            replacements={'<robot_namespace>': ('/' + robot['name'])})

+        params_file = eval(robot['name'] + '_params_file')


Can we change this to:

params_file = LaunchConfiguration(robot['name'] + '_params_file')

Then we can get rid of the LaunchConfiguration lines above.

Maybe it's just me, but I find the use of eval here difficult to read.

So on first glance I agree with you, but something that came to mind was that since we're doing this eval in a loop, that if someone changes 2 robots to N robots and there aren't N parameter files, you're not going to be able to statically look at the initialization of the launch configurations and see that there are things missing.

If you don't think that's a real problem, I can do that -- I'm just thinking and if there's a bug, some analyzer isn't going to catch that.

…into multi_cleanup

SteveMacenski · 2019-11-03T11:37:55Z

Build breaking from branching issue, given it was passing and this is a 1 line change, I'm inclined to push this

…#1214) * cleanup all globally hardcoded remaps for multirobots * fixing map path * fix pep8 indent * linter happy now? * Fix linter errors * reincluding /scan on scan * multirobot launch with unique configurations * multirobot tests using both configuration files * make flake8 happy * flake8 try2 * flakey9 * flake10 * attempt flake exception * flake11 * flake12 * noqa on lin * changing launch configuration in loop

…er controllers (ros-navigation#1214)

cleanup all globally hardcoded remaps for multirobots

8f0b460

SteveMacenski requested a review from orduno October 10, 2019 18:49

fixing map path

aecdd12

Merge branch 'master' into multi_cleanup

4bc4dc9

SteveMacenski requested review from bpwilcox and mhpanah October 15, 2019 21:24

fix pep8 indent

84ae766

ghost reviewed Oct 15, 2019

View reviewed changes

nav2_bringup/bringup/launch/nav2_tb3_simulation_launch.py Outdated Show resolved Hide resolved

nav2_bringup/bringup/params/nav2_params.yaml Outdated Show resolved Hide resolved

SteveMacenski and others added 2 commits October 15, 2019 14:53

linter happy now?

5d0aa72

Fix linter errors

29c3fed

SteveMacenski added 2 commits October 23, 2019 13:08

reincluding /scan on scan

03e4a59

Merge branch 'multi_cleanup' of github.com:stevemacenski/navigation2 …

4ba5e55

…into multi_cleanup

merge master

3888258

multirobot launch with unique configurations

3c57c07

SteveMacenski force-pushed the multi_cleanup branch from e64703a to 3c57c07 Compare October 24, 2019 17:31

SteveMacenski added 3 commits October 24, 2019 10:35

multirobot tests using both configuration files

fff4d67

make flake8 happy

6154be4

flake8 try2

73ea1e4

SteveMacenski added 3 commits October 24, 2019 21:50

flakey9

bddb25f

flake10

ce1ee1c

attempt flake exception

26ac5be

SteveMacenski force-pushed the multi_cleanup branch from 4ca636e to 26ac5be Compare October 25, 2019 16:42

SteveMacenski added 4 commits October 25, 2019 10:05

flake11

19169b5

Merge branch 'master' of github.com:ros-planning/navigation2 into mul…

17f9573

…ti_cleanup

flake12

fd06401

noqa on lin

83155d0

SteveMacenski force-pushed the multi_cleanup branch from c835b4c to 83155d0 Compare October 25, 2019 22:01

SteveMacenski added 2 commits October 25, 2019 22:25

Merge branch 'master' of github.com:ros-planning/navigation2 into mul…

11fdc90

…ti_cleanup

Merge branch 'master' into multi_cleanup

d2d5881

orduno approved these changes Oct 29, 2019

View reviewed changes

ghost reviewed Oct 30, 2019

View reviewed changes

ghost approved these changes Oct 30, 2019

View reviewed changes

SteveMacenski added 2 commits November 3, 2019 11:26

changing launch configuration in loop

7ca68ff

Merge branch 'multi_cleanup' of github.com:stevemacenski/navigation2 …

45fdac6

…into multi_cleanup

SteveMacenski merged commit a227539 into ros-navigation:master Nov 3, 2019

Forsyth-Creations pushed a commit to Forsyth-Creations/navigation2 that referenced this pull request Feb 19, 2025

[PID Controller] Export state interfaces for easier chaining with oth…

a7b2af5

…er controllers (ros-navigation#1214)

Conversation

SteveMacenski commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Oct 10, 2019

Uh oh!

codecov bot commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

orduno commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Oct 16, 2019

Uh oh!

SteveMacenski commented Oct 23, 2019

Uh oh!

orduno commented Oct 23, 2019

Uh oh!

SteveMacenski commented Oct 24, 2019

Uh oh!

orduno commented Oct 24, 2019

Uh oh!

SteveMacenski commented Oct 24, 2019

Uh oh!

orduno commented Oct 24, 2019

Uh oh!

SteveMacenski commented Oct 24, 2019

Uh oh!

SteveMacenski commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveMacenski commented Oct 28, 2019

Uh oh!

ghost Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

SteveMacenski commented Nov 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SteveMacenski commented Oct 10, 2019 •

edited

Loading

codecov bot commented Oct 10, 2019 •

edited

Loading

orduno commented Oct 16, 2019 •

edited

Loading

SteveMacenski commented Oct 24, 2019 •

edited

Loading

SteveMacenski commented Oct 25, 2019 •

edited

Loading

SteveMacenski commented Oct 25, 2019 •

edited

Loading

SteveMacenski commented Oct 25, 2019 •

edited

Loading