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

Put hybrid planning actions under a common namespace #932

Merged
merged 11 commits into from
Jan 2, 2022

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Dec 20, 2021

This groups the hybrid planning action names together so they are easy to find, for example, with ros2 action list.

We're starting to use hybrid planning with a dual arm setup, so cleaning up the interface is becoming more important to me.

Test with the usual hybrid planning demo:

ros2 launch moveit_hybrid_planning hybrid_planning_demo.launch.py

@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #932 (7919f4b) into main (71d6e36) will decrease coverage by 0.02%.
The diff coverage is 71.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
- Coverage   57.91%   57.90%   -0.01%     
==========================================
  Files         310      310              
  Lines       26069    26091      +22     
==========================================
+ Hits        15095    15105      +10     
- Misses      10974    10986      +12     
Impacted Files Coverage Δ
..._manager_component/src/hybrid_planning_manager.cpp 24.05% <68.43%> (+3.22%) ⬆️
...planner_component/src/global_planner_component.cpp 42.00% <71.43%> (+3.37%) ⬆️
...ude/moveit/local_planner/local_planner_component.h 83.34% <100.00%> (+0.73%) ⬆️
..._planner_component/src/local_planner_component.cpp 33.11% <100.00%> (ø)
...ma_kinematics_plugin/src/lma_kinematics_plugin.cpp 72.91% <0.00%> (-3.87%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 53.19% <0.00%> (-0.11%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.93% <0.00%> (+1.12%) ⬆️

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 71d6e36...7919f4b. Read the comment docs.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Makes a lot of sense to use a common namespace here. Using private topics like suggested in the comment is really an option that we should consider, but not strictly necessary here

@moveit moveit deleted a comment from mergify bot Dec 27, 2021
Copy link
Member

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

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

GLOBAL_PLANNING_ACTION_NAME, LOCAL_PLANNING_ACTION_NAME, and RUN_HYBRID_PLANNING_ACTION_NAME are being duplicated in different files, should we move them to a new header file and include that file ?

@henningkayser
Copy link
Member

GLOBAL_PLANNING_ACTION_NAME, LOCAL_PLANNING_ACTION_NAME, and RUN_HYBRID_PLANNING_ACTION_NAME are being duplicated in different files, should we move them to a new header file and include that file ?

that would make a lot of sense

@AndyZe
Copy link
Member Author

AndyZe commented Dec 30, 2021

Where would you suggest putting that header file? It doesn't belong in any of the folders that exist right now: global_planner, hybrid_planning_manager, or local_planner

@henningkayser
Copy link
Member

Where would you suggest putting that header file? It doesn't belong in any of the folders that exist right now: global_planner, hybrid_planning_manager, or local_planner

good question, the directory structure doesn't really allow common resources. Thinking about this more, it could make sense to move the directories into top-level src and include and move common headers into include. Here, the only alternative I see is to add a new subdirectory for the same purpose

@AndyZe
Copy link
Member Author

AndyZe commented Dec 30, 2021

OK, I put the namespace stuff in a common header file. I find the ~ does not work, though. However, everything works if I delete the ~.

namespace moveit::hybrid_planning
{
namespace
{
constexpr char GLOBAL_PLANNING_ACTION_NAME[] = "~/hybrid_planning/global_planning_action";
constexpr char LOCAL_PLANNING_ACTION_NAME[] = "~/hybrid_planning/local_planning_action";
constexpr char RUN_HYBRID_PLANNING_ACTION_NAME[] = "~/hybrid_planning/run_hybrid_planning";
}  // namespace
}  // namespace moveit::hybrid_planning

Co-authored-by: Jafar Abdi <cafer.abdi@gmail.com>
@AndyZe
Copy link
Member Author

AndyZe commented Dec 31, 2021

The ~ causes this: (ros2 action list)

/global_planner/hybrid_planning/global_planning_action
/hybrid_planning_demo_node/hybrid_planning/run_hybrid_planning
/hybrid_planning_manager/hybrid_planning/local_planning_action
/local_planner/hybrid_planning/local_planning_action

So I'm going to suggest we pass an action_namespace parameter from the launch files.

@AndyZe AndyZe force-pushed the andyz/hybrid_planning_namespaces branch from 021fe79 to 7aada5b Compare December 31, 2021 16:14
@AndyZe
Copy link
Member Author

AndyZe commented Dec 31, 2021

OK, I moved the action name parameters to a common config file. I think this is nice, it fits well with the other config files that already exist:

global_planner.yaml
local_planner.yaml
hybrid_planning_manager.yaml
common_hybrid_planning_params.yaml --> the new one

@AndyZe AndyZe merged commit 6141e3a into moveit:main Jan 2, 2022
@AndyZe AndyZe mentioned this pull request Jan 4, 2022
AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Jan 4, 2022
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.

None yet

3 participants