Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events by jaisontj · Pull Request #193 · ros2/rmw

jaisontj · 2019-11-15T01:29:08Z

Related to this feature request. The design and implementation details can also be found there.

Added new enum type RMW_EVENT_REQUESTED_INCOMPATIBLE_QOS and RMW_EVENT_OFFERED_INCOMPATIBLE_QOS to rmw_event_type_t
Defined new structures that encapsulate data for the new events above

Signed-off-by: Jaison Titus jaisontj92@gmail.com

dirk-thomas · 2019-11-15T04:24:58Z

Please reference all related PR from at least one ticket for visibility.

Also for an RMW API change please include significantly more context / information.

jaisontj · 2019-11-16T00:42:11Z

@dirk-thomas Apologies for the lack of context, I planned on adding more information and that is why I had open this as a draft PR.

I've now opened up an issue here tracking all of the related PRs as well as detailing out the need for this feature and the implementation details. Intention is also to initiate a design discussion towards this.

rmw/include/rmw/types.h

piraka9011

LGTM!

mjcarroll

Some small nits

mjcarroll · 2019-12-18T15:33:56Z

rmw/include/rmw/types.h

+  /**
+   * Total cumulative number of times the concerned subscription discovered a
+   * publisher for the same topic with an offered QoS that was incompatible
+   * with that requested by the subscription..


extra period

mjcarroll · 2019-12-18T15:34:07Z

rmw/include/rmw/types.h

+  int32_t total_count_change;
+  /**
+   * The Qos Policy Id of one of the policies that was found to be
+   * incompatible the last time an incompatibility was detected


Missing period

mjcarroll · 2019-12-18T15:34:26Z

rmw/include/rmw/types.h

 #define RMW_QOS_LIFESPAN_DEFAULT {0, 0}
 #define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT {0, 0}

+/// RMW equivalent of the DDS QoS Policy IDs


Is there a reference for these enum values that could be linked?

I would say we should not mention DDS in the rmw API or its doc strings, as the underlying middleware may not be DDS. So we should just define what makes sense for our layer and if that happens to be similar to DDS's then we can mention that, but our interface should stand on its own.

Removed mention of DDS from the comment / docstring.

rmw/include/rmw/types.h

ivanpauno · 2019-12-18T21:06:49Z

rmw/include/rmw/types.h

+  RMW_QOS_POLICY_ID_LIVELINESS = 8,
+  RMW_QOS_POLICY_ID_RELIABILITY= 11,
+  RMW_QOS_POLICY_ID_HISTORY    = 13,
+  RMW_QOS_POLICY_ID_LIFESPAN   = 21


Why those strange numbers?
If they were taken from an specific implementation, please use contiguous numbers here, and convert them in each implementation.

I took these values from the DDS implementation spec assuming that all RMW implementations will follow the same values. However, that may not be a correct assumption.

We shouldn't assume a DDS based implementation.
The numbers in rmw can be just made up by you (if you decide to keep the ones in the dds spec that's ok, but add a comment stating that).
The rmw implementations should have a map between the ros ids and the specific implementations ids, using the macros that both provide.
In that way, we don't depend on future changes of the numbers.

I have changed these values to one-hot bit values, to possibly address the question at #193 (comment) later.

wjwwood · 2020-01-25T01:13:40Z

rmw/include/rmw/types.h

 #define RMW_QOS_LIFESPAN_DEFAULT {0, 0}
 #define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT {0, 0}

+/// RMW equivalent of the DDS QoS Policy IDs


I would say we should not mention DDS in the rmw API or its doc strings, as the underlying middleware may not be DDS. So we should just define what makes sense for our layer and if that happens to be similar to DDS's then we can mention that, but our interface should stand on its own.

wjwwood · 2020-01-25T01:14:07Z

rmw/include/rmw/types.h

+  RMW_QOS_POLICY_ID_LIVELINESS = 8,
+  RMW_QOS_POLICY_ID_RELIABILITY= 11,
+  RMW_QOS_POLICY_ID_HISTORY    = 13,
+  RMW_QOS_POLICY_ID_LIFESPAN   = 21


wjwwood · 2020-01-25T01:16:03Z

rmw/include/rmw/types.h

 #define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT {0, 0}

+/// RMW equivalent of the DDS QoS Policy IDs
+enum RMW_PUBLIC_TYPE rmw_qos_policy_id_t


I'd use the word kind rather than id. For an analogy, if we had an enum for colors we'd not call the enum rmw_color_id_t, we'd just use rmw_color_kind_t or rmw_color_t most likely. So I think this should probably just be rmw_qos_policy_kind_t or rmw_qos_policy_t.

wjwwood · 2020-01-25T01:16:52Z

rmw/include/rmw/types.h

+/// RMW equivalent of the DDS QoS Policy IDs
+enum RMW_PUBLIC_TYPE rmw_qos_policy_id_t
+{
+  RMW_QOS_POLICY_ID_INVALID    = 0,


If we take my other recommendation, these should become RMW_QOS_POLICY_X or RMW_QOS_POLICY_KIND_X where X is each kind, like INVALID or RELIABILITY.

I have changed these enum names to RMW_QOS_POLICY_<X>.

rmw/include/rmw/types.h

wjwwood · 2020-01-25T01:32:42Z

rmw/include/rmw/types.h

+  int32_t total_count_change;
+  /**
+   * The Qos Policy Id of one of the policies that was found to be
+   * incompatible the last time an incompatibility was detected


Is there any logic to which policy id will be here if more than one are incompatible?

I am thinking of last_policy_kind (renamed from last_policy_id) as a bitmask, where each 1 bit represents a different policy kind that was incompatible.

rmw/include/rmw/types.h

wjwwood · 2020-01-25T01:38:44Z

rmw/include/rmw/event.h

  // subscription events
  RMW_EVENT_LIVELINESS_CHANGED,
  RMW_EVENT_REQUESTED_DEADLINE_MISSED,
+  RMW_EVENT_REQUESTED_INCOMPATIBLE_QOS,


This is hard for me to read and make sense of and it is worse in some of the struct descriptions below.

Would another wording make more sense? Maybe RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE would be better. I would read it as:

An RMW_EVENT occurred due to a REQUESTED_QOS being INCOMPATIBLE with the subscription's QoS.

Which I think matches, for example, the deadline one better, which I would read as:

An RMW_EVENT occurred due to a REQUESTED_DEADLINE being MISSED.

Extending this to the other new event, I'd call it RMW_EVENT_OFFERED_QOS_INCOMPATIBLE.

wjwwood · 2020-01-25T01:47:01Z

rmw/include/rmw/types.h

 } rmw_requested_deadline_missed_status_t;

+/// QoS Requested Incompatible QoS information provided by a subscription.
+typedef struct RMW_PUBLIC_TYPE rmw_requested_incompatible_qos_status_t


Maybe you're just basing this off the DDS names, which a good place to start, but status doesn't really make sense to me here. I think that comes from this being the result of a "StatusCondition" in DDS. However, this strikes me as state more than status. I'm not sold on this (I know other similar structs may already use status rather than state), but I think state or event_state are better descriptors. What do other think?

Also, following from my other comment about naming, I'd swap them here too to be like this:

/// Event state for a subscription's 'RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE' events. typedef struct RMW_PUBLIC_TYPE rmw_requested_qos_incompatible_event_state_t { ...

Or we could leave status in place of state and get:

/// Event status for a subscription's 'RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE' events. typedef struct RMW_PUBLIC_TYPE rmw_requested_qos_incompatible_event_status_t { ...

Both options are an improvement in "at-a-glance comprehension", at least for me.

Swapped the positions of incompatible and qos, but let's keep the use of status for consistency.

rmw/include/rmw/incompatible_qos_events_statuses.h

RMW_EVENT_OFFERED_INCOMPATIBLE_QOS to rmw_event_type_t - Defined new structures that encapsulate data for the new events above Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 · 2020-03-24T07:59:24Z

Updated set of pull requests:

TODOs:

~~Add default incompatible QoS callback to rclcpp / rclpy~~ (punted)
Verify functionality of ros2topic changes (similar to for the changes in quality_of_service_demo_cpp / quality_of_service_demo_py)
Re-run tests for the repos

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 · 2020-03-26T16:27:04Z

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/mm318/6e086798710e344feb2df41f94baf717/raw/26f6df7bdb054887b226bac7fc22fca942c04384/ros2_qos.repos
BUILD args: --packages-up-to rclcpp rclpy
TEST args: --packages-select rmw_connext_cpp rmw_connext_shared_cpp rmw_cyclonedds_cpp rmw rcl rclcpp rclpy
Job: ci_launcher

dabonnie · 2020-03-26T18:54:18Z

https://gist.githubusercontent.com/mm318/6e086798710e344feb2df41f94baf717/raw/26f6df7bdb054887b226bac7fc22fca942c04384/ros2_qos.repos

https://ci.ros2.org/job/ci_launcher/5003/console

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2020-03-26T19:11:44Z

rmw/include/rmw/incompatible_qos_events_statuses.h

+   */
+  int32_t total_count_change;
+  /**
+   * The Qos Policy Kind of one of the policies that was found to be


since you've changed the enum to bitflags, is the intent to only return "one of the policies" or is it now intended to return a bitfield with "all of the policies" that were incompatible?

Defining it as a bitflags opens up the possibility of returning multiple policies. However, currently the rmw implementations are still only capable of returning one policy.

mm318 · 2020-03-26T21:00:00Z

Hi @ivanpauno and @wjwwood, do you think the changes up to rcl are ready to merge? I mean the following pull requests:

The test results are here: #193 (comment). I don't think the test failures on Windows and macOS are related to these changes.

ivanpauno · 2020-03-26T21:18:43Z

@mm318 SGTM! CI failures also look unrelated to me.
@wjwwood Do you think that does PRs are ready to be merged?

wjwwood · 2020-03-26T22:20:04Z

Fine by me, I trust you guys to review and merge it since my initial review.

mm318 · 2020-03-27T07:33:09Z

Fine by me, I trust you guys to review and merge it since my initial review.

Let's merge it then please, @ivanpauno. Thanks!

ivanpauno · 2020-03-27T12:27:29Z

@mm318 I merged those four PRs.
Please, open a issue in rmw_fastrtps, to track the future addition of these events.

mm318 · 2020-03-27T15:32:43Z

Thanks!

I have opened ros2/rmw_fastrtps#356 to track the future addition of support for these types of QoS events.

jaisontj marked this pull request as ready for review November 20, 2019 19:22

prajakta-gokhale reviewed Nov 20, 2019

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

piraka9011 reviewed Nov 20, 2019

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

piraka9011 approved these changes Nov 20, 2019

View reviewed changes

jaisontj requested a review from prajakta-gokhale November 20, 2019 19:52

jaisontj mentioned this pull request Nov 20, 2019

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events ros2/rmw_opensplice#294

Closed

prajakta-gokhale approved these changes Nov 20, 2019

View reviewed changes

jaisontj mentioned this pull request Nov 21, 2019

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events ros2/rcl#535

Merged

mjcarroll requested changes Dec 18, 2019

View reviewed changes

ivanpauno reviewed Dec 18, 2019

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

ivanpauno reviewed Dec 18, 2019

View reviewed changes

ivanpauno added the in review Waiting for review (Kanban column) label Dec 18, 2019

wjwwood reviewed Jan 25, 2020

View reviewed changes

mm318 force-pushed the jaisontj/incompatible_qos branch from 3746b42 to 86e698b Compare February 24, 2020 19:38

mm318 force-pushed the jaisontj/incompatible_qos branch from 077e14c to bf50896 Compare March 3, 2020 23:09

ivanpauno approved these changes Mar 4, 2020

View reviewed changes

rmw/include/rmw/incompatible_qos_events_statuses.h Outdated Show resolved Hide resolved

mm318 force-pushed the jaisontj/incompatible_qos branch from ab18f76 to e05106c Compare March 11, 2020 18:12

mm318 mentioned this pull request Mar 17, 2020

Adding support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS event callbacks in ROS2. ros2/ros2#822

Closed

14 tasks

jaisontj and others added 10 commits March 22, 2020 10:10

- Added new enum type RMW_EVENT_REQUESTED_INCOMPATIBLE_QOS and

26b52c6

RMW_EVENT_OFFERED_INCOMPATIBLE_QOS to rmw_event_type_t - Defined new structures that encapsulate data for the new events above Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Added last_policy_id to the new events

4085097

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Comments for newly created structures.

6987af8

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Fixed code formatting issues.

63b298c

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Fixed comment indentation.

a4b6ca4

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Added an enum representing the QosPolicyId_t from DDS

6662afb

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

address PR comments

71cbdfe

Signed-off-by: Miaofei <miaofei@amazon.com>

remove extraneous enum

0ae7095

Signed-off-by: Miaofei <miaofei@amazon.com>

address PR comments

12b79c9

Signed-off-by: Miaofei <miaofei@amazon.com>

fix compilation errors

db521dd

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 force-pushed the jaisontj/incompatible_qos branch from e05106c to db521dd Compare March 22, 2020 18:22

ivanpauno approved these changes Mar 25, 2020

View reviewed changes

uncrustify

a2da1cf

Signed-off-by: Miaofei <miaofei@amazon.com>

ivanpauno approved these changes Mar 26, 2020

View reviewed changes

emersonknapp reviewed Mar 26, 2020

View reviewed changes

ivanpauno merged commit 5356f91 into ros2:master Mar 27, 2020

Conversation

jaisontj commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented Nov 15, 2019

Uh oh!

jaisontj commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

piraka9011 left a comment

Choose a reason for hiding this comment

Uh oh!

mjcarroll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mm318 Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mm318 commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mm318 commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dabonnie commented Mar 26, 2020

Uh oh!

jaisontj commented Nov 15, 2019 •

edited

Loading

jaisontj commented Nov 16, 2019 •

edited

Loading

mm318 Mar 3, 2020 •

edited

Loading

mm318 commented Mar 24, 2020 •

edited

Loading

mm318 commented Mar 26, 2020 •

edited

Loading