Frame callbacks #160

mkhansenbot · 2016-12-01T23:58:36Z

Please ensure the above guidelines for contributing are met.

Fixes Issue: #

Changes proposed in this pull request:

Changes all streams to using callbacks instead of using rs_wait_for_frames
Adds needed mutex to imu callback for zr300
Refactors code to remove duplicated code
Removes the topic_thread since rs_wait_for_frames is removed and everything done via callbacks

mdhorn · 2016-12-14T21:35:07Z

realsense_camera/src/base_nodelet.cpp

+  void BaseNodelet::setStreams()
+  {
+    // Enable streams
+    for (int stream=0; stream < STREAM_COUNT; stream++)


Change the type of stream to be type 'rs_stream' and remove the casts.

Since STREAM_COUNT is an int, I can't change stream to an rs_stream type. Instead, I'm using static_cast<rs_stream>(stream) to pass the stream var.

Why not cast STREAM_COUNT once instead of stream twice?

Or can we change STREAM_COUNT to a rs_stream type?

No ++ operator is defined for the enum type rs_stream, so you get this compiler error:
error: no ‘operator++(int)’ declared for postfix ‘++’ [-fpermissive]
for (rs_stream stream; stream < (rs_stream) STREAM_COUNT; stream++)

We cant change STREAM_COUNT to an rs_stream type, since we are setting the value to 5 in constants.h. I would like us to actually remove the constant in a future patch and do this a different way, as I agree this is non-ideal. I would prefer a member variable num_streams_ that isn't hard-coded and varies depending on the camera type, set in the onInit() function for each type to the correct value for that camera. But that's another patch for another day.

mdhorn · 2016-12-14T21:41:49Z

realsense_camera/src/base_nodelet.cpp

-      image_depth16_ = reinterpret_cast<const uint16_t *>(rs_get_frame_data(rs_device_, stream_index, 0));
-      if (depth_scale_meters_ == MILLIMETER_METERS)
+      image_depth16_ = reinterpret_cast<const uint16_t *>(frame.get_data());
+      float depth_scale_meters = rs_get_device_depth_scale(rs_device_, &rs_error_);


Should this function be called in here? getStreamData is called the most frequently in the code.
Can we initialize this something else? Or make it a local static and initialize it once.

At a minimum we need to add a check error for the rs_error_

The value rs_get_device_depth_scale returns is device.config.depth_scale a member variable in the rs_device_ object. Do you still think it's an issue?

mdhorn · 2016-12-14T21:47:10Z

realsense_camera/src/base_nodelet.cpp


-        publishTopics();
+    if (rs_is_stream_enabled(rs_device_, (rs_stream) stream_index, 0) == 1)


Since we are called by the callback, we have to be enabled. We should remove this check.

mdhorn · 2016-12-14T21:50:32Z

realsense_camera/include/realsense_camera/base_nodelet.h

-    virtual void publishTopic(rs_stream stream_index);
-    virtual void getStreamData(rs_stream stream_index);
+    virtual void publishTopic(rs_stream stream_index, rs::frame &  frame);
+    virtual void getStreamData(rs_stream stream_index, rs::frame &  frame);


Rename to setImageData

mdhorn · 2016-12-14T21:50:45Z

realsense_camera/src/base_nodelet.cpp

@@ -724,81 +726,94 @@ namespace realsense_camera
  /*
   * Copy frame data from realsense to member cv images.
   */
-  void BaseNodelet::getStreamData(rs_stream stream_index)
+  void BaseNodelet::getStreamData(rs_stream stream_index, rs::frame & frame)


Rename to setImageData

mkhansenbot · 2016-12-15T22:35:35Z

I squashed the commits down to 6, this is ready for final review and approval.

mdhorn

Previous discussion items implemented.

LGTM 👍

reaganlo

Approved

mkhansenbot force-pushed the frame_callbacks branch 2 times, most recently from 1a9223e to 879eb59 Compare December 14, 2016 18:45

mdhorn reviewed Dec 14, 2016

View reviewed changes

mkhansenbot force-pushed the frame_callbacks branch 2 times, most recently from 8aa11da to a68ed0c Compare December 15, 2016 22:34

mdhorn approved these changes Dec 15, 2016

View reviewed changes

Matt Hansen added 6 commits December 15, 2016 14:49

Added frame callbacks instead of rs_wait_for_frames

40c856e

Added imu_mutex for data sync to callback

157182c

Removed prepareTopics thread; replaced with setDepthEnable

765e7db

Refactored setStreams

2773cc8

Changed depth_scale_meters to local var

bf562b3

Renamed getStreamData to setImageData

6112978

mkhansenbot force-pushed the frame_callbacks branch from a68ed0c to 6112978 Compare December 15, 2016 22:53

reaganlo approved these changes Dec 15, 2016

View reviewed changes

mkhansenbot merged commit 7340761 into IntelRealSense:indigo-devel Dec 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frame callbacks #160

Frame callbacks #160

mkhansenbot commented Dec 1, 2016 •

edited

Loading

mdhorn Dec 14, 2016

mkhansenbot Dec 15, 2016

mdhorn Dec 15, 2016

mdhorn Dec 15, 2016 •

edited

Loading

mkhansenbot Dec 15, 2016

mkhansenbot Dec 15, 2016

mdhorn Dec 14, 2016

mkhansenbot Dec 15, 2016

mdhorn Dec 14, 2016

mdhorn Dec 14, 2016

mdhorn Dec 14, 2016

mkhansenbot commented Dec 15, 2016

mdhorn left a comment

reaganlo left a comment


		publishTopics();
		if (rs_is_stream_enabled(rs_device_, (rs_stream) stream_index, 0) == 1)

Frame callbacks #160

Frame callbacks #160

Conversation

mkhansenbot commented Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhorn Dec 15, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhansenbot commented Dec 15, 2016

mdhorn left a comment

Choose a reason for hiding this comment

reaganlo left a comment

Choose a reason for hiding this comment

mkhansenbot commented Dec 1, 2016 •

edited

Loading

mdhorn Dec 15, 2016 •

edited

Loading