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.

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

QoS - API and implemention for event callbacks #6

Closed
wants to merge 11 commits into from

Conversation

emersonknapp
Copy link

@emersonknapp emersonknapp commented Apr 12, 2019

Depends on ros2/rcl#408

Expose QoS event callbacks to the ROS2 Python client.

rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/publisher.py Outdated Show resolved Hide resolved
rclpy/rclpy/qos_event.py Outdated Show resolved Hide resolved
rclpy/rclpy/qos_event.py Outdated Show resolved Hide resolved
rclpy/rclpy/qos_event.py Show resolved Hide resolved
rclpy/rclpy/qos_event.py Outdated Show resolved Hide resolved
rclpy/rclpy/qos_event.py Outdated Show resolved Hide resolved
rclpy/rclpy/subscription.py Outdated Show resolved Hide resolved
rclpy/src/rclpy_common/src/common.c Outdated Show resolved Hide resolved
@emersonknapp
Copy link
Author

For folks looking at this here's a demo app i've been using to try it out

import rclpy
# from rclpy.duration import Duration
from rclpy.executors import SingleThreadedExecutor
from rclpy.node import Node
from rclpy.publisher import PublisherEventCallbacks
# from rclpy.qos import QoSLivelinessPolicy
from rclpy.qos import QoSProfile
from rclpy.subscription import SubscriptionEventCallbacks

from std_msgs.msg import String


class Talker(Node):
    def __init__(self):
        super().__init__('Talker')
        qos = QoSProfile(
            # lifespan=Duration(seconds=0),
            # deadline=Duration(seconds=0),
            # liveliness=QoSLivelinessPolicy.RMW_QOS_POLICY_LIVELINESS_AUTOMATIC,
            # liveliness_lease_duration=Duration(seconds=2),
        )
        self.publisher = self.create_publisher(
            String, 'topic', qos_profile=qos,
            event_callbacks=PublisherEventCallbacks(
                deadline_callback=self.dead_cb,
                liveliness_callback=self.live_cb
            ))
        self.count = 0

        self.timer = self.create_timer(0.6, self.timer_cb)

    def fini(self):
        # Destroy the timer attached to the node explicitly
        # (optional - otherwise it will be done automatically
        # when the garbage collector destroys the node object)
        self.get_logger().info('Shutting down')
        self.destroy_timer(self.timer)
        self.destroy_node()

    def timer_cb(self):
        msg = String()
        msg.data = 'Hello World: %d' % self.count
        self.count += 1
        self.get_logger().info('Publishing: "%s"' % msg.data)
        self.publisher.publish(msg)

    def dead_cb(self, evt):
        self.get_logger().info('DEADLINE missed: "{}"'.format(evt))

    def live_cb(self, evt):
        self.get_logger().info('LIVELINESS failed: "{}"'.format(evt))


class Listener(Node):
    def __init__(self):
        super().__init__('Listener')
        self.subscription = self.create_subscription(
            String, 'topic', self.msg_cb,
            event_callbacks=SubscriptionEventCallbacks(
                deadline_callback=self.dead_cb,
                liveliness_callback=self.live_cb
            ))

    def msg_cb(self, msg):
        self.get_logger().info('Heard: "%s"' % msg.data)

    def dead_cb(self, evt):
        self.get_logger().info('DEADLINE missed: "{}"'.format(evt))

    def live_cb(self, evt):
        self.get_logger().info('LIVELINESS changed: "{}"'.format(evt))

def main(args=None):
    rclpy.init(args=args)

    executor = SingleThreadedExecutor()

    talker = Talker()
    listener = Listener()

    def killa():
        executor.remove_node(talker)
        talker.fini()

    killswitch = talker.create_timer(4.0, killa)  # NOQA

    executor.add_node(talker)
    executor.add_node(listener)

    executor.spin()

    talker.fini()
    rclpy.shutdown()


if __name__ == '__main__':
    main()

@emersonknapp emersonknapp force-pushed the qos-4-callbacks branch 3 times, most recently from b6f4478 to 0f8b8b0 Compare April 16, 2019 20:25
@emersonknapp emersonknapp changed the title QoS callbacks - sanity check QoS - API and implemention for event callbacks Apr 16, 2019
@emersonknapp emersonknapp changed the base branch from master to qos_reviewed April 16, 2019 20:27
@emersonknapp emersonknapp changed the base branch from qos_reviewed to master April 16, 2019 20:27
@emersonknapp
Copy link
Author

@thomas-moulard I think this is ready (except for one last TODO that I am tracking down) it seems like you've got a bit of experience with C-Python interfaces, can you give this a once-over review before I open up the review to openrobotics?

qos_profile: QoSProfile = qos_profile_default
qos_profile: QoSProfile = qos_profile_default,
callback_group: CallbackGroup = None,
event_callbacks: PublisherEventCallbacks = PublisherEventCallbacks(),
Copy link

Choose a reason for hiding this comment

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

One of the subtleties of default parameters in Python is that they are only evaluated once when the function is defined instead of every time the function is executed. This means that every call to create_publisher will use the same default PublisherEventCallbacks() object instance. The safe option is to use None as the default value and then construct a new instance of the object within the function itself when providing a default for mutable objects.

I don't think it breaks anything right now, so I wouldn't consider this a blocking issue. I wanted to bring it up anyways in case I'm missing something in how it's used.

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

Good catch - I'll default to None

self.event_handlers: List[QoSEventHandler] = []
if event_callbacks:
if event_callbacks.deadline:
self.event_handlers.append(QoSEventHandler(
Copy link

Choose a reason for hiding this comment

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

Kind of a nitpick, but I think it would be better if there was a function in the PublisherEventCallbacks class that returns a list of QoSEventHandler objects for all the event handlers in the class. My reasoning is that if someone adds a new type of event handle in the future it is more obvious that they need to add it to the list in that class than it is this list in an unrelated file.

Copy link
Author

Choose a reason for hiding this comment

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

I like that idea - will do

rclpy/rclpy/qos_event.py Show resolved Hide resolved

self.event_callbacks = event_callbacks
self.event_handlers: List[QoSEventHandler] = []
if event_callbacks:
Copy link

Choose a reason for hiding this comment

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

Same comment here as in the Publisher.

qos_profile: QoSProfile = qos_profile_default
qos_profile: QoSProfile = qos_profile_default,
callback_group: CallbackGroup = None,
event_callbacks: PublisherEventCallbacks = PublisherEventCallbacks(),

Choose a reason for hiding this comment

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

+1

@@ -348,16 +350,24 @@ def create_publisher(
msg_type,
topic: str,
*,
qos_profile: QoSProfile = qos_profile_default
qos_profile: QoSProfile = qos_profile_default,
callback_group: CallbackGroup = None,

Choose a reason for hiding this comment

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

callback_group type is Optional[CallbackGroup] as None is allowed.

:return: The new publisher.
"""
if callback_group is None:

Choose a reason for hiding this comment

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

I like this syntax, you don't need to use it but FYI

callback_group = callback_group or self.default_callback_group

@@ -380,6 +398,7 @@ def create_subscription(
*,
qos_profile: QoSProfile = qos_profile_default,
callback_group: CallbackGroup = None,
event_callbacks: SubscriptionEventCallbacks = SubscriptionEventCallbacks(),

Choose a reason for hiding this comment

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

ditto

self.event_callbacks = event_callbacks
self.event_handlers: List[QoSEventHandler] = []
if event_callbacks:
if event_callbacks.deadline:

Choose a reason for hiding this comment

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

Sounds like this could be split into multiple functions, this starts to be a bit long

Copy link
Author

Choose a reason for hiding this comment

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

Going to implement as function on EventCallbacks to keep centralized - as per Nick's comment below


Mirrors rmw_requested_deadline_missed_status_t from rmw/types.h
"""
QoSRequestedDeadlineMissedInfo = namedtuple(

Choose a reason for hiding this comment

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

rclpy/rclpy/qos_event.py Show resolved Hide resolved
rclpy/rclpy/qos_event.py Outdated Show resolved Hide resolved
rclpy/rclpy/qos_event.py Show resolved Hide resolved
_rclpy_destroy_event(PyObject * pycapsule)
{
rcl_event_t * event = PyCapsule_GetPointer(pycapsule, "rcl_event_t");
rcl_ret_t ret_event = rcl_event_fini(event);

Choose a reason for hiding this comment

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

Order sounds wrong there, freeing the C underlying object before the python one?

Copy link
Author

Choose a reason for hiding this comment

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

This is called automatically as the destructor of the allocated Capsule when it is destroyed by Python https://github.com/aws-robotics/rclpy/pull/6/files#diff-80dda03110b5be606b823d6b0558b5c2R1416

There isn't a Python object for me to free.

Copy link
Author

Choose a reason for hiding this comment

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

Unless I have misunderstood your comment - and you mean event should be freed first - but I have to send event to fini before that, so its members can be freed - if I were to free event first I couldn't access its members any more to free them.

from rclpy.impl.implementation_singleton import rclpy_implementation as _rclpy
from rclpy.qos import QoSProfile
from rclpy.qos_event import PublisherEventCallbacks
from rclpy.qos_event import QoSEventHandler
Copy link

@mm318 mm318 Apr 17, 2019

Choose a reason for hiding this comment

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

I think we both got the naming convention wrong. In rclcpp it's QOSEventHandler, here it's QoSEventHandler, but the Google style guide would've wanted QosEventHandler (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules). Let's change it?

Copy link
Author

Choose a reason for hiding this comment

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

The existing python codebase uses QoS (AaA) for the functionality that was already here. I'm going to keep it this way to match the existing convention.

See https://github.com/ros2/rclpy/blob/master/rclpy/rclpy/qos.py#L21

Copy link

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

We should run ASAN on this PR before merging it, it's hard to get the Python ref-counting right.

* \return NULL on failure
*/
static PyObject *
rclpy_create_event(PyObject * Py_UNUSED(self), PyObject * args)

Choose a reason for hiding this comment

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

Big function - can we split this?

Copy link
Author

Choose a reason for hiding this comment

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

C is pretty verbose - do you really think this is actually more than one logical unit of code?

Choose a reason for hiding this comment

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

Here is a re-implementation of your function (coding style, passing precisely the args etc. is left as an exercise for the reader :)

bool _is_pycapsule_publisher(PyObject* pyobject) {
  return PyCapsule_IsValid(pyparent, "rcl_subscription_t");
}

bool _is_pycapsule_subscription(PyObject* pyobject) {
  return PyCapsule_IsValid(pyparent, "rcl_publisher_t");
}

rcl_event_t * _allocate_and_zero_initialize_rcl_event() {
  rcl_event_t * event = (rcl_event_t *)PyMem_Malloc(sizeof(rcl_event_t));
  if (!event) {
    PyErr_Format(PyExc_MemoryError, "Failed to allocate memory for event");
    return NULL;
  }
  *event = rcl_get_zero_initialized_event();
  return event;
}

// forcing yourself to divide your code in smaller functions will help build
// generally useful utility functions such as this one instead of considering
// the code "write-only".
bool _check_rcl_errors_status() {
  if (ret != RCL_RET_OK) {
    rcl_reset_error();
    PyErr_Format(PyExc_RuntimeError, rcutils_get_error_string());
    return true;
  } else {
    return false;
  }
}

bool _initialize_event_publisher(event) {
  const rcl_ret_t ret = rcl_subscription_event_init(event, subscription, pyevent_type);
  return _check_rcl_error_status();
}

bool _initialize_event_subscription(event) {
  const rcl_ret_t ret = rcl_subscription_event_init(event, subscription, pyevent_type);
  return _check_rcl_error_status();
}

bool _initialize_event(event) {
  if(_is_pycapsule_publisher(event)) {
    return _initialize_event_publisher();
  } else if(_is_pycapsule_subscription(event)) {
    return _initialize_event_subscription();
  }
}

static PyObject *
rclpy_create_event(PyObject * Py_UNUSED(self), PyObject * args) {
  const rcl_event_t * event = _allocate_and_zero_initialize_rcl_event();
  if (!event) {
    return NULL;
  }
  if (_initialize_event(event)) {
    return event;
  } else {
    PyMem_Free(event);  // Nesting functions will help you ensuring cleanup code is needed only ONCE.
    return NULL;
  }
}

* \return NULL on failure
*/
static PyObject *
rclpy_take_event(PyObject * Py_UNUSED(self), PyObject * args)

Choose a reason for hiding this comment

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

Big function - can we split this?

{
"rclpy_take_event", rclpy_take_event, METH_VARARGS,
"Get the pending data for a ready QoS Event."
},

Choose a reason for hiding this comment

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

This file looks huge, time to break into one file per python function or something? (at least partially split this)

Copy link
Author

Choose a reason for hiding this comment

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

It is huge - but i'm kind of in favor of big code files for feature sets - single compilation unit usually builds faster than code with lots of headers and you always know where to find what you're looking for. Regardless - I'd be hesitant to break it out as part of this feature - that feels like its own dedicated change.

PyObject * pyqos_event_class = NULL;
PyObject * pyqos_event = NULL;

pyqos_event_module = PyImport_ImportModule("rclpy.qos_event");

Choose a reason for hiding this comment

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

Let's have a single cleanup logic using Py_XDECREF on all variables to reduce the complexity of this code.

Copy link
Author

Choose a reason for hiding this comment

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

ok - I'll restructure it a bit

@thomas-moulard
Copy link

thomas-moulard commented Apr 17, 2019

Additionally, this PR needs unit tests. It can be python code, but it should call directly each bound symbol one by one.

@emersonknapp
Copy link
Author

Working on adding some unit tests still - but @thomas-moulard do you like the look of it better this way? I tried to make the functions more readable, and I see the value in abstracting away some of the string-based functions, so that those potential mistakes can at least be centralized.

@emersonknapp emersonknapp changed the base branch from master to qos April 18, 2019 01:06
@emersonknapp emersonknapp changed the base branch from qos to master April 18, 2019 01:07
sloretz and others added 4 commits April 19, 2019 11:02
* Every executor gets its own SIGINT guard condition

Moves signal handling code to _rclpy_signal_handler
Every executor adds a guard condition to a global list
SIGINT signal handler triggers all guard conditions in global list

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* _sigint_gc robust to shutdown() called twice

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Remove redundant comments

Comments say the same thing twice.
It only needs to be said once.
Remove extra comments so the same thing is not repeated.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Split loop for readability

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* g_guard_conditions atomic variable

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Use rclutils_atomics macros

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Call original handler before losing reference to it

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* remove extra unnecessary assignment

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* g_guard_conditions is a struct on windows

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Rename action state transitions (ros2#300)

* Rename action state transitions

Now using active verbs as described in the design doc:

http://design.ros2.org/articles/actions.html#goal-states

Connects to ros2/rcl#399.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* add missing error handling and cleanup (ros2#315)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* Don't store sigint_gc address

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* remove redundant conditional

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Every executor gets its own SIGINT guard condition

Moves signal handling code to _rclpy_signal_handler
Every executor adds a guard condition to a global list
SIGINT signal handler triggers all guard conditions in global list

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* _sigint_gc robust to shutdown() called twice

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Remove redundant comments

Comments say the same thing twice.
It only needs to be said once.
Remove extra comments so the same thing is not repeated.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Split loop for readability

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* g_guard_conditions atomic variable

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Use rclutils_atomics macros

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Call original handler before losing reference to it

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* remove extra unnecessary assignment

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* g_guard_conditions is a struct on windows

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Don't store sigint_gc address

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* remove redundant conditional

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
* add tests ensuring TypeError is raised

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* raise TypeError when passing invalid message types

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Emerson Knapp added 7 commits April 23, 2019 19:31
…in my test app, not in the rclpy functionality

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Author

Closing in favor of ros2#316

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

Successfully merging this pull request may close these issues.

6 participants