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

detect and report local graph changes #194

Merged
merged 8 commits into from
Oct 29, 2016
Merged

detect and report local graph changes #194

merged 8 commits into from
Oct 29, 2016

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jul 6, 2016

Fixes #168

This required, apparently for the first time, that the node is given to the rmw_destroy_client and rmw_destroy_service calls, so that will change the rmw API.

I'm still debugging some issues with the tests.

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Jul 6, 2016
@wjwwood wjwwood self-assigned this Jul 6, 2016
@wjwwood
Copy link
Member Author

wjwwood commented Jul 7, 2016

So, it seems that Connext's request-reply uses some strange topic names. For example, the service server creates:

+S add_two_intsRequest <example_interfaces::srv::dds_::AddTwoInts_Request_>
+P add_two_intsReply <example_interfaces::srv::dds_::AddTwoInts_Response_>

But the client creates:

+S add_two_intsReply_ac1702d4000020e70000000180002003 <example_interfaces::srv::dds_::AddTwoInts_Response_>
+P add_two_intsRequest <example_interfaces::srv::dds_::AddTwoInts_Request_>

Note what looks like a GUID attached to the reply topic. It's not clear to me how the replier (service server) delivers the reply to the requester (service client).

I spent the better part of last night trying to figure out a better way to figure out when the service server is available for how Connext implements Request-Reply. I have some ideas, but as of right now this is not ready for merging.

@wjwwood wjwwood force-pushed the local_graph_changes branch 2 times, most recently from 7fde876 to 8239f9a Compare September 21, 2016 22:02
@@ -176,7 +198,7 @@ rmw_create_publisher(
return nullptr;
}

auto node_info = static_cast<ConnextNodeInfo *>(node->data);
ConnextNodeInfo * node_info = static_cast<ConnextNodeInfo *>(node->data);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be changed back to auto?

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.

I think I did that so my autocomplete would work, but I can change them back: 90d4c86

@wjwwood wjwwood merged commit bb9b904 into master Oct 29, 2016
@wjwwood wjwwood deleted the local_graph_changes branch October 29, 2016 01:31
@wjwwood wjwwood removed the in progress Actively being worked on (Kanban column) label Oct 29, 2016
@dirk-thomas
Copy link
Member

This recent change broke rmw_connext_dynamic_cpp: http://ci.ros2.org/job/ci_linux/1870/testReport/

While not being actively tested at the moment maybe it can be easily fixed @wjwwood ?

@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2016

I believe it is now failing because the wait_for_service is still not implemented, see:

http://ci.ros2.org/job/ci_linux/1870/testReport/(root)/projectroot/requester_replier_cpp__Primitives__rmw_connext_dynamic_cpp/

The tests could be disabled, but I don't know if it is worth it since we're not regularly trying to figure out which failures are known and which are not for that rmw implementation. The fact that this function is not implemented is already tracked in an issue: #203

I'm not sure about the other failures.

@dirk-thomas
Copy link
Member

Since the merge numerous very simple tests which only do pub/sub are failing. I don't think the referenced ticket about wait_for_service is related to all of them. When I tried it with the new composition demos the executable crashed when calling one of the new functions around add_information. Therefore I am pretty sure that this is a regression of this PR.

@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2016

Well I'm sorry it is broken, because it wasn't my intention to break it, in fact I spent some extra time trying to keep it up to speed. However, fixing this is not at the top of my priority list because it's not one of our supported rmw implementations right now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants