-
Notifications
You must be signed in to change notification settings - Fork 11
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
converting the environment variable for choosing the robot and map into launch arguments #82
Conversation
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.
Good Start!
But there are multiple return statements at the end of each launch_setup
function. Could you please recheck what's happening there?If there is a return
statement that is redundant, you can remove that..
It will be also nice if you can make a video of the working once you are done with the complete feature, so it will be easier to accept and review the PR. You can aim for a documentation ready video, by that way it solves multiple purposes.
from launch.launch_description_sources import PythonLaunchDescriptionSource | ||
from launch.substitutions import LaunchConfiguration, PythonExpression | ||
from launch_ros.actions import Node, PushRosNamespace | ||
from launch.conditions import IfCondition | ||
|
||
MY_NEO_ROBOT = os.environ.get('MY_ROBOT', "mpo_700") | ||
MY_NEO_ENVIRONMENT = os.environ.get('MAP_NAME', "neo_workshop") | ||
# MY_NEO_ROBOT = os.environ.get('MY_ROBOT', "mpo_700") |
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.
No need to be commented
MY_NEO_ROBOT = os.environ.get('MY_ROBOT', "mpo_700") | ||
MY_NEO_ENVIRONMENT = os.environ.get('MAP_NAME', "neo_workshop") | ||
# MY_NEO_ROBOT = os.environ.get('MY_ROBOT', "mpo_700") | ||
# MY_NEO_ENVIRONMENT = os.environ.get('MAP_NAME', "neo_workshop") |
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.
you can remove this too
description='Map Types: "neo_track1", "neo_track2", "neo_workshop"') | ||
|
||
# Create launch configuration variables for the robot and map name | ||
MY_NEO_ROBOT_ARG = LaunchConfiguration('MY_ROBOT') |
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 would change the MY_NEO_ROBOT_ARGS
variable to small letter, as well as the MY_ROBOT
argument itself to small letter..
|
||
# Create launch configuration variables for the robot and map name | ||
MY_NEO_ROBOT_ARG = LaunchConfiguration('MY_ROBOT') | ||
MY_NEO_ENVIRONMENT_ARG = LaunchConfiguration('MAP_NAME') |
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.
same as above
'params_file': param_dir}.items()), | ||
) | ||
|
||
return launch_actions |
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 see multiple returns here, is it still a work in progress?
name='teleop') | ||
|
||
return [gazebo, spawn_entity, start_robot_state_publisher_cmd, teleop] |
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.
There are also multiple returns here? not sure if I understood it correctly?
along with the map and robot model, the use sim time and use robot state publisher parts of the code were also modified