Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Added read_condition for services and clients #66

Merged
merged 1 commit into from
Oct 15, 2015
Merged

Added read_condition for services and clients #66

merged 1 commit into from
Oct 15, 2015

Conversation

esteve
Copy link
Member

@esteve esteve commented Aug 3, 2015

This should fix ros2/rclcpp#52 for services

Connects to ros2/rclcpp#52

@esteve esteve added the in progress Actively being worked on (Kanban column) label Aug 3, 2015
@esteve
Copy link
Member Author

esteve commented Aug 3, 2015

@dirk-thomas I applied the same fix as for subscriptions, but can't seem to make the client fire when the service sends back the response. I ran rtiddsspy and messages are sent between the service and the client, though. Would appreciate any feedback from you. Thanks.

@esteve
Copy link
Member Author

esteve commented Aug 3, 2015

@dirk-thomas I think I found where the problem is, the client's read condition was not being attached to the waitset.

@esteve
Copy link
Member Author

esteve commented Aug 3, 2015

Linux buildfarm:

http://ci.ros2.org/job/ros2_batch_ci_linux/137/

dirk-thomas
dirk-thomas reviewed Aug 3, 2015
View reviewed changes
rmw_connext_cpp/src/functions.cpp
@@ -1242,6 +1221,13 @@ rmw_create_client(
goto fail;
}

read_condition = response_datareader->create_readcondition(
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be cleaned up in the fail section.

Same below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@dirk-thomas dirk-thomas mentioned this pull request Aug 3, 2015
Closed
@tfoote tfoote mentioned this pull request Aug 3, 2015
Merged
@dirk-thomas
Copy link
Member

+1

Has it been tested that this fixes ros2/system_tests#14

@esteve
Copy link
Member Author

esteve commented Aug 4, 2015

Buildfarm job:

http://ci.ros2.org/job/ros2_batch_ci_linux/143/

@dirk-thomas
Copy link
Member

This CI job does not cover the new parameter test yet.

@wjwwood
Copy link
Member

wjwwood commented Aug 4, 2015

Code changes look good, waiting on the test.

@tfoote
Copy link
Contributor

tfoote commented Aug 18, 2015

+1, unfortunately this needs to be rebased now

@esteve
Copy link
Member Author

esteve commented Aug 18, 2015

Rebased.

@esteve esteve force-pushed the fix_service_samples branch 2 times, most recently from 8eb34b2 to 650068f Compare September 15, 2015 23:52
@dirk-thomas dirk-thomas mentioned this pull request Oct 15, 2015
Merged
@jacquelinekay
Copy link
Contributor

I had to rebase this on master to get it to build on my machine (currently test ros2/system_tests#57)

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 15, 2015
@jacquelinekay
Copy link
Contributor

+1

esteve added a commit that referenced this pull request Oct 15, 2015
@esteve
Merge pull request #66 from ros2/fix_service_samples
7528ffe 
Added read_condition for services and clients
@esteve esteve merged commit 7528ffe into master Oct 15, 2015
@esteve esteve removed the in review Waiting for review (Kanban column) label Oct 15, 2015
@esteve esteve deleted the fix_service_samples branch October 15, 2015 18:39
@dirk-thomas dirk-thomas mentioned this pull request Feb 5, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subscriptions are not triggered sometimes
5 participants
@esteve @dirk-thomas @wjwwood @tfoote @jacquelinekay