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

add QoS Profile/Depth support to Node. #1376

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions rclpy/rclpy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,12 @@
from typing import Optional
from typing import Type
from typing import TYPE_CHECKING
from typing import Union

from rclpy.context import Context
from rclpy.parameter import Parameter
from rclpy.qos import qos_profile_system_default
from rclpy.qos import QoSProfile
from rclpy.signals import install_signal_handlers
from rclpy.signals import SignalHandlerOptions
from rclpy.signals import uninstall_signal_handlers
Expand Down Expand Up @@ -214,6 +217,7 @@ def create_node(
namespace: Optional[str] = None,
use_global_arguments: bool = True,
enable_rosout: bool = True,
rosout_qos_profile: Optional[Union[QoSProfile, int]] = qos_profile_system_default,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the correct "default" profile for the rosout publisher. As far as I can tell, the default here is actually https://github.com/ros2/rcl/blob/d9daca7c0af7ecdcd103ece30bb7c64bc919ef24/rcl/include/rcl/logging_rosout.h#L37-L48 . So I think we should define this to be the same by default.

start_parameter_services: bool = True,
parameter_overrides: Optional[List[Parameter]] = None,
allow_undeclared_parameters: bool = False,
Expand All @@ -233,6 +237,10 @@ def create_node(
:param use_global_arguments: ``False`` if the node should ignore process-wide command line
arguments.
:param enable_rosout: ``False`` if the node should ignore rosout logging.
:param rosout_qos_profile: A QoSProfile or a history depth to apply to rosout publisher.
In the case that a history depth is provided, the QoS history is set to KEEP_LAST,
the QoS history depth is set to the value of the parameter,
and all other QoS settings are set to their default values.
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
:param start_parameter_services: ``False`` if the node should not create parameter services.
:param parameter_overrides: A list of :class:`.Parameter` which are used to override the
initial values of parameters declared on this node.
Expand All @@ -251,6 +259,7 @@ def create_node(
node_name, context=context, cli_args=cli_args, namespace=namespace,
use_global_arguments=use_global_arguments,
enable_rosout=enable_rosout,
rosout_qos_profile=rosout_qos_profile,
start_parameter_services=start_parameter_services,
parameter_overrides=parameter_overrides,
allow_undeclared_parameters=allow_undeclared_parameters,
Expand Down
11 changes: 10 additions & 1 deletion rclpy/rclpy/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
from rclpy.publisher import Publisher
from rclpy.qos import qos_profile_parameter_events
from rclpy.qos import qos_profile_services_default
from rclpy.qos import qos_profile_system_default
from rclpy.qos import QoSProfile
from rclpy.qos_overriding_options import _declare_qos_parameters
from rclpy.qos_overriding_options import QoSOverridingOptions
Expand Down Expand Up @@ -137,6 +138,7 @@ def __init__(
namespace: Optional[str] = None,
use_global_arguments: bool = True,
enable_rosout: bool = True,
rosout_qos_profile: Optional[Union[QoSProfile, int]] = qos_profile_system_default,
start_parameter_services: bool = True,
parameter_overrides: Optional[List[Parameter[Any]]] = None,
allow_undeclared_parameters: bool = False,
Expand All @@ -157,6 +159,10 @@ def __init__(
:param use_global_arguments: ``False`` if the node should ignore process-wide command line
args.
:param enable_rosout: ``False`` if the node should ignore rosout logging.
:param rosout_qos_profile: A QoSProfile or a history depth to apply to rosout publisher.
In the case that a history depth is provided, the QoS history is set to KEEP_LAST
the QoS history depth is set to the value of the parameter,
and all other QoS settings are set to their default value.
:param start_parameter_services: ``False`` if the node should not create parameter
services.
:param parameter_overrides: A list of overrides for initial values for parameters declared
Expand Down Expand Up @@ -194,6 +200,8 @@ def __init__(
if self._context.handle is None or not self._context.ok():
raise NotInitializedException('cannot create node')

rosout_qos_profile = self._validate_qos_or_depth_parameter(rosout_qos_profile)

with self._context.handle:
try:
self.__node = _rclpy.Node(
Expand All @@ -202,7 +210,8 @@ def __init__(
self._context.handle,
cli_args,
use_global_arguments,
enable_rosout
enable_rosout,
rosout_qos_profile.get_c_qos_profile()
)
except ValueError:
# these will raise more specific errors if the name or namespace is bad
Expand Down
9 changes: 7 additions & 2 deletions rclpy/src/rclpy/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,8 @@ Node::Node(
Context & context,
py::object pycli_args,
bool use_global_arguments,
bool enable_rosout)
bool enable_rosout,
py::object rosout_qos_profile)
: context_(context)
{
rcl_ret_t ret;
Expand Down Expand Up @@ -500,6 +501,10 @@ Node::Node(
options.arguments = arguments;
options.enable_rosout = enable_rosout;

if (!rosout_qos_profile.is_none()) {
options.rosout_qos = rosout_qos_profile.cast<rmw_qos_profile_t>();
}

ret = rcl_node_init(
rcl_node_.get(), node_name, namespace_, context.rcl_ptr(), &options);

Expand Down Expand Up @@ -581,7 +586,7 @@ void
define_node(py::object module)
{
py::class_<Node, Destroyable, std::shared_ptr<Node>>(module, "Node")
.def(py::init<const char *, const char *, Context &, py::object, bool, bool>())
.def(py::init<const char *, const char *, Context &, py::object, bool, bool, py::object>())
.def_property_readonly(
"pointer", [](const Node & node) {
return reinterpret_cast<size_t>(node.rcl_ptr());
Expand Down
4 changes: 3 additions & 1 deletion rclpy/src/rclpy/node.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,16 @@ class Node : public Destroyable, public std::enable_shared_from_this<Node>
* \param[in] pycli_args a sequence of command line arguments for just this node, or None
* \param[in] use_global_arguments if true then the node will also use cli arguments on context
* \param[in] enable rosout if true then enable rosout logging
* \param[in] rosout_qos_profile rmw_qos_profile_t object for rosout publisher.
*/
Node(
const char * node_name,
const char * namespace_,
Context & context,
py::object pycli_args,
bool use_global_arguments,
bool enable_rosout);
bool enable_rosout,
py::object rosout_qos_profile);

/// Get the fully qualified name of the node.
/**
Expand Down
24 changes: 24 additions & 0 deletions rclpy/test/test_create_node.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from rclpy.exceptions import InvalidNamespaceException
from rclpy.exceptions import InvalidNodeNameException
from rclpy.parameter import Parameter
from rclpy.qos import qos_profile_sensor_data


class TestCreateNode(unittest.TestCase):
Expand Down Expand Up @@ -82,6 +83,29 @@ def test_create_node_with_parameter_overrides(self):
]
).destroy_node()

def test_create_node_disable_rosout(self):
node_name = 'create_node_test_disable_rosout'
namespace = '/ns'
node = rclpy.create_node(
node_name, namespace=namespace, context=self.context, enable_rosout=False)
node.destroy_node()

def test_create_node_rosout_qos_profile(self):
node_name = 'create_node_test_rosout_rosout_qos_profile'
namespace = '/ns'
node = rclpy.create_node(
node_name, namespace=namespace, context=self.context, enable_rosout=True,
rosout_qos_profile=qos_profile_sensor_data)
node.destroy_node()

def test_create_node_rosout_qos_depth(self):
node_name = 'create_node_test_rosout_rosout_qos_depth'
namespace = '/ns'
node = rclpy.create_node(
node_name, namespace=namespace, context=self.context, enable_rosout=True,
rosout_qos_profile=10)
node.destroy_node()
Comment on lines +86 to +107
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of tests that don't assert anything. I guess these do test that this isn't throwing an exception, but they aren't testing that what we asked for actually happened. Could we assert that rosout exists (or doesn't exist), and that it has the QoS profile we provided?



if __name__ == '__main__':
unittest.main()