Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

Merged
merged 9 commits into from
Apr 1, 2020
Merged

Support for ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS events #459

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed09bf1
- Added INCOMPATIBLE_QOS event type to QoSPublisherEventType and
jaisontj Nov 12, 2019
10cf607
Added support for last_policy_id
jaisontj Nov 15, 2019
6a17ca9
- Code style fixes
jaisontj Nov 21, 2019
d743efa
fix compilation issues
mm318 Mar 11, 2020
b5f7a05
partially address PR comments
mm318 Mar 11, 2020
900772a
add new UnsupportedEventTypeException type
mm318 Mar 24, 2020
885a520
address some PR comments
mm318 Mar 26, 2020
2095a30
move UnsupportedEventTypeError to _rclpy.c
mm318 Mar 30, 2020
c8d8309
fix test_qos_event.py unit tests
mm318 Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 36 additions & 1 deletion rclpy/rclpy/qos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,43 @@
from rclpy.impl.implementation_singleton import rclpy_implementation as _rclpy


class QoSPolicyKind(IntEnum):
"""
Enum for types of QoS policies that a Publisher or Subscription can set.

This enum matches the one defined in rmw/incompatible_qos_events_statuses.h
"""

RMW_QOS_POLICY_INVALID = 1 << 0
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

QoSPublisherEventType and QoSSubscriptionEventType were also done this way.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

RMW_QOS_POLICY_DURABILITY = 1 << 1
RMW_QOS_POLICY_DEADLINE = 1 << 2
RMW_QOS_POLICY_LIVELINESS = 1 << 3
RMW_QOS_POLICY_RELIABILITY = 1 << 4
RMW_QOS_POLICY_HISTORY = 1 << 5
RMW_QOS_POLICY_LIFESPAN = 1 << 6


def qos_policy_name_from_kind(policy_kind: QoSPolicyKind):
"""Get QoS policy name from QoSPolicyKind enum."""

if policy_kind == RMW_QOS_POLICY_DURABILITY:
return 'DURABILITY_QOS_POLICY'
elif policy_kind == RMW_QOS_POLICY_DEADLINE:
return 'DEADLINE_QOS_POLICY'
elif policy_kind == RMW_QOS_POLICY_LIVELINESS:
return 'LIVELINESS_QOS_POLICY'
elif policy_kind == RMW_QOS_POLICY_RELIABILITY:
return 'RELIABILITY_QOS_POLICY'
elif policy_kind == RMW_QOS_POLICY_HISTORY:
return 'HISTORY_QOS_POLICY'
elif policy_kind == RMW_QOS_POLICY_LIFESPAN:
return 'LIFESPAN_QOS_POLICY'
else:
return 'INVALID_QOS_POLICY'


class InvalidQoSProfileException(Exception):
"""Raised when concstructing a QoSProfile with invalid arguments."""
"""Raised when constructing a QoSProfile with invalid arguments."""

def __init__(self, *args):
Exception.__init__(self, 'Invalid QoSProfile', *args)
Expand Down
46 changes: 45 additions & 1 deletion rclpy/rclpy/qos_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class QoSPublisherEventType(IntEnum):

RCL_PUBLISHER_OFFERED_DEADLINE_MISSED = 0
RCL_PUBLISHER_LIVELINESS_LOST = 1
RCL_PUBLISHER_OFFERED_INCOMPATIBLE_QOS = 2


class QoSSubscriptionEventType(IntEnum):
Expand All @@ -46,6 +47,7 @@ class QoSSubscriptionEventType(IntEnum):

RCL_SUBSCRIPTION_REQUESTED_DEADLINE_MISSED = 0
RCL_SUBSCRIPTION_LIVELINESS_CHANGED = 1
RCL_SUBSCRIPTION_REQUESTED_INCOMPATIBLE_QOS = 2


"""
Expand All @@ -72,6 +74,18 @@ class QoSSubscriptionEventType(IntEnum):
('not_alive_count_change', 'int'),
])

"""
Payload type for Subscription Incompatible QoS callback.

Mirrors rmw_requested_incompatible_qos_status_t from rmw/types.h
"""
QoSRequestedIncompatibleQoSInfo = NamedTuple(
'QoSRequestedIncompatibleQoSInfo', [
('total_count', 'int'),
('total_count_change', 'int'),
('last_policy_kind', 'int'),
])

"""
Payload type for Publisher Deadline callback.

Expand All @@ -94,6 +108,20 @@ class QoSSubscriptionEventType(IntEnum):
('total_count_change', 'int'),
])

"""
Payload type for Publisher Incompatible QoS callback.

Mirrors rmw_offered_incompatible_qos_status_t from rmw/types.h
"""
QoSOfferedIncompatibleQoSInfo = QoSRequestedIncompatibleQoSInfo


class UnsupportedEventTypeException(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, NotImplementedError may be a better base class.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

"""Raised when registering a callback for an event type that is not supported."""

def __init__(self, *args):
Exception.__init__(self, 'QoS event type is unsupported', *args)


class QoSEventHandler(Waitable):
"""Waitable type to handle QoS events."""
Expand Down Expand Up @@ -160,6 +188,7 @@ def __init__(
*,
deadline: Optional[Callable[[QoSRequestedDeadlineMissedInfo], None]] = None,
liveliness: Optional[Callable[[QoSLivelinessChangedInfo], None]] = None,
incompatible_qos: Optional[Callable[[QoSRequestedIncompatibleQoSInfo], None]] = None,
) -> None:
"""
Create a SubscriptionEventCallbacks container.
Expand All @@ -171,6 +200,7 @@ def __init__(
"""
self.deadline = deadline
self.liveliness = liveliness
self.incompatible_qos = incompatible_qos

def create_event_handlers(
self, callback_group: CallbackGroup, subscription_handle: Handle,
Expand All @@ -188,6 +218,12 @@ def create_event_handlers(
callback=self.liveliness,
event_type=QoSSubscriptionEventType.RCL_SUBSCRIPTION_LIVELINESS_CHANGED,
parent_handle=subscription_handle))
if self.incompatible_qos:
event_handlers.append(QoSEventHandler(
callback_group=callback_group,
callback=self.incompatible_qos,
event_type=QoSSubscriptionEventType.RCL_SUBSCRIPTION_REQUESTED_INCOMPATIBLE_QOS,
parent_handle=subscription_handle))
return event_handlers


Expand All @@ -198,7 +234,8 @@ def __init__(
self,
*,
deadline: Optional[Callable[[QoSOfferedDeadlineMissedInfo], None]] = None,
liveliness: Optional[Callable[[QoSLivelinessLostInfo], None]] = None
liveliness: Optional[Callable[[QoSLivelinessLostInfo], None]] = None,
incompatible_qos: Optional[Callable[[QoSRequestedIncompatibleQoSInfo], None]] = None
) -> None:
"""
Create and return a PublisherEventCallbacks container.
Expand All @@ -210,6 +247,7 @@ def __init__(
"""
self.deadline = deadline
self.liveliness = liveliness
self.incompatible_qos = incompatible_qos

def create_event_handlers(
self, callback_group: CallbackGroup, publisher_handle: Handle,
Expand All @@ -227,4 +265,10 @@ def create_event_handlers(
callback=self.liveliness,
event_type=QoSPublisherEventType.RCL_PUBLISHER_LIVELINESS_LOST,
parent_handle=publisher_handle))
if self.incompatible_qos:
event_handlers.append(QoSEventHandler(
callback_group=callback_group,
callback=self.incompatible_qos,
event_type=QoSPublisherEventType.RCL_PUBLISHER_OFFERED_INCOMPATIBLE_QOS,
parent_handle=publisher_handle))
return event_handlers
75 changes: 68 additions & 7 deletions rclpy/src/rclpy/_rclpy_qos_event.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@

#include "rcl/event.h"
#include "rclpy_common/handle.h"
#include "rmw/incompatible_qos_events_statuses.h"

typedef union _qos_event_callback_data {
// Subscription events
rmw_requested_deadline_missed_status_t requested_deadline_missed;
rmw_liveliness_changed_status_t liveliness_changed;
rmw_requested_qos_incompatible_event_status_t requested_incompatible_qos;
// Publisher events
rmw_offered_deadline_missed_status_t offered_deadline_missed;
rmw_liveliness_lost_status_t liveliness_lost;
rmw_offered_qos_incompatible_event_status_t offered_incompatible_qos;
} _qos_event_callback_data_t;
Copy link
Member

Choose a reason for hiding this comment

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

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.


typedef PyObject * (* _qos_event_data_filler_function)(_qos_event_callback_data_t *);
Expand All @@ -30,14 +33,36 @@ static
bool
_check_rcl_return(rcl_ret_t ret, const char * error_msg)
{
if (RCL_RET_OK != ret) {
PyErr_Format(
PyExc_RuntimeError,
"%s: %s", error_msg, rcl_get_error_string().str);
rcl_reset_error();
return false;
if (RCL_RET_OK == ret) {
return true;
}

PyObject * exception = PyExc_RuntimeError;
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
PyObject * pyqos_event_module = NULL;
PyObject * pyqos_event_exception = NULL;

if (RCL_RET_UNSUPPORTED == ret) {
PyObject * pyqos_event_module = PyImport_ImportModule("rclpy.qos_event");
Copy link
Member

Choose a reason for hiding this comment

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

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(
.

Copy link
Member

Choose a reason for hiding this comment

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

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.*

Copy link
Member

Choose a reason for hiding this comment

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

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

@wjwwood for a second opinion.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

if (NULL == pyqos_event_module) {
goto throw_exception;
}

PyObject * pyqos_event_exception = PyObject_GetAttrString(pyqos_event_module, "UnsupportedEventTypeException");
if (NULL == pyqos_event_exception) {
goto throw_exception;
}

exception = pyqos_event_exception;
}
return true;

throw_exception:
PyErr_Format(exception, "%s: %s", error_msg, rcl_get_error_string().str);
rcl_reset_error();

Py_XDECREF(pyqos_event_module);
Py_XDECREF(pyqos_event_exception);

return false;
}

static
Expand Down Expand Up @@ -159,6 +184,22 @@ _liveliness_changed_to_py_object(_qos_event_callback_data_t * data)
return _create_py_qos_event("QoSLivelinessChangedInfo", args);
}

static
PyObject *
_requested_incompatible_qos_to_py_object(_qos_event_callback_data_t * data)
{
rmw_requested_qos_incompatible_event_status_t * actual_data = &data->requested_incompatible_qos;
PyObject * args = Py_BuildValue(
"iii",
actual_data->total_count,
actual_data->total_count_change,
actual_data->last_policy_kind);
if (!args) {
return NULL;
}
return _create_py_qos_event("QoSRequestedIncompatibleQoSInfo", args);
}

static
PyObject *
_offered_deadline_missed_to_py_object(_qos_event_callback_data_t * data)
Expand Down Expand Up @@ -189,6 +230,22 @@ _liveliness_lost_to_py_object(_qos_event_callback_data_t * data)
return _create_py_qos_event("QoSLivelinessLostInfo", args);
}

static
PyObject *
_offered_incompatible_qos_to_py_object(_qos_event_callback_data_t * data)
{
rmw_offered_qos_incompatible_event_status_t * actual_data = &data->offered_incompatible_qos;
PyObject * args = Py_BuildValue(
"iii",
actual_data->total_count,
actual_data->total_count_change,
actual_data->last_policy_kind);
if (!args) {
return NULL;
}
return _create_py_qos_event("QoSOfferedIncompatibleQoSInfo", args);
}

static
_qos_event_data_filler_function
_get_qos_event_data_filler_function_for(PyObject * pyparent, unsigned PY_LONG_LONG event_type)
Expand All @@ -199,6 +256,8 @@ _get_qos_event_data_filler_function_for(PyObject * pyparent, unsigned PY_LONG_LO
return &_requested_deadline_missed_to_py_object;
case RCL_SUBSCRIPTION_LIVELINESS_CHANGED:
return &_liveliness_changed_to_py_object;
case RCL_SUBSCRIPTION_REQUESTED_INCOMPATIBLE_QOS:
return &_requested_incompatible_qos_to_py_object;
default:
PyErr_Format(
PyExc_ValueError,
Expand All @@ -210,6 +269,8 @@ _get_qos_event_data_filler_function_for(PyObject * pyparent, unsigned PY_LONG_LO
return &_offered_deadline_missed_to_py_object;
case RCL_PUBLISHER_LIVELINESS_LOST:
return &_liveliness_lost_to_py_object;
case RCL_PUBLISHER_OFFERED_INCOMPATIBLE_QOS:
return &_offered_incompatible_qos_to_py_object;
default:
PyErr_Format(
PyExc_ValueError,
Expand Down