Improve bridge command parser

LucasHaug commented 1 year ago

This PR attempts to improve the usability of the dynamic bridge and the parameter bridge by splitting the ROS1 and ROS2 arguments into two separate lists of arguments. This solves the issue #316 and also enables the possibility to set namespaces, node names, remappings, etc, for the ROS1 node and for the ROS2 node, isolatting the ROS1 arguments from the ROS2 argumetns. Additionaly the command parser from the parameter bridge was fixed in order to be more similar to the dynamic bridge.

Besides that, I made some changes to the GitHub action, witch can be discussed, in order to simplify the setup of the environment where the package is built for the action.

MichaelOrlov commented 1 year ago

@clalancette @sloretz @wjwwood This PR hangs out for while (2 weeks) and needs to be reviewed. Could you please advise who would be a right person for review?

methylDragon commented 1 year ago

Hey! Before I do my review, can I double check that this PR is just concerned with splitting the ROS args so there's no case where a command parser mutates a shared list of commands before a second parser reads it?

Also I'm not sure if the workflow changes should be lumped into this PR, it might be better to split it off into its own PR, or is it a required dependency for the rest of the changes here?

LucasHaug commented 1 year ago

Hey! Before I do my review, can I double check that this PR is just concerned with splitting the ROS args so there's no case where a command parser mutates a shared list of commands before a second parser reads it?

Hey thanks for the reply, the idea here was to just split the arguments.

But I changed the dynamic bridge parse_command_options function to remove the flags that are used for the bridge configuration like the --bridge-all-topics before passing the arguments to the ROS init parser.

So basically the idea in both bridges was to, when passing arguments in the command line, use a structure like:

ros2 run ros1_bridge [dynamic_bridge|parameter_bridge] [Bridge specific arguments] [ROS1 arguments] --ros-args [ROS2 arguments]

The bridge specific parameters are removed from the arguments list before passing the list to the split_ros1_ros2_args function. Then just function just considers everything before the --ros-args to be arguments for the ROS1 node, and everything after that to be for the ROS2 node.

Also I'm not sure if the workflow changes should be lumped into this PR, it might be better to split it off into its own PR, or is it a required dependency for the rest of the changes here?

Make sense, I added them here so the bridge could be built in the GitHub Action, but I can open another PR with it.

methylDragon commented 1 year ago

I wonder if we should do this:

ros2 run ros1_bridge \
  [dynamic_bridge|parameter_bridge] [Bridge specific arguments] \
  --ros1-args [ROS1 arguments] \
  --ros2-args [ROS2 arguments]

And also continue to support the current case (but deprecate it with a warning when the ros1-args and ros2-args flags are missing?)

LucasHaug commented 1 year ago

I wonder if we should do this:
ros2 run ros1_bridge \
  [dynamic_bridge|parameter_bridge] [Bridge specific arguments] \
  --ros1-args [ROS1 arguments] \
  --ros2-args [ROS2 arguments]
And also continue to support the current case (but deprecate it with a warning when the ros1-args and ros2-args flags are missing?)

I think is a valid option, I can work on that

methylDragon commented 11 months ago