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

LaunchConfiguration is not read from ComposableNode when included inside a group action #328

Closed
stevedanomodolor opened this issue Oct 12, 2022 · 11 comments
Assignees
Labels
bug Something isn't working question Further information is requested

Comments

@stevedanomodolor
Copy link

Bug report

Required Info:

Steps to reproduce issue

ComposableNode(
                        condition=LaunchConfigurationNotEquals('map', ''),
                        package='nav2_map_server',
                        plugin='nav2_map_server::MapServer',
                        name='map_server',
                        parameters=[configured_params, {'yaml_filename': map_yaml_file}],
                        remappings=remappings
                        ),
ComposableNode(
                        condition=LaunchConfigurationNotEquals('map', ''),
                        package='nav2_map_server',
                        plugin='nav2_map_server::MapServer',
                        name='map_server',
                        parameters=[configured_params, {'yaml_filename': map_yaml_file}],
                        remappings=remappings
                        ),

Expected behavior

It should pay attention to the condition but executes both composable node. The issue #114 should have fixed it but seems not to have.

Actual behavior

It does not pay attention to the condition at all and executes both composablenode

Additional information


Feature request

Feature description

Implementation considerations

@clalancette
Copy link
Contributor

So we do have tests to ensure that condition works in a ComposableNode with IfCondition and UnlessCondition:

So I'm somewhat confident the basic functionality works.

That said, we don't have tests for LaunchConfigurationNotEquals with ComposableNode. I don't even know if it is supposed to work. Pinging @adityapande-1995 @jacobperron @hidmic for thoughts.

@clalancette clalancette added bug Something isn't working question Further information is requested labels Oct 27, 2022
@methylDragon
Copy link
Contributor

methylDragon commented Nov 1, 2022

Well..., two things:

The LaunchConfigurationEquals and LaunchConfigurationNotEquals conditions are pretty recently deprecated.

Also, weird things happen when trying to check for equality with empty strings with that condition..

Try with the EqualsSubstitution instead!
https://github.com/ros2/launch/blob/rolling/launch/launch/conditions/launch_configuration_equals.py

e.g.:

IfCondition(EqualsSubstitution(LaunchConfiguration('some_launch_arg'), "some_equality_check")

If that fails, try the more terse conditions as suggested by Chris, above.

@stevedanomodolor
Copy link
Author

How do i upgrade the launch package to have this changes, it seems that this feature was added not recently. I am using ros2 rolling. @methylDragon

@methylDragon
Copy link
Contributor

Try

sudo apt update
sudo apt upgrade --only-upgrade ros-rolling-launch

@stevedanomodolor
Copy link
Author

I get this when I try to upgrade, is this feature available in ros-rolling yet?

root@stevedan-OMEN-Laptop-15-ek0xxx:/opt/ros/rolling/lib/python3.10/site-packages/launch/substitutions# sudo apt-get install --only-upgrade ros-rolling-launch
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
ros-rolling-launch is already the newest version (1.2.0-1jammy.20220914.023741).
0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
root@stevedan-OMEN-Laptop-15-ek0xxx:/opt/ros/rolling/lib/python3.10/site-packages/launch/substitutions# ls
__init__.py              environment_variable.py    python_expression.py
__pycache__              find_executable.py         substitution_failure.py
anon_name.py             launch_configuration.py    text_substitution.py
boolean_substitution.py  local_substitution.py      this_launch_file.py
command.py               path_join_substitution.py  this_launch_file_dir.py
root@stevedan-OMEN-Laptop-15-ek0xxx:/opt/ros/rolling/lib/python3.10/site-packages/launch/substitutions# 

@methylDragon
Copy link
Contributor

Hmm, you're right. The update was released 5 days ago, but it might take a little while for it to get through to the main repos. It should come out soon though.

@hodnajit
Copy link

hodnajit commented Dec 2, 2022

I am facing the similar issue, but I am using IfCondition. You may find the issue description here:
#335

@methylDragon
Copy link
Contributor

I am facing the similar issue, but I am using IfCondition. You may find the issue description here: #335

I think the issue is related. I am going to put some time into investigating this...

@methylDragon
Copy link
Contributor

methylDragon commented Dec 8, 2022

Hello! This PR should address the issue (:

Do note that it'll take a little while for this change to propagate through and get released (the PR will need to get merged, then the repo has to get released, and finally there has to be a rolling sync.) So I'd recommend building from source if you want the change immediately.
@stevedanomodolor

@stevedanomodolor
Copy link
Author

Thanks for the update

@SteveMacenski
Copy link

SteveMacenski commented Mar 3, 2023

Changes being made now in Nav2 https://github.com/ros-planning/navigation2/pull/3445/files

I'll say that having to replace LaunchConfigurationEquals with IfCondition(EqualsSubstitution(LaunchConfiguration())) seems to be moving in the wrong direction in launch of making it all increasingly obtuse, not simpler. I'm not exactly sure why a method to wrap that logic called LaunchConfigurationEquals was depreciated. It is a useful shorthand of a common desire to check the value of a launch configuration provided on the commandline. All 3 of those actions should definitely exist, but the method making it easier to use it seems useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants