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

Adding new mission profile for row operation #182

Draft
wants to merge 32 commits into
base: humble-dev
Choose a base branch
from

Conversation

GPrathap
Copy link
Contributor

@GPrathap GPrathap commented Feb 9, 2024

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

@GPrathap GPrathap marked this pull request as draft February 9, 2024 10:01
@Iranaphor
Copy link
Contributor

Iranaphor commented Mar 28, 2024

As row operations are a use-case action, rather then a general use of topological navigation, this may be best to not define internally but through supporting systems (i.e. a polytunnel_topological_navigation package) which works alongside toponav.

If you can make the row-operation more generic than I think it sohuld be okay though. Perhaps more of a focus on precision edge-traversal.

Thoughts @marc-hanheide ?

@Iranaphor Iranaphor requested a review from marc-hanheide March 28, 2024 11:48
@Iranaphor Iranaphor self-assigned this Mar 28, 2024
Copy link
Member

@marc-hanheide marc-hanheide left a comment

Choose a reason for hiding this comment

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

I think we must rethink the structure. I see 2 options:

  1. We evolve a plugin infrastructure, allowing specific functions to be loaded dynamically and configurable.
  2. We consider things like the edge_manager (and other managers) as base classes and allow them to be overridden. It still requires some refactoring of the overall structure.

So, I wouldn't want to merge this as the fully correct way forward. For now, to demonstrate the AOC system, we should keep in a separate (aoc) branch or so, but making this the main concept looks not properly thought out yet.

@Iranaphor @GPrathap shall we meet soon and reinvigorate the toponav working group?

self.bt_trees = {}
self.in_row_operation = False
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @Iranaphor that we should NOT have anothing specific about row navigation in topological_navigation. topological_navigation must remain domain-general. If its current structure doesn't allow to embed domain specific navigation modes then we must see how we facilitate this, e.g., through a plugin infrastrcture.


self.update_params_control_server = update_params_control_server
self.current_robot_pose = None
self.odom_sub = self.create_subscription(Odometry, '/odometry/global', self.odom_callback,
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have fixed global topic names (without namespace). They are very application specific. Again, if a special way of treating e.g. progression along an edge then we should have something like a plugin that toponav uses to facilitate this. I sense we need a dedicated meeting to agree on the structure of the new topo. We should schedule a meeting just on that after Easter.

@@ -102,9 +137,17 @@ def get_goal_cancle_error_msg(self, status_code):
return self.ACTIONS.goal_cancle_error_codes[0]

def initialise(self, bt_trees, edge, destination_node, origin_node=None
, action_name=None, package="nav2_msgs.action"):
, action_name=None, package="nav2_msgs.action", in_row_operation=False):
Copy link
Member

Choose a reason for hiding this comment

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

see above, I'm reluctant to have row navigation as a mode within toponav

@marc-hanheide
Copy link
Member

Moving towards a soon-ish new toponav release, where are we this?

@GPrathap
Copy link
Contributor Author

GPrathap commented Nov 4, 2024

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.

4 participants