Skip to content

Improve NavigationToPose action feedback#2194

Closed
deepaktalwardt wants to merge 10 commits intoros-navigation:foxy-develfrom
deepaktalwardt:feature/deepak/improve_navigate_to_goal_pose
Closed

Improve NavigationToPose action feedback#2194
deepaktalwardt wants to merge 10 commits intoros-navigation:foxy-develfrom
deepaktalwardt:feature/deepak/improve_navigate_to_goal_pose

Conversation

@deepaktalwardt
Copy link
Copy Markdown
Contributor

@deepaktalwardt deepaktalwardt commented Feb 19, 2021

Basic Info

Info Please fill out this column
Ticket(s) this addresses #2154
Primary OS tested on Ubuntu 20.04
Robotic platform tested on Gazebo Simulation of Turtlebot3

Description of contribution in a few bullet points

  • Improves feedback of the NavigateToPose action server by adding:
    • Better distance to goal estimate: by integrating distance between planned path poses.
    • Estimated time remaining: by dividing the distance remaining by current robot linear speed (using smoothed odometry).
  • Updates the feedback in .action file for NavigateToPose action server.
  • Adds a utility to the nav2_util::geometry_utils to calculate path length.
  • Adds simple unit testing for this function.

Description of documentation updates required from your changes

  • Documentation for .action needs to be updated.

Future work that may be required in bullet points

  • Currently the time remaining largely ignores the target yaw of the robot. It may be the case that the robot needs more time to rotate to the correct heading.

@deepaktalwardt deepaktalwardt mentioned this pull request Feb 19, 2021
Closed
@deepaktalwardt deepaktalwardt changed the title Feature/deepak/improve navigate to goal pose Improve NavigationToPose action feedback Feb 19, 2021
Timple
Timple reviewed Feb 19, 2021
View reviewed changes
Timple
Timple reviewed Feb 19, 2021
View reviewed changes
SteveMacenski
SteveMacenski requested changes Feb 19, 2021
View reviewed changes
@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Feb 19, 2021 •
edited
Loading

Thanks for this - great intro to the stack and I appreciate that you added tests off the bat without me having to ask.

We typically target PRs to main though, not foxy. In ROS2, we maintain usually master/main branches for development that are released into distributions in syncs (unlike in ROS1). This is so that we can make sure we maintain ABI compatibility. This will by synced to Foxy in the next sync, but we should merge this into main so that every distribution moving forward has this work as well!

Also we should add this great new work / ETA feature in our migration guides https://navigation.ros.org/migration/Foxy.html

@deepaktalwardt
Copy link
Copy Markdown
Contributor Author

deepaktalwardt commented Feb 19, 2021 •
edited
Loading

We typically target PRs to main though, not foxy. In ROS2, we maintain usually master/main branches for development that are released into distributions in syncs (unlike in ROS1). This is so that we can make sure we maintain ABI compatibility. This will by synced to Foxy in the next sync, but we should merge this into main so that every distribution moving forward has this work as well!

Ah I wasn't aware! Let's continue the discussion on this PR, once we agreed upon the code I'll merge main in and create a new PR from this branch. Thanks!

SteveMacenski
SteveMacenski reviewed Feb 23, 2021
View reviewed changes
SteveMacenski
SteveMacenski reviewed Feb 23, 2021
View reviewed changes
@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Feb 25, 2021

@deepaktalwardt sounds like the pruning is probably required to cover our bases

@deepaktalwardt
Copy link
Copy Markdown
Contributor Author

deepaktalwardt commented Feb 25, 2021

@deepaktalwardt sounds like the pruning is probably required to cover our bases

@SteveMacenski sounds good. I'll attempt adding that this weekend.

SteveMacenski
SteveMacenski reviewed Mar 2, 2021
View reviewed changes
SteveMacenski
SteveMacenski requested changes Mar 2, 2021
View reviewed changes
SteveMacenski
SteveMacenski approved these changes Mar 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@SteveMacenski SteveMacenski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! I just realized that we changed the ABI on the main navigate to pose action, we can't merge this into foxy but we can into main for Galactic and newer! Make sure to update the migration guide (https://navigation.ros.org/migration/Foxy.html), submit a PR to main and we should be good to go!

@SteveMacenski SteveMacenski changed the base branch from foxy-devel to main March 10, 2021 00:57
@SteveMacenski SteveMacenski changed the base branch from main to foxy-devel March 10, 2021 00:57
@deepaktalwardt deepaktalwardt mentioned this pull request Mar 14, 2021
Merged
@deepaktalwardt
Copy link
Copy Markdown
Contributor Author

deepaktalwardt commented Mar 14, 2021

A PR into main was created here #2246. Closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@doisyg doisyg doisyg left review comments

@SteveMacenski SteveMacenski SteveMacenski approved these changes

+1 more reviewer

@Timple Timple Timple left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@deepaktalwardt @SteveMacenski @Timple @doisyg