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

Add pollForService feature #509

Merged
merged 5 commits into from
Sep 14, 2019

Conversation

koonpeng
Copy link
Collaborator

I notice rclnodejs blocks indefinitely when doing a service call immediately after creating a client. This is possibly because the service is not ready yet.

This PR adds isServiceServerAvailable and pollForService method. They aim to provide the same functionality as wait_for_service in rclpy. (https://github.com/ros2/rclpy/blob/1cf216dd9ecd4359dc93f317e01b7386d5a36c5a/rclpy/rclpy/client.py#L146)

@minggangw
Copy link
Member

Thanks for your contribution! Indeed, we should have these two functions to facilitate developers, otherwise, they will meet the situation you describe. Some suggestions:

  • Would you please change the method name pollForService to waitForService, as I prefer to align with rclpy naming to reduce confusion.
  • About the CIs
    • For Travis-Ci (Unbuntu), there is a problem when installing the dependency base on the latest nightly build, but we can ignore it.
    • For the circleci (macOS), I notice some tests failed. I'm afraid we have to solve this before merging.
  • Finally, you could add yourself to the contributors 😄

@coveralls
Copy link

coveralls commented Sep 12, 2019

Coverage Status

Coverage increased (+0.06%) to 95.803% when pulling 9dc4eb7 on koonpeng:add-pollforservice into bbd5a5a on RobotWebTools:develop.

@koonpeng
Copy link
Collaborator Author

Thanks for the comments!

I changed pollForService to waitForService as suggested. I wan't sure what to name it because pollForService gives a better description but I can see how having the same name as rclpy helps users.

For the mac tests, it looks like it fails in ActionClient status succeed which this PR shouldn't affect. The 2nd failure seems to log that the publisher is no longer valid. Unfortunately I don't have a mac and I can't reproduce it on Linux. A subsequent run does pass though so it could be a one off error?

@minggangw
Copy link
Member

minggangw commented Sep 12, 2019

For the mac tests, it looks like it fails in ActionClient status succeed which this PR shouldn't affect. The 2nd failure seems to log that the publisher is no longer valid. Unfortunately I don't have a mac and I can't reproduce it on Linux. A subsequent run does pass though so it could be a one off error?

Yes, some tests become flaky at a low rate, so you have to rerun it manually. Once it becomes green and we can land it.

The problem on travis-ci should have been fixed by ros2/ament_cmake_ros#5, you can try to trigger the bot again after there is a newly built package on the ci.ros2.org. Thanks!

@minggangw
Copy link
Member

As all bots turn green, I create an issue #512 to track this feature and will merge it soon, thanks!

@minggangw minggangw merged commit 4c6b4a7 into RobotWebTools:develop Sep 14, 2019
@minggangw
Copy link
Member

@koonpeng PR has been merged, thanks!

You are always welcome to contribute any new features to this module if you are interested in. Actually, #498 has a detailed summary of the current future absence compared with rclpy and I hope we can offer an alternative to other developers to bridge them into ROS2 😃

@koonpeng koonpeng deleted the add-pollforservice branch March 9, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants