-
Notifications
You must be signed in to change notification settings - Fork 914
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
roslaunch - pass through command-line args to the xmlloader when using the API #1115
Conversation
This feature is really needed, I hope it gets merged soon! |
@@ -407,14 +407,16 @@ def _select_machine(self, node): | |||
# assign to local machine | |||
return self.machines[''] | |||
|
|||
def load_config_default(roslaunch_files, port, roslaunch_strs=None, loader=None, verbose=False, assign_machines=True): | |||
def load_config_default(roslaunch_files, port, roslaunch_args=None, roslaunch_strs=None, loader=None, verbose=False, assign_machines=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add new arguments at the end of the signature since existing code might call keyword arguments as positional which would break.
Same below.
Fixed position of new arguments to be at the end of the signatures. |
@@ -445,10 +448,11 @@ def load_config_default(roslaunch_files, port, roslaunch_strs=None, loader=None, | |||
load_roscore(loader, config, verbose=verbose) | |||
|
|||
# load the roslaunch_files into the config | |||
for f in roslaunch_files: | |||
for i, f in enumerate(roslaunch_files): | |||
args = roslaunch_args[i] if roslaunch_args else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this requires roslaunch_args
to be either None
or at least have the same length as roslaunch_files
. It would be good to support the usage where roslaunch_args
contains less items than roslaunch_files
since a user might only pass fewer roslaunch_args
since the other launch files don't need arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the interface is not very pretty or ideal, but I'm having trouble seeing any way to determine which roslaunch file should get an arg list apart from 2 equal-length lists. Otherwise, if the user gives multiple launch files, but only one arg list, how do we decide which file to give the list to? Can we just document that any launch files w/o args must pass in a None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.:
ROSLaunchParent(uuid, [file1, file2], roslaunch_args=[['args', 'for', 'file1'], None])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps we pass that args in as a dictionary with roslaunch filenames as keys and arglists as values? That would remove all ambiguity... And dict.get
already plays nice by defaulting to None
for nonexistent keys. I'll work on that tack and see how the interface feels.
Update:
Didn't consider the fact that the dictionary would not allow the user to launch the same file multiple times with different arguments if using the filename as a key. Unfortunately, this is one of the main use cases I foresee. If launching a swarm of robots, one might want to launch the same instance with e.g. different namespaces as arguments. Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just allowing the roslaunch_args
list to be shorter. That avoids that the user needs to append None
for launch files at the end which don't have arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an option. What are your thoughts on my later commits using a dictionary? Seems to give the best of both worlds to me, but maybe I've got blinders on. I can revert those commits and go with this instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two lists which are correlated by their index is certainly not very elegant.
But with the current dict I do see two downsides:
-
the repetition of the launch file
-
the changed semantic when using arguments for launch files which appear more than once
- Before calling it with
[foo, foo, bar]
was straight forward. But when passing arguments those need to be grouped by the file name{foo: [args-for-first-foo, args-for-second-foo], bar: [args-for-bar]}
which seems rather unintuitive.
- Before calling it with
Another alternative might be to pass the filenames and their arguments in a single parameter. Each item in the list could be either a plain filename or a tuple where the first element is the filename and the second element contains the arguments. That might provide a nicer API while still being backward compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your last idea. It's intuitive and should be easy to implement as well as understand for users. I'm on it.
try: | ||
logger.info('loading config file %s'%f) | ||
loader.load(f, config, argv=args, verbose=verbose) | ||
if args and len(args) and hasattr(args[0],'__iter__'): # non-empty list of iterables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow for one launchfile with multiple distinct calls by allowing the args
itself to be a list of arglists. The file will be launched once for each arglist
in args
. Otherwise, just launch the file with args
as normal.
8e043d8
to
2117b31
Compare
2117b31
to
11b29bd
Compare
Updated per suggestion from @dirk-thomas . Fully backwards-compatible and minimal changes made to add new functionality. Calling the api with launch arguments is a little clunky as you now have to build a list of tuples, but it's no worse than any of the alternatives. New API: cli_args = ['pkg', 'file.launch', 'arg1:=arg1', 'arg2:=arg2']
roslaunch_args = cli_args[2:]
roslaunch_file = [(roslaunch.rlutil.resolve_launch_arguments(cli_args)[0], roslaunch_args)]
parent = roslaunch.parent.ROSLaunchParent(uuid, roslaunch_file) The same can be done for multiple launch files. Launch files with and without arguments can be freely mixed in a list. If it has args, it is a tuple, Multiple launchfiles will look something like: launch_files = ["/path/to/file1", ("path/to/file2", ["args", "for", "launch2"]), "/path/to/file3", ...] where |
@dirk-thomas, have you had a chance to look at this? I know it's pretty low-priority, but just wondering if this was going to make it in or if it should be closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current patch looks good to me. Please update the docblock of the function to mention the new possible values for roslaunch_files
.
Done. Let me know if that's not clear enough. Also, would it break the API to change # current behavior. would remain unchanged.
roslaunch.rlutil.resolve_launch_arguments(['pkg', 'file']) #==> returns ['/path/to/file']
# current behavior with args. same return value as above
roslaunch.rlutil.resolve_launch_arguments(['pkg','file','arg:=arg']) #==> returns ['/path/to/file']
# suggested modification
roslaunch.rlutil.resolve_launch_arguments(['pkg','file','arg:=arg']) #==> returns [('/path/to/file', ['arg:=arg'])] Thoughts? This would change the way all roslaunch arguments are handled internally, but the change is innocuous (args would be passed through as function arguments via the new tuple interface instead of being read in from If interested, I can open a new PR to explore this. Or you can tell me to leave well enough alone and I'll drop it. :) |
As far as I understand your example I would say since you are changing the return value. Callers expecting a plain list with launch file names would fail then if each item in the list is a tuple.
Since it is a public function you have to assume that it is used somewhere. I rephrased the docblock a bit in d9cd550 to clarify that the item is either a launch file name or a tuple with exactly two elements. I think this is good to be merge now. Thanks for the quick update. |
Sounds good to me. Thanks for getting this through! |
…g the API (ros#1115) * roslaunch - pass command-line arguments through to the xmlloader when using the api * update doc block with new roslaunch_files value options * rephrase doc block * the second element of the tuple is actually not optional
# This is the 1st commit message: Add LOT implementation and benchmarks # The commit message #2 will be skipped: # add benchmarks # The commit message #3 will be skipped: # add TZC and SHM header files # The commit message #4 will be skipped: # backport Boost 1.65.0 # The commit message #5 will be skipped: # port ros_comms/test folder # The commit message #6 will be skipped: # fixup! port ros_comms/tools folder # The commit message #7 will be skipped: # port ros_comms/utilities folder # The commit message #8 will be skipped: # remove metrics.cpp/hpp # The commit message #9 will be skipped: # enable C++11 in rosbag package # The commit message #10 will be skipped: # defined missing macro ROSCPP_START_STOP_STREAM_DEBUG # The commit message #11 will be skipped: # remove feature file # The commit message #12 will be skipped: # backporting roslaunch fix - see ros/ros_comm#1115 # The commit message #13 will be skipped: # relocate and update README file # The commit message #14 will be skipped: # remove references to Five repos in the Docker file # The commit message #15 will be skipped: # add copyright to various files # The commit message #16 will be skipped: # fix typo in README file + minor cosmetic fixes # The commit message #17 will be skipped: # make links relative # The commit message #18 will be skipped: # fix broken link # The commit message #19 will be skipped: # add copyright to Dockerfile # The commit message #20 will be skipped: # add explanatory comments regarding the usage of "experimental" Docker keyword # The commit message #21 will be skipped: # remove reference to Five registry from docker-compose file # The commit message #22 will be skipped: # some more copyright fixes + remove commented code # The commit message #23 will be skipped: # trim the collection of marcos # The commit message #24 will be skipped: # remove unsued code from String.h # The commit message #25 will be skipped: # removed unused type # The commit message #26 will be skipped: # several cosmetic fixes # The commit message #27 will be skipped: # use static_cast and replace shm alias by shaed memory # The commit message #28 will be skipped: # fix several typos # The commit message #29 will be skipped: # remove redundant comments # The commit message #30 will be skipped: # remove commented code + some extra blank lines # The commit message #31 will be skipped: # revert unnecessary formatting change to ROS source file # The commit message #32 will be skipped: # enhance comment in shm_puller.cpp # The commit message #33 will be skipped: # remove "platform" intermediate namespace # The commit message #34 will be skipped: # update error message # The commit message #35 will be skipped: # remove confusing comment # The commit message #36 will be skipped: # now benchmark results folder is specified via SHM_ON_ROS_HOME env variable in both docker-compose and launch.xml files # # create SHM_ON_ROS_HOME folder upon user log in in the container # # set SHM_ON_ROS_HOME in the docker-compose file # # escape $ in docker file # The commit message #37 will be skipped: # update README file # The commit message #38 will be skipped: # generate MD5 sums for the newly introduced SHM ROS messages # The commit message #39 will be skipped: # remove ofending comment from roscpp test cmake file # The commit message #40 will be skipped: # add comments to various source files # The commit message #41 will be skipped: # placing SHM messages in fiveai::shm_msgs to reduce code verbosity # The commit message #42 will be skipped: # remove fiveai sufix # The commit message #43 will be skipped: # now update README file # The commit message #44 will be skipped: # updates to docker and README files after renaming branch to fiveshm # The commit message #45 will be skipped: # enhance comment clarity # The commit message #46 will be skipped: # update github repo to point to fiveai # The commit message #47 will be skipped: # Readme tweaks and remove SHM_ON_ROS_HOME references. # The commit message #48 will be skipped: # Address review comments. # The commit message #49 will be skipped: # Wrapped code in appropriate markdown.
# This is the 1st commit message: Add LOT implementation and benchmarks # The commit message #2 will be skipped: # add benchmarks # The commit message #3 will be skipped: # add TZC and SHM header files # The commit message #4 will be skipped: # backport Boost 1.65.0 # The commit message #5 will be skipped: # port ros_comms/test folder # The commit message #6 will be skipped: # fixup! port ros_comms/tools folder # The commit message #7 will be skipped: # port ros_comms/utilities folder # The commit message #8 will be skipped: # remove metrics.cpp/hpp # The commit message #9 will be skipped: # enable C++11 in rosbag package # The commit message #10 will be skipped: # defined missing macro ROSCPP_START_STOP_STREAM_DEBUG # The commit message #11 will be skipped: # remove feature file # The commit message #12 will be skipped: # backporting roslaunch fix - see ros/ros_comm#1115 # The commit message #13 will be skipped: # relocate and update README file # The commit message #14 will be skipped: # remove references to Five repos in the Docker file # The commit message #15 will be skipped: # add copyright to various files # The commit message #16 will be skipped: # fix typo in README file + minor cosmetic fixes # The commit message #17 will be skipped: # make links relative # The commit message #18 will be skipped: # fix broken link # The commit message #19 will be skipped: # add copyright to Dockerfile # The commit message #20 will be skipped: # add explanatory comments regarding the usage of "experimental" Docker keyword # The commit message #21 will be skipped: # remove reference to Five registry from docker-compose file # The commit message #22 will be skipped: # some more copyright fixes + remove commented code # The commit message #23 will be skipped: # trim the collection of marcos # The commit message #24 will be skipped: # remove unsued code from String.h # The commit message #25 will be skipped: # removed unused type # The commit message #26 will be skipped: # several cosmetic fixes # The commit message #27 will be skipped: # use static_cast and replace shm alias by shaed memory # The commit message #28 will be skipped: # fix several typos # The commit message #29 will be skipped: # remove redundant comments # The commit message #30 will be skipped: # remove commented code + some extra blank lines # The commit message #31 will be skipped: # revert unnecessary formatting change to ROS source file # The commit message #32 will be skipped: # enhance comment in shm_puller.cpp # The commit message #33 will be skipped: # remove "platform" intermediate namespace # The commit message #34 will be skipped: # update error message # The commit message #35 will be skipped: # remove confusing comment # The commit message #36 will be skipped: # now benchmark results folder is specified via SHM_ON_ROS_HOME env variable in both docker-compose and launch.xml files # # create SHM_ON_ROS_HOME folder upon user log in in the container # # set SHM_ON_ROS_HOME in the docker-compose file # # escape $ in docker file # The commit message #37 will be skipped: # update README file # The commit message #38 will be skipped: # generate MD5 sums for the newly introduced SHM ROS messages # The commit message #39 will be skipped: # remove ofending comment from roscpp test cmake file # The commit message #40 will be skipped: # add comments to various source files # The commit message #41 will be skipped: # placing SHM messages in fiveai::shm_msgs to reduce code verbosity # The commit message #42 will be skipped: # remove fiveai sufix # The commit message #43 will be skipped: # now update README file # The commit message #44 will be skipped: # updates to docker and README files after renaming branch to fiveshm # The commit message #45 will be skipped: # enhance comment clarity # The commit message #46 will be skipped: # update github repo to point to fiveai # The commit message #47 will be skipped: # Readme tweaks and remove SHM_ON_ROS_HOME references. # The commit message #48 will be skipped: # Address review comments. # The commit message #49 will be skipped: # Wrapped code in appropriate markdown. # The commit message #50 will be skipped: # Update README.md # The commit message #51 will be skipped: # Update README.md
ros/ros_comm#1115 keyword variable in both docker-compose and launch.xml files
ros/ros_comm#1115 keyword variable in both docker-compose and launch.xml files
Using the roslaunch API to manage multiple launch files, it seems logical that you would be able to specify
arg:=value
pairs; however, the documentation does not seem to hint that this is possible. After digging, it is indeed impossible in the current API to do so. This is due to the fact that there is no mechanism to pass any arguments to the XML loader from a ROSLaunchParent. The XML loader currently just looks atsys.argv
for arguments. This is obviously fine for command-line usage, but breaks any usage from the API.The way to do this is a question occasionally from users as it is natural to think it is possible:
Simple usage examples.