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

Added new functionality: service_is_available #56

Merged
merged 2 commits into from
Sep 20, 2016

Conversation

richiprosima
Copy link
Contributor

@richiprosima richiprosima commented Sep 9, 2016

This PR contains the service_is_available functionality. To work properly Fast-RTPS has to be in/after commit eProsima/Fast-DDS@24825bb.

Closes #14.

@richiprosima richiprosima added the in progress Actively being worked on (Kanban column) label Sep 9, 2016
@dirk-thomas
Copy link
Member

@richiprosima The latest changes in FastRTPS introduced new compiler warnings and test failures. I think you should consider using Travis / AppVeyor to test the codebase automatically. I filled eProsima/Fast-DDS#62 for this but it applies to all repositories.

@richiprosima
Copy link
Contributor Author

Your nightly CI job started while I was developing this new functionality. It checked out a commit which is not the final commit of my work. Sorry for that, I shouldn't have worked with master branch of Fast-RTPS.
Currently we have an internal Jenkins server for testing Fast-RTPS and other projects. In future we think about Travis. Thanks for your suggestion.

@wjwwood
Copy link
Member

wjwwood commented Sep 15, 2016

I'm testing this out right now. I believe we'll need to remove some test exclusions if this starts working:

ros2/examples#113
ros2/system_tests#143

If this works ok, I'll add pr's with the same branch name as this one that remove the test exclusions and/or sleeps so that we can run CI.

@wjwwood
Copy link
Member

wjwwood commented Sep 16, 2016

This is looking ok, but what's missing is graph notifications from fastrtps. What this means is that the blocking call wait_for_service will always only check the state of the services (with service_is_available) once. The intention is that it checks each time the graph changes, but that's not currently happening.

I'm looking into what's missing for that to start working. Based on how far off that functionality is, I'll recommend whether or not to merge this as an incremental improvement or to wait and get the graph change notifications in place first.

@wjwwood
Copy link
Member

wjwwood commented Sep 16, 2016

Looks like it is going to be easy to piggy-back the changes to support graph change notifications on top of the changes you made here to support service_is_available. I'll update this pr with my changes asap.

@wjwwood
Copy link
Member

wjwwood commented Sep 16, 2016

With d7b9634, the "add two ints" client/service examples work really well with each other. I'm going to continue digging into what needs to be done to re-enable all the appropriate tests.

@wjwwood
Copy link
Member

wjwwood commented Sep 19, 2016

Starting CI (fastrtps only):

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

@wjwwood
Copy link
Member

wjwwood commented Sep 19, 2016

I'm rerunning CI (still fastrtps only) since I removed an extra sleep that @dhood found:

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

@wjwwood
Copy link
Member

wjwwood commented Sep 19, 2016

Actually, all those were invalid because the rclcpp examples were not correctly enabled (I enabled them in a different way locally):

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

@wjwwood
Copy link
Member

wjwwood commented Sep 20, 2016

Ok, the last ones definitely had the new rclcpp examples test for fastrtps:

http://ci.ros2.org/job/ci_linux/1729/testReport/(root)/test_add_two_ints_server_add_two_ints_client__rmw_fastrtps_cpp_/test_executable/

@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 20, 2016
@wjwwood
Copy link
Member

wjwwood commented Sep 20, 2016

Ok, I've got passing CI with fastrtps and Connext now that I removed the system_tests pr from this group:

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

This pr and the related pr on the examples repository are ready for review and merge.

@dhood
Copy link
Member

dhood commented Sep 20, 2016

Making it more explicit: system_tests using services, when re-enabled for both fastrtps and connext in ros2/system_tests#162, worked fine for fastrtps, just not connext (e.g. http://ci.ros2.org/job/ci_linux/1731/testReport/(root)/test_client_scope_cpp__rmw_fastrtps_cpp_/test_client_scope_cpp/)
so I would say it looks good

@wjwwood wjwwood merged commit f9aed02 into master Sep 20, 2016
@wjwwood wjwwood deleted the feature/service_is_available branch September 20, 2016 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants