Skip to content

Add support for docking through nav2 #215

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aionescu-cpr
Copy link

New feature implementation

Implemented feature

This change adds docking support using Nav2 as requested here: #187

Implementation description

The main logic change is done in the Nav2RobotAdapter's navigate function, where rather than only sending a robot to a destination, a check was added to determine whether a dock was specified in the destination. If a dock was specified then the robot is sent a DockRobot action goal, otherwise the robot is given the default action of NavigateToPose.

As part of the change, the ros2_types.py was modified to add translation for the related DockRobot_* action classes. The default zenoh config was also modified to allow the .*/dock_robot action topics. A nav2send_dock_robot.py example was also added to allow the zenoh bridge to be tested.

The large change I introduced is a refactoring of the Nav2RobotAdapter as the "navigate_to_pose" and "dock_robot" actions share a lot of the same logic. Rather than duplicate a lot of the existing code and make the adapter larger, I added a NavigationExecutionItem which encapsulates the request being executed, with two classes inheriting from this for each action: NavigateToPoseExecutionItem and DockExecutionItem. This class is responsible for executing the goal, providing updates and stopping it. The handling of these functions were previously all in the Nav2RobotAdapter class, which was simplified to now focus on mainly the control logic.

This is a large change and it may not be in the direction that the maintainers of the repo want the design to go into, so I understand if a simpler approach is requested.

GenAI Use

We follow OSRA's policy on GenAI tools

  • I used a GenAI tool in this PR.
  • I did not use GenAI

Generated-by:

Signed-off-by: Andrei Ionescu <aionescu@clearpath.ai>
@mxgrey mxgrey added this to PMC Board Aug 18, 2025
@github-project-automation github-project-automation bot moved this to Inbox in PMC Board Aug 18, 2025
@aionescu-cpr aionescu-cpr marked this pull request as draft August 18, 2025 22:06
@aaronchongth aaronchongth marked this pull request as ready for review August 19, 2025 01:50
@aaronchongth aaronchongth marked this pull request as draft August 19, 2025 01:50
@aaronchongth
Copy link
Member

Ignore the change from draft to ready-to-review, I misclicked, I've reverted it.

...


class NavigationExecutionItem(ExecutionItem):
Copy link
Member

Choose a reason for hiding this comment

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

This is great! We can use this for other ROS 2 actions in the future, not just for navigation.
Perhaps we can name it to target ROS 2 actions more generically, what do you think about something along the lines of Ros2ActionInterface?

)


class NavigateToPoseExecutionItem(NavigationExecutionItem):
Copy link
Member

Choose a reason for hiding this comment

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

We can then perhaps call this more explicitly as NavigateToPoseActionInterface

return NavigateToPose_GetResult_Response.deserialize(payload)


class DockExecutionItem(NavigationExecutionItem):
Copy link
Member

Choose a reason for hiding this comment

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

and this along the lines of DockActionInterface

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @aionescu-cpr! Overall the changes to the abstractions make sense, I like that this opens the door to more configurable behaviors, allowing the use of other BTs than NavigateToPose.

We can probably give more thought to the naming, to prevent confusion with the existing ExecutionHandle. I've added some comments, but overall it looks good.

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

Successfully merging this pull request may close these issues.

2 participants