Add wait_for_all_acked support #793

Barry-Xu-2018 · 2021-05-10T06:50:09Z

fujitatomoya · 2021-05-12T01:33:29Z

rclpy/rclpy/publisher.py

+            false.
+        """
+        with self.handle:
+            return self.__publisher.wait_for_all_acked(timeout._duration_handle)


it should use try&except statement? Publisher::wait_for_all_acked would throw RCLError exception.

After check other codes, I find it also doesn't deal with RCLError.
e.g.

rclpy/rclpy/rclpy/publisher.py

Lines 59 to 73 in 714f69e

def publish(self, msg: Union[MsgType, bytes]) -> None:

"""

Send a message to the topic for the publisher.

:param msg: The ROS message to publish.

:raises: TypeError if the type of the passed message isn't an instance

of the provided type when the publisher was constructed.

"""

with self.handle:

if isinstance(msg, self.msg_type):

self.__publisher.publish(msg)

elif isinstance(msg, bytes):

self.__publisher.publish_raw(msg)

else:

raise TypeError('Expected {}, got {}'.format(self.msg_type, type(msg)))

Not catch exception

The underlying code may throw exception

rclpy/rclpy/src/rclpy/publisher.cpp

Lines 121 to 133 in 714f69e

void

Publisher::publish(py::object pymsg)

{

auto raw_ros_message = convert_from_py(pymsg);

if (!raw_ros_message) {

throw py::error_already_set();

}

rcl_ret_t ret = rcl_publish(rcl_publisher_.get(), raw_ros_message.get(), NULL);

if (RCL_RET_OK != ret) {

throw RCLError("Failed to publish");

}

}

fujitatomoya · 2021-05-12T01:37:27Z

rclpy/test/test_publisher.py

@@ -75,6 +76,26 @@ def do_test_topic_name(cls, test_topics, node):
            assert publisher.topic_name == target_topic
            publisher.destroy()

+    @classmethod
+    def do_test_wait_for_all_acked(cls, test_topic, node):
+        pub = node.create_publisher(BasicTypes, test_topic, 1)


QoS Reliable setting would be nice to check too.

Barry-Xu-2018 · 2021-05-17T09:44:16Z

Friendly ping @fujitatomoya. Please look at my comments and I supplement tests.

fujitatomoya · 2021-05-18T12:12:40Z

CC: @ivanpauno @wjwwood friendly ping.

sloretz

LGTM, since it's been a while I made some changes pushed it to your branch directly

I would like it to be reviewed by once more by someone who hasn't edited this PR

rclpy/rclpy/publisher.py

rclpy/src/rclpy/publisher.cpp

rclpy/test/test_publisher.py

sloretz · 2021-08-12T18:14:49Z

rclpy/test/test_publisher.py

+
+        # Publisher is reliable and subscription is best effort
+        pub_qos.reliability = rclpy.qos.QoSReliabilityPolicy.RELIABLE
+        TestPublisher.do_test_wait_for_all_acked(TEST_TOPIC, self.node, pub_qos, sub_qos)


I don't think there's a need for trying the different QoS options - it seems unrelated to the feature being tested.

rclpy/src/rclpy/publisher.hpp

rclpy/src/rclpy/publisher.cpp

Signed-off-by: Barry Xu <barry.xu@sony.com>

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

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2021-09-02T06:41:36Z

Do rebase.

Barry-Xu-2018 · 2021-09-02T06:48:00Z

rcl package isn't updated (ros2/rcl#913 isn't included). So CI failed.

14:41:44 /tmp/ws/src/rclpy/rclpy/src/rclpy/publisher.cpp: In member function ‘bool rclpy::Publisher::wait_for_all_acked(rcl_duration_t)’:
14:41:44 /tmp/ws/src/rclpy/rclpy/src/rclpy/publisher.cpp:149:19: error: ‘rcl_publisher_wait_for_all_acked’ was not declared in this scope
14:41:44   149 |   rcl_ret_t ret = rcl_publisher_wait_for_all_acked(rcl_publisher_.get(), pytimeout.nanoseconds);
14:41:44       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
14:41:44 make[2]: *** [CMakeFiles/_rclpy_pybind11.dir/build.make:297: CMakeFiles/_rclpy_pybind11.dir/src/rclpy/publisher.cpp.o] Error 1
14:41:44 make[1]: *** [CMakeFiles/Makefile2:193: CMakeFiles/_rclpy_pybind11.dir/all] Error 2
14:41:44 make: *** [Makefile:141: all] Error 2

ivanpauno · 2021-09-02T13:08:37Z

Released rcl (ros/rosdistro#30681), the Rpr checker should start passing in some hours.

Barry-Xu-2018 · 2021-09-07T08:11:34Z

@ros-pull-request-builder retest this please

ivanpauno · 2021-09-13T14:34:46Z

@Barry-Xu-2018 @fujitatomoya can you run ci?
This is ready to go

fujitatomoya · 2021-09-13T15:40:05Z

CI:

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2021-09-15T23:12:21Z

rclpy/test/test_publisher.py

@@ -103,6 +105,29 @@ def test_topic_name_remapping(self):
        ]
        TestPublisher.do_test_topic_name(test_topics, self.node)

+    def test_wait_for_all_acked(self):


This test is failing with Connext #824

Barry-Xu-2018 mentioned this pull request May 11, 2021

Add ability to confirm reader receive all sent messages ros2/rmw#295

Closed

fujitatomoya reviewed May 12, 2021

View reviewed changes

fujitatomoya added the enhancement New feature or request label May 18, 2021

clalancette assigned sloretz Jun 4, 2021

sloretz force-pushed the topic-wait_for_all_acked-feature branch from 85c8ac1 to 4cd0524 Compare August 12, 2021 17:57

sloretz approved these changes Aug 12, 2021

View reviewed changes

ivanpauno approved these changes Aug 13, 2021

View reviewed changes

rclpy/src/rclpy/publisher.cpp Show resolved Hide resolved

ivanpauno approved these changes Aug 13, 2021

View reviewed changes

Barry-Xu-2018 and others added 8 commits September 1, 2021 13:24

Add wait_for_all_acked support

ed6a84f

Signed-off-by: Barry Xu <barry.xu@sony.com>

Add tests

c38fd0e

Signed-off-by: Barry Xu <barry.xu@sony.com>

Fix some grammar issues

1a92c4a

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

Swap comparison order

024c223

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

Remove unnecessary else

10af959

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

Add missing include

c1461c3

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

Address feedback on test

0ceb768

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

Update the description of wait_for_all_acked

e1c6ca2

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 force-pushed the topic-wait_for_all_acked-feature branch from 2efbaa3 to e1c6ca2 Compare September 2, 2021 06:39

ivanpauno approved these changes Sep 13, 2021

View reviewed changes

ivanpauno merged commit a1b1cc2 into ros2:master Sep 13, 2021

jacobperron reviewed Sep 15, 2021

View reviewed changes

jacobperron mentioned this pull request Sep 15, 2021

test_publisher.test_wait_for_all_acked failing with Connext #824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wait_for_all_acked support #793

Add wait_for_all_acked support #793

Barry-Xu-2018 commented May 10, 2021

fujitatomoya May 12, 2021

Barry-Xu-2018 May 14, 2021

fujitatomoya May 12, 2021

Barry-Xu-2018 commented May 17, 2021

fujitatomoya commented May 18, 2021

sloretz left a comment

sloretz Aug 12, 2021

sloretz Aug 12, 2021 •

edited

Loading

Barry-Xu-2018 commented Sep 2, 2021

Barry-Xu-2018 commented Sep 2, 2021

ivanpauno commented Sep 2, 2021

Barry-Xu-2018 commented Sep 7, 2021

ivanpauno commented Sep 13, 2021

fujitatomoya commented Sep 13, 2021

jacobperron Sep 15, 2021

	def publish(self, msg: Union[MsgType, bytes]) -> None:
	"""
	Send a message to the topic for the publisher.

	:param msg: The ROS message to publish.
	:raises: TypeError if the type of the passed message isn't an instance
	of the provided type when the publisher was constructed.
	"""
	with self.handle:
	if isinstance(msg, self.msg_type):
	self.__publisher.publish(msg)
	elif isinstance(msg, bytes):
	self.__publisher.publish_raw(msg)
	else:
	raise TypeError('Expected {}, got {}'.format(self.msg_type, type(msg)))

	void
	Publisher::publish(py::object pymsg)
	{
	auto raw_ros_message = convert_from_py(pymsg);
	if (!raw_ros_message) {
	throw py::error_already_set();
	}

	rcl_ret_t ret = rcl_publish(rcl_publisher_.get(), raw_ros_message.get(), NULL);
	if (RCL_RET_OK != ret) {
	throw RCLError("Failed to publish");
	}
	}

Add wait_for_all_acked support #793

Add wait_for_all_acked support #793

Conversation

Barry-Xu-2018 commented May 10, 2021

fujitatomoya May 12, 2021

Choose a reason for hiding this comment

Barry-Xu-2018 May 14, 2021

Choose a reason for hiding this comment

fujitatomoya May 12, 2021

Choose a reason for hiding this comment

Barry-Xu-2018 commented May 17, 2021

fujitatomoya commented May 18, 2021

sloretz left a comment

Choose a reason for hiding this comment

sloretz Aug 12, 2021

Choose a reason for hiding this comment

sloretz Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Barry-Xu-2018 commented Sep 2, 2021

Barry-Xu-2018 commented Sep 2, 2021

ivanpauno commented Sep 2, 2021

Barry-Xu-2018 commented Sep 7, 2021

ivanpauno commented Sep 13, 2021

fujitatomoya commented Sep 13, 2021

jacobperron Sep 15, 2021

Choose a reason for hiding this comment

sloretz Aug 12, 2021 •

edited

Loading