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.

Sign up for GitHub

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

Jump to bottom

Avoid passing parameters using a wildcard if fully qualified node name is known at launch time #154

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Avoid passing parameters using a wildcard if fully qualified node name is known at launch time #154

merged 1 commit into from
Jun 25, 2020

Conversation

ivanpauno
Copy link
Member

This can be considered a fix of the problem described here.

This only solves the problem if both name and namespace were explicitly specified at launch.
If we finally decide to also change rcl behavior, this change doesn't do any harm.

Depends on #153.

@ivanpauno ivanpauno self-assigned this Jun 19, 2020
@ivanpauno
Copy link
Member Author

I will have to rebase on master after #153 gets merged.

@ivanpauno
Copy link
Member Author

@clalancette @jacobperron @hidmic do you consider this a fix to ros2/rclcpp#953? should we change behavior in rcl too?

I can also add a warning if somebody passes both parameter files and a dictionary, telling the user that the passed parameters can't override values in the parameter file if they don't specify the fully qualified node name at launch time.

hidmic
hidmic approved these changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI

test_launch_ros/test/test_launch_ros/actions/test_node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
@hidmic
Copy link
Contributor

hidmic commented Jun 19, 2020

do you consider this a fix to ros2/rclcpp#953? should we change behavior in rcl too?

I think they are orthogonal.

@ivanpauno ivanpauno mentioned this pull request Jun 19, 2020
Open
@hidmic hidmic mentioned this pull request Jun 19, 2020
Merged
@jacobperron
Copy link
Member

do you consider this a fix to ros2/rclcpp#953? should we change behavior in rcl too?

I agree that this change is separate from the rclcpp issue. I'm still not sure what the correct behavior should be at the rcl level.

@ivanpauno
Avoid using a wildcard to specify parameters if possible
e7990e4 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/dont-use-wildcard-if-possible branch from 5ba4984 to e7990e4 Compare June 24, 2020 21:51
@ivanpauno
Copy link
Member Author

Rebased after #153 was merged

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 6c1f36a into master Jun 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/dont-use-wildcard-if-possible branch June 25, 2020 15:41
jacobperron pushed a commit that referenced this pull request Oct 20, 2020
@ivanpauno @jacobperron
Avoid using a wildcard to specify parameters if possible (#154)
bd7ec23 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@jacobperron jacobperron mentioned this pull request Oct 20, 2020
Merged
jacobperron added a commit that referenced this pull request Oct 20, 2020
@jacobperron @ivanpauno
Avoid using a wildcard to specify parameters if possible (#154) (#188)
42e0ba9 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

@clalancette clalancette Awaiting requested review from clalancette

@jacobperron jacobperron Awaiting requested review from jacobperron

Assignees

@ivanpauno ivanpauno

Labels
in review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivanpauno @hidmic @jacobperron