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

Update dwb_controller as per robot_navigation stack. #1501

Merged
merged 5 commits into from
Feb 11, 2020

Conversation

shivaang12
Copy link
Collaborator


Basic Info

Info Please fill out this column
Ticket(s) this addresses #229
Primary OS tested on Ubuntu 18.04
Robotic platform tested on Turtlebot3

Description of contribution in a few bullet points

  • Ported most of the changes from the robot_navigation repo.

Future work that may be required in bullet points

  • I haven't ported some changes which includes input_params and some minor changes.
  • I am planning to complete this work shortly.
  • Need to know if anyone wants me to port input_params which I think is redundant. But it is just my thoughts.

@shivaang12 shivaang12 requested review from SteveMacenski and crdelsey and removed request for SteveMacenski February 6, 2020 18:41
@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 6, 2020

Can you describe what input params are?

Thanks for this, great stuff. Have you tested this in simulation with the turtlebot? Also, I see you added a few parameters, can you make sure to update those in the nav2_params.yaml and in the website docs.

This is a big help, thanks for your hard work

@shivaang12
Copy link
Collaborator Author

I have tested this on my old repo (which is approx 3 weeks old) as I am having issues with running it on current nav stack (I still working on the issue). It would be helpful if anyone volunteers to check on their system till I fix my system.

Below are the input params
nav_grid - A templatized interface for overlaying a two dimensional grid on the world. (Which has not been incorported in nav2 stack)
start_pose - Start pose.
goal_pose - Goal pose .
velocity - velocity.
which are being published in the new update.

I will update the params nav2_params.yaml.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Looks like a good first run at it.

How has the testing for this been going? Should we update the default parameters?

nav2_core/include/nav2_core/goal_checker.hpp Show resolved Hide resolved
nav2_dwb_controller/dwb_core/src/dwb_local_planner.cpp Outdated Show resolved Hide resolved
nav2_dwb_controller/dwb_core/src/trajectory_utils.cpp Outdated Show resolved Hide resolved
nav2_dwb_controller/dwb_core/src/trajectory_utils.cpp Outdated Show resolved Hide resolved
nav2_dwb_controller/dwb_critics/src/rotate_to_goal.cpp Outdated Show resolved Hide resolved
@SteveMacenski
Copy link
Member

Pull in master, CI is fixed

@shivaang12
Copy link
Collaborator Author

@SteveMacenski Would it okay if I submit new PR for the updates to doc for new params. As my fork do not have "configuration" udpate in "sphinx_doc".

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 10, 2020

please add them in here- its one of those technical debt things that we shouldn’t get in the habit of ignoring. Plus when this goes in, it’ll make the documentation on the website outdated and we should keep that current.

Also, you'll need to pull in current changes in order for CI to run and succeed.

@shivaang12
Copy link
Collaborator Author

Okay! I will have to rebase then.

@SteveMacenski
Copy link
Member

Toggled to retrigger CI

- Add statefull param to params yaml file.
- Add doc of new params.
@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #1501 into master will decrease coverage by 0.12%.
The diff coverage is 27.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   37.47%   37.34%   -0.13%     
==========================================
  Files         226      227       +1     
  Lines       11774    11881     +107     
  Branches     5084     5149      +65     
==========================================
+ Hits         4412     4437      +25     
- Misses       3985     4034      +49     
- Partials     3377     3410      +33
Flag Coverage Δ
#project 37.34% <27.1%> (-0.13%) ⬇️
Impacted Files Coverage Δ
...ns/include/dwb_plugins/limited_accel_generator.hpp 100% <ø> (ø) ⬆️
...controller/dwb_core/include/dwb_core/publisher.hpp 50% <ø> (ø) ⬆️
...dwb_critics/include/dwb_critics/rotate_to_goal.hpp 100% <ø> (ø) ⬆️
...ns/include/dwb_plugins/standard_traj_generator.hpp 100% <ø> (ø) ⬆️
...lugins/include/dwb_plugins/simple_goal_checker.hpp 100% <ø> (ø) ⬆️
...er/dwb_core/include/dwb_core/dwb_local_planner.hpp 100% <ø> (ø) ⬆️
...2_dwb_controller/dwb_plugins/test/goal_checker.cpp 50% <0%> (-0.95%) ⬇️
nav2_core/include/nav2_core/goal_checker.hpp 66.66% <0%> (-33.34%) ⬇️
...2_dwb_controller/dwb_core/src/trajectory_utils.cpp 0% <0%> (ø)
...roller/dwb_plugins/src/limited_accel_generator.cpp 56.25% <100%> (+8.25%) ⬆️
... and 16 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 aaa9790...7f0572e. Read the comment docs.

@SteveMacenski
Copy link
Member

Looks like you have a bunch of linting problems

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

LGTM after linting issues and passes tests

Thanks for your attention to detail

@SteveMacenski
Copy link
Member

Do you notice if this is still true after your updates? #938

@shivaang12
Copy link
Collaborator Author

Haven't noticed this behavior but let me check.

@shivaang12
Copy link
Collaborator Author

Nope! This update does not solve the issue. Robot is still traversing dangerously close to obstacles.

@SteveMacenski
Copy link
Member

Got it.

With these updates should any of the gains be tuned differently? (Ei is control worse or changed from before the update?)

@shivaang12
Copy link
Collaborator Author

shivaang12 commented Feb 11, 2020

This update only address some calculation (algorithmic) bugs/improvements not much on gains.

@SteveMacenski
Copy link
Member

I know. If you change algorithms, that is going to inherently change how the current gains are going to effect the system.

Can you do some testing to show that this either improves controller behavior or at least just doesn't effect it? If it's worse, updating the gains to be at minimum as good as today?

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 11, 2020

I did some testing, I'm happy with the performance

Thanks for your hard work in getting these updates in!

@SteveMacenski SteveMacenski merged commit f3a1381 into ros-navigation:master Feb 11, 2020
@shivaang12
Copy link
Collaborator Author

Cool! Thank you for testing (on my behalf).

savalena pushed a commit to savalena/navigation2 that referenced this pull request Jul 5, 2024
…1501)

* Update dwb_controller as per robot_navigation stack.

* Address PR comments

* Add new dwb params in tunable-params.rst list.

* Add documentation of new params

- Add statefull param to params yaml file.
- Add doc of new params.

* Fix linting and test errors
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.

2 participants