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

Workaround for logger exception #532

Closed

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2019


Basic Info

Info Please fill out this column
Ticket(s) this addresses NA
Primary OS tested on Ubuntu
Robotic platform tested on Turtlebot 3 in Gazebo

Description of contribution in a few bullet points

  • This works around Exception being thrown: failed to initialize rcl node: Logger already initialized for node ros2/rcl#375. This was causing our system test to fail consistently. Our system test should pass again with this workaround.
  • This replaces Disable unstable system test in Travis. #527 as a better alternative.
  • The problem with this PR is that the use_sim_time parameter cannot be changed from the command line. Instead, if you want to use real time, you must manually change the nav2_params.yaml file. To fix this, we need to upgrade the ros2 launch tool, which is in the works.
  • One last point. I did not apply the workaround to the map_server node. Due to fortunate circumstances, this node is not affected by the issue. By not applying the workaround, we are able leave the map file name as a parameter to the launch file.

@ghost ghost requested review from mkhansenbot, mhpanah and orduno January 24, 2019 19:22
Copy link
Collaborator

@mkhansenbot mkhansenbot left a comment

Choose a reason for hiding this comment

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

Something seems strange that this passed even though it uses the nav2_system_tests/params/nav2_params.yaml file and you didn't update that file to set use_sim_time=True. Am I missing something? How is this passing?

@ghost
Copy link
Author

ghost commented Jan 25, 2019

It passes because the test_system_node.py code sets use_sim_time = true on nodes in all three namespaces. I believe that causes everything to switch to sim_time.

It is basically the same as what we were doing before we started setting use_sim_time in the launch file. We'd let everything come up, then set the parameter on one node, and all the nodes would see it and switch.

@mkhansenbot
Copy link
Collaborator

If that's the case, we can just remove the params file from the parameters list for those nodes that are only setting use_sim_time: amcl, navfn and simple_navigator and it should still pass

@ghost
Copy link
Author

ghost commented Jan 25, 2019

Looks like you are correct. I just pushed that change.

Copy link
Collaborator

@mkhansenbot mkhansenbot left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Carl!

@@ -31,9 +31,8 @@ def generate_launch_description():
launch_ros.actions.Node(
package='nav2_amcl',
node_executable='amcl',
node_name='amcl',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line is the only one that matters for working around ros2/rcl#375. The change to how parameters are passed doesn't look to be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah never mind, I didn't realize launch required node_name when using this form of paramters

`[ERROR] [launch.LaunchService]: Caught exception in launch (see debug for traceback): If a dictionary of parameters is specified, the node name must also be specified. See https://github.com/ros2/launch/issues/139

@mkhansenbot
Copy link
Collaborator

@orduno or @SteveMacenski - can either of you approve so we can merge this and get the Travis master branch build back to green?

@mkhansenbot mkhansenbot added the 0 - Critical Critical to project, highest priority label Jan 28, 2019
@ghost ghost added this to the EndFebruary milestone Jan 29, 2019
@ghost ghost self-assigned this Jan 29, 2019
@ghost
Copy link
Author

ghost commented Jan 30, 2019

Since #538 seems to be working, I'm closing this one.

@ghost ghost closed this Jan 30, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Critical Critical to project, highest priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants