Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

jaisontj · 2019-11-15T01:15:07Z

Depends on ros2/rcl:535

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

Added INCOMPATIBLE_QOS event type to QoSPublisherEventType and QoSSubscriptionEventType
Added support for the above events in the QoSEventHandler
modified test_qos_event.py to include the new events.

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

thomas-moulard

LGTM, thanks Jaison!

rclpy/rclpy/qos_event.py

ivanpauno · 2019-12-18T20:57:07Z

rclpy/src/rclpy/_rclpy_qos_event.c

  // Publisher events
  rmw_offered_deadline_missed_status_t offered_deadline_missed;
  rmw_liveliness_lost_status_t liveliness_lost;
+  rmw_offered_incompatible_qos_status_t offered_incompatible_qos;
 } _qos_event_callback_data_t;


The idea of _qos_event_callback_data_t doesn't scale well, it gets bigger and bigger ...
The problem is maybe unrelated to the PR, but creating an issue may worth it.

rclpy/rclpy/qos_event.py

ivanpauno

LGTM!

QoSSubscriptionEventType - Added support for the above events in the QoSEventHandler Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

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

- Added test for incompatible_qos events Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

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

rclpy/rclpy/qos.py

ivanpauno · 2020-03-25T13:09:28Z

rclpy/rclpy/qos.py

+    This enum matches the one defined in rmw/incompatible_qos_events_statuses.h
+    """
+
+    RMW_QOS_POLICY_INVALID = 1 << 0


It would also be good to use the values defined in rmw, instead of manually write them here again.

I don't mind strongly, you can left a TODO.

I am not sure how to do that. Do you have an example?

QoSPublisherEventType and QoSSubscriptionEventType were also done this way.

Not really nice, but here an example:

rclpy/rclpy/src/rclpy/_rclpy_logging.c

Line 244 in 3d1698a

rclpy_get_error_logging_severity(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args))

You could also create the constants in the module init function, but again, not super nice.

I see. I'm not sure if this is worth the effort, because it won't automatically synchronize when new enums are added to rmw_qos_policy_kind_t.

I'll leave a TODO here.

I see. I'm not sure if this is worth the effort, because it won't automatically synchronize when new enums are added to rmw_qos_policy_kind_t.

Sounds good!

ivanpauno · 2020-03-25T13:15:15Z

rclpy/rclpy/qos_event.py

+QoSOfferedIncompatibleQoSInfo = QoSRequestedIncompatibleQoSInfo
+
+
+class UnsupportedEventTypeException(Exception):


RuntimeError sounds like a better base class.
I would also rename the exception to UnsupportedEventTypeError, as Error is usually used as suffix in Python instead of Exception.

Actually, NotImplementedError may be a better base class.

The Python documentation says "All user-defined exceptions should also be derived from this class (Exception)", so I don't think RuntimeError or NotImplementedError are meant to be base classes.

The Python documentation says "All user-defined exceptions should also be derived from this class (Exception)", so I don't think RuntimeError or NotImplementedError are meant to be base classes.

IMHO, that means that isinstance(CustomError(..), Exception) should be True.
That's the case, because the base case of RuntimeError is Exception.
There's a nice hierarchy tree at the end of the page you linked.
AFAIK, it's a usual practice to subclass RuntimeError or any other python built-in exception.
e.g.: RCLError subclass from RuntimeError.

@wjwwood what do you think?

Correct, you can, and where appropriate, you should inherit from other exception types. They just mean you should raise a completely custom class that does not inherit from exception at all, directly or indirectly.

I will move the definition of this exception to _rclpy.c as suggested at #459 (comment) , in which case this exception will inherit from RCLError instead of RuntimeError.

ivanpauno · 2020-03-25T13:25:21Z

rclpy/src/rclpy/_rclpy_qos_event.c

+  PyObject * pyqos_event_exception = NULL;
+
+  if (RCL_RET_UNSUPPORTED == ret) {
+    PyObject * pyqos_event_module = PyImport_ImportModule("rclpy.qos_event");


I think that it's better to define the new exception in C, like this:

rclpy/rclpy/src/rclpy/_rclpy.c

Line 5523 in 3d1698a

RCLError = PyErr_NewExceptionWithDoc(

.

If we define the new exception in C, then it looks like using it from Python module would be something like:

from rclpy.impl.implementation_singleton import rclpy_implementation as _rclpy ... try: ... except _rclpy.UnsupportedEventTypeError as e: ...

whereas if we define it in Python (as it is in this pull request currently), the usage would be like:

from rclpy.qos_event import UnsupportedEventTypeError ... try: ... except UnsupportedEventTypeError as e: ...

I think the latter is better, because you probably wouldn't want a user to be importing rclpy.impl.*

you can import it in rclpy.qos_event and then reexport it.

@wjwwood for a second opinion.

I think both will work. It’s slightly less awkward to avoid importing code in the c extension and then reexport it as @ivanpauno said, but I don’t mind as long as it works.

I have moved the definition of the new exception into C instead of Python.

rclpy/src/rclpy/_rclpy_qos_event.c

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

mm318 · 2020-03-26T07:42:13Z

Are the rclpy tests only run with rmw_fastrtps_cpp? Should we make it run with additional rmw implementations?

ivanpauno · 2020-03-26T17:44:23Z

Are the rclpy tests only run with rmw_fastrtps_cpp? Should we make it run with additional rmw implementations?

I'm ok with running them in only one implementation.
We also have jobs in build.ros2.org where only one implementation is available, so it will cover all off them there.

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

ivanpauno

LGTM! (with green CI)

mm318 · 2020-03-30T22:48:41Z

Thanks!

The following is the set of remaining pull requests for this feature:

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events rclcpp#924
Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459
Added incompatible event support to ros2 topic echo and ros2 topic pub ros2cli#410
Demo to show the working of the incompatible_qos event callbacks. demos#416

After I add some more tests to rclcpp (to address ros2/rclcpp#924 (review)), I'll request to kick of CI.

mm318 · 2020-03-31T07:28:40Z

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/mm318/e32e5d8c1e712f6098101fa6b32efc57/raw/48efc95ff1c41ef1cab6322f2b639221637c34a0/ros2_qos.repos
BUILD args: --packages-up-to ros2topic quality_of_service_demo_cpp quality_of_service_demo_py
TEST args: --packages-select rclcpp rclpy ros2topic quality_of_service_demo_cpp quality_of_service_demo_py
Job: ci_launcher

dabonnie · 2020-03-31T16:37:49Z

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/mm318/e32e5d8c1e712f6098101fa6b32efc57/raw/48efc95ff1c41ef1cab6322f2b639221637c34a0/ros2_qos.repos
BUILD args: --packages-up-to ros2topic quality_of_service_demo_cpp quality_of_service_demo_py
TEST args: --packages-select rclcpp rclpy ros2topic quality_of_service_demo_cpp quality_of_service_demo_py
Job: ci_launcher

Linux
Linux-aarch64
macOS
Windows

mm318 · 2020-03-31T18:57:00Z

I have pushed a fix to ros2/ros2cli#410. @ros2/aws-oncall, can you please re-run CI (#459 (comment))?

dabonnie · 2020-03-31T19:16:15Z

Linux
Linux-aarch64
macOS
Windows

mm318 · 2020-03-31T21:56:34Z

Hi @ivanpauno, can you take a look at the CI results at #459 (comment)? I think the macOS and Windows CI test failures are unrelated.

ivanpauno · 2020-04-01T12:46:38Z

Hi @ivanpauno, can you take a look at the CI results at #459 (comment)? I think the macOS and Windows CI test failures are unrelated.

Seems to be unrelated to me.

mm318 · 2020-04-01T15:04:35Z

Great! Then this set of pull requests are ready to merge:

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events rclcpp#924
Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459
Added incompatible event support to ros2 topic echo and ros2 topic pub ros2cli#410
Demo to show the working of the incompatible_qos event callbacks. demos#416

Thanks, @ivanpauno!

ivanpauno · 2020-04-01T15:16:38Z

ros2/demos#416

Please, address the comment in that one. I will merge the others now.

Revert "switch to slightly more generic isinstance" This reverts commit 77df874. Revert "Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events (#459)" This reverts commit a1477e8.

…_QOS events (#459) Signed-off-by: Jaison Titus <jaisontj92@gmail.com> Signed-off-by: Miaofei <miaofei@amazon.com> Co-authored-by: Miaofei <miaofei@amazon.com>

jaisontj mentioned this pull request Nov 16, 2019

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

Closed

14 tasks

jaisontj force-pushed the jaisontj/incompatible_qos branch from 293d7d3 to b37d9ce Compare November 22, 2019 00:33

jaisontj marked this pull request as ready for review November 22, 2019 00:34

thomas-moulard approved these changes Nov 22, 2019

View reviewed changes

jaisontj mentioned this pull request Nov 27, 2019

Added incompatible event support to ros2 topic echo and ros2 topic pub ros2/ros2cli#410

Merged

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

ivanpauno reviewed Dec 18, 2019

View reviewed changes

wjwwood self-assigned this Jan 9, 2020

mm318 force-pushed the jaisontj/incompatible_qos branch from b37d9ce to 8ec5d35 Compare February 24, 2020 19:38

mm318 force-pushed the jaisontj/incompatible_qos branch from 40a6a9a to c78cb46 Compare March 11, 2020 18:12

ivanpauno approved these changes Mar 17, 2020

View reviewed changes

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

- Added INCOMPATIBLE_QOS event type to QoSPublisherEventType and

ed09bf1

QoSSubscriptionEventType - Added support for the above events in the QoSEventHandler Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Added support for last_policy_id

10cf607

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

- Code style fixes

6a17ca9

- Added test for incompatible_qos events Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

fix compilation issues

d743efa

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

partially address PR comments

b5f7a05

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

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

mm318 mentioned this pull request Mar 24, 2020

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events ros2/rmw#193

Merged

add new UnsupportedEventTypeException type

900772a

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

ivanpauno reviewed Mar 25, 2020

View reviewed changes

address some PR comments

885a520

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

mm318 force-pushed the jaisontj/incompatible_qos branch from a5eaf57 to 885a520 Compare March 26, 2020 07:38

ivanpauno mentioned this pull request Mar 26, 2020

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events ros2/rclcpp#924

Merged

mm318 added 2 commits March 30, 2020 10:36

move UnsupportedEventTypeError to _rclpy.c

2095a30

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

fix test_qos_event.py unit tests

c8d8309

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

ivanpauno approved these changes Mar 30, 2020

View reviewed changes

ivanpauno merged commit a1477e8 into ros2:master Apr 1, 2020

mm318 mentioned this pull request Apr 1, 2020

Add default Incompatible QoS callbacks #533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

jaisontj commented Nov 15, 2019 •

edited

thomas-moulard left a comment

ivanpauno Dec 18, 2019

ivanpauno left a comment

ivanpauno Mar 25, 2020

mm318 Mar 26, 2020

ivanpauno Mar 26, 2020

mm318 Mar 26, 2020

ivanpauno Mar 26, 2020

ivanpauno Mar 25, 2020

ivanpauno Mar 25, 2020

mm318 Mar 26, 2020

ivanpauno Mar 26, 2020

wjwwood Mar 26, 2020

mm318 Mar 30, 2020

ivanpauno Mar 25, 2020

mm318 Mar 26, 2020

ivanpauno Mar 26, 2020

wjwwood Mar 26, 2020

mm318 Mar 30, 2020

mm318 commented Mar 26, 2020 •

edited

ivanpauno commented Mar 26, 2020

ivanpauno left a comment

mm318 commented Mar 30, 2020

mm318 commented Mar 31, 2020

dabonnie commented Mar 31, 2020 •

edited

mm318 commented Mar 31, 2020

dabonnie commented Mar 31, 2020

mm318 commented Mar 31, 2020

ivanpauno commented Apr 1, 2020

mm318 commented Apr 1, 2020

ivanpauno commented Apr 1, 2020

		QoSOfferedIncompatibleQoSInfo = QoSRequestedIncompatibleQoSInfo


		class UnsupportedEventTypeException(Exception):

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

Conversation

jaisontj commented Nov 15, 2019 • edited

thomas-moulard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpauno left a 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

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

mm318 commented Mar 26, 2020 • edited

ivanpauno commented Mar 26, 2020

ivanpauno left a comment

Choose a reason for hiding this comment

mm318 commented Mar 30, 2020

mm318 commented Mar 31, 2020

dabonnie commented Mar 31, 2020 • edited

mm318 commented Mar 31, 2020

dabonnie commented Mar 31, 2020

mm318 commented Mar 31, 2020

ivanpauno commented Apr 1, 2020

mm318 commented Apr 1, 2020

ivanpauno commented Apr 1, 2020

jaisontj commented Nov 15, 2019 •

edited

mm318 commented Mar 26, 2020 •

edited

dabonnie commented Mar 31, 2020 •

edited