Report sample lost events #686

gonzodepedro · 2021-05-11T21:20:18Z

Description

Add report sample lost events in RViz.
Users will be able to detect situations where messages were sent by a publisher, but never arrived.

Checklist

If you are addressing rendering issues, please provide:
- Images of both, broken and fixed renderings.
- Source code to reproduce the issue, e.g. a YAML or rosbag file with a MarkerArray msg.
If you are changing GUI, please include screenshots showing how things looked before and after.
Choose the proper target branch: latest release branch, for non-ABI-breaking changes, future release branch otherwise.
Due to the lack of active maintainers, we cannot provide support for older release branches anymore.
Did you change how RViz works? Added new functionality? Do not forget to update the tutorials and/or documentation on the ROS wiki
While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers of RViz. Refer to the RViz Wiki for reviewing guidelines.

jacobperron · 2021-05-18T16:53:42Z

Sorry, I almost forgot about this. I'll take a look today 👀

jacobperron

Looks okay to me, though I haven't been able to trigger a message lost event to see what the UI looks like... I tried modifying the QoS demos, but can't even make them report lost messages 😅 Any tips?

jacobperron · 2021-05-19T03:47:16Z

rviz_common/include/rviz_common/ros_topic_display.hpp

+      sub_opts.event_callbacks.message_lost_callback =
+        [&](rclcpp::QOSMessageLostInfo & info)
+        {
+          std::stringstream sstm;


We should #include <sstream> .

nit:

Suggested change

std::stringstream sstm;

std::ostringstream sstm;

jacobperron · 2021-05-19T03:49:41Z

rviz_common/include/rviz_common/ros_topic_display.hpp

@@ -206,11 +206,23 @@ class RosTopicDisplay : public _RosTopicDisplay

    try {
      // TODO(anhosi,wjwwood): replace with abstraction for subscriptions once available


nit: I would keep this comment directly above L220

jacobperron · 2021-05-19T04:20:27Z

rviz_common/include/rviz_common/ros_topic_display.hpp

@@ -206,11 +206,23 @@ class RosTopicDisplay : public _RosTopicDisplay

    try {
      // TODO(anhosi,wjwwood): replace with abstraction for subscriptions once available
+      rclcpp::SubscriptionOptions sub_opts;
+      sub_opts.event_callbacks.message_lost_callback =


Do we have to guard against RMWs that don't support sample lost events?

ivanpauno · 2021-05-19T14:48:39Z

I tried modifying the QoS demos, but can't even make them report lost messages

It was easier to get notifications with rmw_connext_cpp AFAIR.
I guess that still applies to rmw_connextdds

ivanpauno · 2021-05-19T14:49:29Z

rviz_common/include/rviz_common/ros_topic_display.hpp

+      sub_opts.event_callbacks.message_lost_callback =
+        [&](rclcpp::QOSMessageLostInfo & info)
+        {
+          std::stringstream sstm;


nit:

Suggested change

std::stringstream sstm;

std::ostringstream sstm;

rviz_common/include/rviz_common/ros_topic_display.hpp

jacobperron · 2021-08-05T21:58:16Z

LGTM, looks like Ivan's comments about using std::ostringstream instead of std::stringstream can still be applied though.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro · 2021-08-09T14:22:17Z

I think it is applied here:

rviz/rviz_common/include/rviz_common/ros_topic_display.hpp

Line 213 in bc9dc5b

std::ostringstream sstm;

Am I missing some other place where it should be applied?

jacobperron

I think all of these lines can be change to use std::ostringstream.

jacobperron · 2021-08-10T17:29:40Z

rviz_default_plugins/src/rviz_default_plugins/displays/camera/camera_display.cpp

+    sub_opts.event_callbacks.message_lost_callback =
+      [&](rclcpp::QOSMessageLostInfo & info)
+      {
+        std::stringstream sstm;


Suggested change

std::stringstream sstm;

std::ostringstream sstm;

jacobperron · 2021-08-10T17:30:07Z

rviz_default_plugins/src/rviz_default_plugins/displays/map/map_display.cpp

+    sub_opts.event_callbacks.message_lost_callback =
+      [&](rclcpp::QOSMessageLostInfo & info)
+      {
+        std::stringstream sstm;


Suggested change

std::stringstream sstm;

std::ostringstream sstm;

jacobperron · 2021-08-10T17:30:14Z

rviz_default_plugins/src/rviz_default_plugins/displays/marker/marker_display.cpp

+    sub_opts.event_callbacks.message_lost_callback =
+      [&](rclcpp::QOSMessageLostInfo & info)
+      {
+        std::stringstream sstm;


Suggested change

std::stringstream sstm;

std::ostringstream sstm;

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro · 2021-08-11T19:48:12Z

CI build: --packages-up-to rviz2

Linux
Linux-aarch64
macOS
Windows

gonzodepedro requested a review from jacobperron May 11, 2021 21:20

gonzodepedro self-assigned this May 11, 2021

jacobperron reviewed May 19, 2021

View reviewed changes

ivanpauno reviewed May 19, 2021

View reviewed changes

gonzodepedro force-pushed the gonzodepedro/report-sample-lost branch from 7f17d14 to bc9dc5b Compare July 21, 2021 19:49

gonzodepedro requested review from jacobperron and ivanpauno August 4, 2021 20:14

Gonzalo de Pedro added 4 commits August 9, 2021 11:10

Report messages lost

481b594

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Uncrustify

b9f420f

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

more uncrustify

d34fc18

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Changes based on review

4f577fd

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro force-pushed the gonzodepedro/report-sample-lost branch from bc9dc5b to 4f577fd Compare August 9, 2021 14:28

jacobperron reviewed Aug 10, 2021

View reviewed changes

Changed stringstream for ostringstream

e345c2a

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro requested a review from jacobperron August 12, 2021 21:04

jacobperron approved these changes Aug 12, 2021

View reviewed changes

gonzodepedro merged commit 60a7269 into ros2 Aug 13, 2021

jacobperron deleted the gonzodepedro/report-sample-lost branch August 25, 2021 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report sample lost events #686

Report sample lost events #686

gonzodepedro commented May 11, 2021

jacobperron commented May 18, 2021

jacobperron left a comment

jacobperron May 19, 2021

ivanpauno May 19, 2021

jacobperron May 19, 2021

jacobperron May 19, 2021

ivanpauno commented May 19, 2021

ivanpauno May 19, 2021

jacobperron commented Aug 5, 2021

gonzodepedro commented Aug 9, 2021

jacobperron left a comment

jacobperron Aug 10, 2021

jacobperron Aug 10, 2021

jacobperron Aug 10, 2021

gonzodepedro commented Aug 11, 2021

		@@ -206,11 +206,23 @@ class RosTopicDisplay : public _RosTopicDisplay

		try {
		// TODO(anhosi,wjwwood): replace with abstraction for subscriptions once available

Report sample lost events #686

Report sample lost events #686

Conversation

gonzodepedro commented May 11, 2021

Description

Checklist

jacobperron commented May 18, 2021

jacobperron 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

ivanpauno commented May 19, 2021

Choose a reason for hiding this comment

jacobperron commented Aug 5, 2021

gonzodepedro commented Aug 9, 2021

jacobperron 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

gonzodepedro commented Aug 11, 2021