Fix default QoS settings for services #100

jacquelinekay · 2015-09-09T23:37:05Z

Connects to #96

See ros2/rmw_opensplice/pull/82 for description.

dirk-thomas · 2015-09-10T21:57:47Z

I am not sure about the changes since the pub / sub equivalent of the code does something different (especially after #88):

Just from reading these two code parts it looks to me (on a first look) that they are contradicting each other.

I know this is not directly related to this PR but since this PR adds similar stuff for the services as for pub/sub I think those should be similar (or even the same).

jacquelinekay · 2015-09-10T22:13:16Z

I'm not sure what you mean when you say these sections of code contradict each other.

These are the three differences I see:

The QoS configuration code for publishers sets the policy for a DataWriter object. The equivalent for subscriptions sets the policy for a DataReader object. The equivalent for services and clients sets both a DataReader and a DataWriter, because both clients and services have a DataReader and DataWriter object.
The service/client code casts the history policy depth as a DDS_Long, while the pub/sub code does not do any casting. I can take this out for the sake of similarity as I don't think it's required.
The service/client code sets the durability policy to TRANSIENT_LOCAL. I can also take this out for the sake of similarity, though I need to confirm first that it's not needed to fix the bug.

dirk-thomas · 2015-09-11T06:47:01Z

The first code block sets the history depth of the writer to the requested value unconditionally (if RMW_QOS_POLICY_DEPTH_SYSTEM_DEFAULT was not requested).

The second block increases the history of the reader to the requested queue size.

The two logics are significantly different in behavior and I think they should be considered to to the same conceptional thing instead. Currently the resulting behavior is not obvious and different for readers and writers.

esteve · 2015-09-11T17:19:47Z

rmw_connext_cpp/src/functions.cpp

@@ -934,6 +934,47 @@ rmw_create_client(
    goto fail;
  }

+  // For now, use the default QoS profile for services.


I'd move this code to create_requester (in srv__type_support.cpp.template), pass a rmw_qos_profile_t to rmw_create_client and default it to rmw_qos_profile_services_default.

Alright, I'll make the necessary changes here and to rmw and rmw_opensplice. I'll leave the work to propagating the QoS profile to rclcpp until after this set gets merged, if that's ok.

No problem, we can split work and I can deal with rclcpp if you're ok with that.

jacquelinekay · 2015-09-11T17:23:39Z

Oh, I see, Dirk is talking about lines 269-282 (create_publisher) and 599-612 (create_subscription).

https://github.com/ros2/rmw_connext/blob/master/rmw_connext_cpp/src/functions.cpp#L269

https://github.com/ros2/rmw_connext/blob/master/rmw_connext_cpp/src/functions.cpp#L599

It looks to me like the existing behavior is the same for readers and writers, and I need to change my initialization of the service/client readers and writers to match. I'm still not sure why the "ensure history depth is at least the requested size" block is needed though; why isn't the first part (lines 265-267) sufficient?

dirk-thomas · 2015-09-14T22:41:08Z

I was not referring to any difference between create_publisher and create_subscription. I was referring to the logic how the history depth of the data reader and writer is set.
For one value the code overrides the configuration from the xml - for the other the configuration can override the value specified in the code. I think we need to unify that and come up with a clear documentation / rational why we do it that way.

wjwwood · 2015-09-15T20:41:24Z

Is this discrepancy covered by ros2/rmw#13?

esteve · 2015-09-16T22:07:19Z

I've rebased on top of master and ran the tests in gtest-parameters several times on Linux and it seems any issues are now gone, clients are no longer stuck waiting for a response from servers.

jacquelinekay · 2015-09-17T18:37:50Z

Regarding the discussion about how depth is set: can we table the discussion for now and merge these PRs, since they fix several issues with parameters and services?

dirk-thomas · 2015-09-17T19:02:38Z

rosidl_typesupport_connext_cpp/resource/srv__type_support.cpp.template

+  DDS_DataWriterQos datawriter_qos;
+
+  if (!get_datareader_qos(participant, datareader_qos)) {
+    RMW_SET_ERROR_MSG("Unable to initialize datareader QoS");


It looks like these error messages are being overwritten afterwards if the function return NULL.

wjwwood · 2015-09-17T19:02:45Z

I think it needs to be unified one way or the other before merging this, but I don't care which way it is unified. The discussion of which way they both should be can be tabled.

dirk-thomas · 2015-09-17T19:06:28Z

rosidl_typesupport_connext_cpp/src/dds_utilities.cpp

+#include <rosidl_typesupport_connext_cpp/dds_utilities.hpp>
+
+bool
+get_datareader_qos(DDSDomainParticipant * participant, DDS_DataReaderQos & datareader_qos)


Moving / copying these functions from rmw into the rosidl package looks weird to me. It implies that every compiled message package embeds this logic. I would prefer if this stays in rmw and if the typesupport needs its somehow gets its passed as an argument.

This is compiled into the library for this package, not into each message library:

https://github.com/ros2/rmw_connext/blob/fix_service_qos/rosidl_typesupport_connext_cpp/CMakeLists.txt#L33

It may be true that it should be in rmw_connext_cpp instead though. The question is where is it used from. If it is used from the message specific code, then it would need to be here or as you suggested moved to rmw and used there, passing the result to the message specific code.

Good point - I didn't notice that. Before the type support library only contained the identifier.

Without this change the reader / write qos was determined within rmw so it should be possible to keep it there.

These functions are used in `srv__type_support.cpp.template, line 71:
https://github.com/ros2/rmw_connext/pull/100/files#diff-a2cb81bcab4593a61a1d8ad7a7308f25R71

I could change this function to take a DDSDataWriterQos struct and a DDSDataReaderQos struct, instead of an rmw_qos_profile, and initialize the DDS QoS structs in the rmw implementation before create_requester is called. Then the get_datareader/writer_qos functions would not need to be in the typesupport library.

That sounds like a good way to go.

Done. The diff is much simpler now. Not sure what I was doing before; I think I was trying to make this implementation match the OpenSplice typesupport library.

dirk-thomas · 2015-09-17T22:17:34Z

rmw_connext_cpp/src/functions.cpp

+    }
+    datareader_qos.history.depth = static_cast<DDS_Long>(qos_profile.depth);
+  }
+


Wouldn't it make sense to move all this code into the get_datareader_qos / get_datawriter_qos functions where all the other qos properties are set? That would also avoid have the same ~60 lines in four locations.

True, that would be a big improvement. Done in 65a3d43

dirk-thomas · 2015-09-17T23:52:24Z

LGTM. Let's spin some CI jobs.

jacquelinekay · 2015-09-18T18:24:38Z

http://ci.ros2.org/job/ros2_batch_ci_linux/372/
http://ci.ros2.org/job/ros2_batch_ci_osx/278/
http://ci.ros2.org/job/ros2_batch_ci_windows/421/

dirk-thomas · 2015-09-18T18:27:04Z

+1

…and share code for setting DataReader/Writer QoS.

Fix default QoS settings for services

jacquelinekay added the in progress Actively being worked on (Kanban column) label Sep 9, 2015

jacquelinekay mentioned this pull request Sep 9, 2015

Fix default QoS settings for services ros2/rmw_opensplice#82

Merged

jacquelinekay self-assigned this Sep 10, 2015

jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 10, 2015

jacquelinekay removed their assignment Sep 10, 2015

esteve reviewed Sep 11, 2015
View reviewed changes

dirk-thomas reviewed Sep 17, 2015
View reviewed changes

jacquelinekay force-pushed the fix_service_qos branch from 4095789 to da37f81 Compare September 17, 2015 20:59

dirk-thomas reviewed Sep 17, 2015
View reviewed changes

Pass services QoS profile to DataReader/Writer for services/clients, …

287e7ee

…and share code for setting DataReader/Writer QoS.

jacquelinekay force-pushed the fix_service_qos branch from 4409bac to 287e7ee Compare September 20, 2015 18:30

jacquelinekay added a commit that referenced this pull request Sep 20, 2015

Merge pull request #100 from ros2/fix_service_qos

030772b

Fix default QoS settings for services

jacquelinekay merged commit 030772b into master Sep 20, 2015

jacquelinekay deleted the fix_service_qos branch September 20, 2015 18:33

jacquelinekay removed the in review Waiting for review (Kanban column) label Sep 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default QoS settings for services #100

Fix default QoS settings for services #100

jacquelinekay commented Sep 9, 2015

dirk-thomas commented Sep 10, 2015

jacquelinekay commented Sep 10, 2015

dirk-thomas commented Sep 11, 2015

esteve Sep 11, 2015

jacquelinekay Sep 11, 2015

esteve Sep 11, 2015

jacquelinekay commented Sep 11, 2015

dirk-thomas commented Sep 14, 2015

wjwwood commented Sep 15, 2015

esteve commented Sep 16, 2015

jacquelinekay commented Sep 17, 2015

dirk-thomas Sep 17, 2015

wjwwood commented Sep 17, 2015

dirk-thomas Sep 17, 2015

wjwwood Sep 17, 2015

wjwwood Sep 17, 2015

dirk-thomas Sep 17, 2015

jacquelinekay Sep 17, 2015

dirk-thomas Sep 17, 2015

jacquelinekay Sep 17, 2015

dirk-thomas Sep 17, 2015

jacquelinekay Sep 17, 2015

dirk-thomas commented Sep 17, 2015

jacquelinekay commented Sep 18, 2015

dirk-thomas commented Sep 18, 2015

Fix default QoS settings for services #100

Fix default QoS settings for services #100

Conversation

jacquelinekay commented Sep 9, 2015

dirk-thomas commented Sep 10, 2015

jacquelinekay commented Sep 10, 2015

dirk-thomas commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquelinekay commented Sep 11, 2015

dirk-thomas commented Sep 14, 2015

wjwwood commented Sep 15, 2015

esteve commented Sep 16, 2015

jacquelinekay commented Sep 17, 2015

Choose a reason for hiding this comment

wjwwood commented Sep 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Sep 17, 2015

jacquelinekay commented Sep 18, 2015

dirk-thomas commented Sep 18, 2015