Skip to content

[WIP/For discussion]: Including parameters in python based launch file#209

Closed
JafarAbdi wants to merge 1 commit intomoveit:mainfrom
JafarAbdi:pr-port_launch_files
Closed

[WIP/For discussion]: Including parameters in python based launch file#209
JafarAbdi wants to merge 1 commit intomoveit:mainfrom
JafarAbdi:pr-port_launch_files

Conversation

@JafarAbdi
Copy link
Copy Markdown
Member

Description

Right now to launch a MoveIt 2 node we will list all the parameters manually example, this's a proposal to do something similar to ROS example

This's based on this commit and this package

I did try to declare the args in the other launch files using DeclareLaunchArgument and include them with PythonLaunchDescriptionSource/IncludeLaunchDescription and propagate the arguments with get_launch_arguments() but the solution was complicated and even it didn't work with arguments with non-default values

I Opened a PR rather than an issue just so you can easily see the diff for this package

The naming and the structure of moveit_helpers need some changes, this's just a draft

@mikeferguson
Copy link
Copy Markdown
Contributor

I like how much cleaner things get - I wonder if it makes sense to try and upstream some of these utilities into ros2_launch itself - seems every project is creating some version of these in their ROS2 repo.

@henningkayser
Copy link
Copy Markdown
Member

@JafarAbdi thanks for starting this! It really looks much cleaner and I like the approach in general. My suggestion would be to use a specific name and structure convention for pure parameter files, meaning the old xml.launch files should not be confused with launch.py files that actually construct a LaunchDescription and run the nodes. I'm thinking something like exposing a function load_parameters that you can than map into your desired namespace or node.

@henningkayser henningkayser changed the base branch from master to main September 4, 2020 12:40
Copy link
Copy Markdown
Contributor

@DLu DLu left a comment

Choose a reason for hiding this comment

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

  1. We should open a ticket upstream to get the MoveIt! helpers merged into ROS 2 Launch.
  2. This currently has conflicts, but otherwise looks good.

@DLu
Copy link
Copy Markdown
Contributor

DLu commented May 4, 2021

Also, are there plans to release moveit_helpers?

Copy link
Copy Markdown
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

This seems like a no-brainer code cleanup PR, just needs conflicts resolved.

except EnvironmentError: # parent of IOError, OSError *and* WindowsError where available
return None

from moveit_helpers.get_parameters_module import get_parameters_module, load_file, load_yaml
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@DLu is wondering if this should be a standard helping function in ros2 launch, vs a moveit_helper function

@henningkayser
Copy link
Copy Markdown
Member

Also, are there plans to release moveit_helpers?

Not sure if this really makes a lot of sense at this point. While this PR includes some nice cleanup attempts, I'm not convinced that this is a good long-term solution. It doesn't help a lot with reducing launch file redundancy and we should probably rather use the native way of loading yaml files instead of load_yaml().

@JafarAbdi
Copy link
Copy Markdown
Member Author

Replaced by #591

@JafarAbdi JafarAbdi closed this Aug 3, 2021
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.

5 participants