Por upstream sync v2.2.3 #23

abhijitmajumdar · 2019-04-05T21:49:44Z

This PR is to update sync with upstream for v2.2.3. The key features required were form the commits f002a7b and eea5b15 to add the fix to load default params from ros param server on startup, and the addition of hole filling filter as a separate filter.

- restore T265 camera launch instructions - add launch instructions for T265 RViz demo

… This unit adjustment is needed for rgbd_launch package and it's point cloud value.

* fix depth scale to always follow ROS convention of 1mm * incorporates PR#605

* add example for checking the depth at the center of the image. * fix bug: did not fix depth scale for single frame.

thanks.

@efernandez

@efernandez , thanks for noticing. @stwirth thanks for the explanation.

fixes IntelRealSense#609 Only if the ROS params are set

with equivalent to ros::names::isValidCharInName(char c)

by finding the ROS (static) param in the enum dictionary, which can take values in the min:step:max range (not just 0 <= value < enum_dict.size()). Also remove the check when the option is taken from the sensor, which should always be correct.

so we don't loop with float values.

…ment

* use wheel_Odometry Add parameters to launch files: * topic_odom_in - The topic on which wheel odometry arrives. * calib_odom_file - path to calibration.json file, of the librealsense format. i.e.: https://github.com/IntelRealSense/librealsense/blob/master/unit-tests/resources/calibration_odometry.json

… schmidtp1-patch-2

update README.md link to librealsense v2.19.2

update version to 2.2.3

…and eea5b15

abhijitmajumdar · 2019-04-05T21:51:43Z

Testing:
Build: Success
Camera IR RGB: Success
Product integration: Success

130s · 2019-04-08T08:38:26Z

Looks good. I can only check whether the sync is cleanly done or not, and I confirmed it is done fine as follows.

Took a diff of commits in 2 branches, between 2.2.3 and abhijitmajumdar/por_upstream_sync. While there are 7 commits that only exist in your brnach, looks like the real change is the first 2 commits (the ones at the bottom of the list below).

$ git log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit --date=relative 2.2.3..abhijitmajumdar/por_upstream_sync
* 3e326df - (HEAD -> abhijit_por_upstream_sync, abhijitmajumdar/por_upstream_sync) merging upstream changes from v2.2.3 to add important feature f002a7b and eea5b15 (2 days ago)
* 139478b - (tag: 100.1.0, origin/por_upstream_sync, por_upstream_sync) 100.1.0 (4 days ago)
* 78ef4d6 - POR: sync pkg version in a repo so that catkin_prepare_release can run. (4 days ago)
* da86f9d - Update changelog for 100.1.0, Plus One Robotics forked version. (4 days ago)
* 274beb1 - Merge pull request #21 from abhijitmajumdar/por_upstream_sync (4 days ago)
* 9cab3ec - removed debug (not used) code (5 days ago)
* 34b75c8 - (abhijitmajumdar/infrargb_2.2.1) working irrgb stream (2 weeks ago)

There's no new commit in 2.2.3.

$ git log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit --date=relative abhijitmajumdar/por_upstream_sync..2.2.3
$

…tream

abhijitmajumdar · 2019-04-08T17:21:40Z

Looks good. I can only check whether the sync is cleanly done or not, and I confirmed it is done fine as follows.

Took a diff of commits in 2 branches, between 2.2.3 and abhijitmajumdar/por_upstream_sync. While there are 7 commits that only exist in your brnach, looks like the real change is the first 2 commits (the ones at the bottom of the list below).

$ git log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit --date=relative 2.2.3..abhijitmajumdar/por_upstream_sync
* 3e326df - (HEAD -> abhijit_por_upstream_sync, abhijitmajumdar/por_upstream_sync) merging upstream changes from v2.2.3 to add important feature f002a7b and eea5b15 (2 days ago)
* 139478b - (tag: 100.1.0, origin/por_upstream_sync, por_upstream_sync) 100.1.0 (4 days ago)
* 78ef4d6 - POR: sync pkg version in a repo so that catkin_prepare_release can run. (4 days ago)
* da86f9d - Update changelog for 100.1.0, Plus One Robotics forked version. (4 days ago)
* 274beb1 - Merge pull request #21 from abhijitmajumdar/por_upstream_sync (4 days ago)
* 9cab3ec - removed debug (not used) code (5 days ago)
* 34b75c8 - (abhijitmajumdar/infrargb_2.2.1) working irrgb stream (2 weeks ago)

There's no new commit in 2.2.3.

$ git log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit --date=relative abhijitmajumdar/por_upstream_sync..2.2.3
$

So I did end up making a new change, it does not affect the functionality, just modifies some code to indicate a cleaner logic implementation

joshuaplusone · 2019-04-09T01:23:37Z

realsense2_camera/src/base_realsense_node.cpp

    for (int i = 0; i < 5; i++)
    {
-        _camera_info[stream_index].D.push_back(intrinsic.coeffs[i]);
+        _camera_info[stream_index].D.at(i) = intrinsic.coeffs[i];


nice optimization

lol, not mine though. That was pulled from the upstream

joshuaplusone · 2019-04-09T01:33:26Z

@abhijitmajumdar The MR looks good enough to eat.

joshuaplusone · 2019-04-09T01:33:33Z

@abhijitmajumdar So, I couldn't find the launch file params for publishing an infrared image with color correction.

abhijitmajumdar · 2019-04-09T01:58:45Z

So, I couldn't find the launch file params for publishing an infrared image with color correction.

@joshuaplusone ~~Do you still have access to Gitlab? I could point you to the launch file. Or should I just attach a file here?~~

So there is no param that publishes the IR with Color yet, as of now, the code itself defaults to use RGB for the IR and the configuration I use to run the realsense is:

roslaunch realsense2_camera rs_rgbd.launch enable_fisheye:=false enable_infra2:=false enable_gyro:=false enable_accel:=false filters:=colorizer,disparity,spatial,decimation,pointcloud,hole_filling enable_pointcloud:=true

The IR with RGB gets published as /camera/infra1/image_rect_raw

joshuaplusone · 2019-04-09T02:11:42Z

Thanks for the heads up. So are there plans to add that back in for another PR. There used to be a synchronization bug that would cause the rgb / infrared results to skip frames. For noether we bypassed that by just using the depth frame without any synchronization.

abhijitmajumdar · 2019-04-09T02:22:40Z

So maybe I explained wrong. The IR with color gets published with this PR, there is currently no option to revert that back(or a switch for that matter) right now. So the IR defaults to a color stream. There are plans to add this switch in a future PR. The RGB support for the IR stream was included in the librealsense driver itself. The edits I made in this code are only to enable that in the ROS wrapper.

However I do not know anything about the bug which caused the frames to skip. When run using this code, and with the configuration I mentioned above, I am able to reliably get 30Hz on each stream. I did also configure the frames to go upto 60Hz, but I could subscribe to around 45Hz (which is I think expected given the bandwidth of 3 streams - IR, Color and Depth comming through)

Am I missing something?

joshuaplusone · 2019-04-09T15:14:54Z

Is it possible to only publish ir and depth?

joshuaplusone · 2019-04-09T15:16:18Z

This MR is fine in my book. My only concerns are based on whether or not intel has fixed their synchronization issues. If they haven't then we will need to have a way to only publish ir image and depth pointcloud.

abhijitmajumdar · 2019-04-09T15:22:59Z

Is it possible to only publish ir and depth?

Yes, except the colorizer would have to be disabled or switched to use another color stream. Also the /depth/color/points would not publish or they'd have to be configured to use another stream which can be done using an argument pointcloud_texture_stream:=RS2_STREAM_INFRARED

This MR is fine in my book. My only concerns are based on whether or not intel has fixed their synchronization issues. If they haven't then we will need to have a way to only publish ir image and depth pointcloud.

Can you explain what the synchronization bug was? Maybe I could answer your question

joshuaplusone · 2019-04-09T15:44:18Z

there are two pull requests that I believe for using infrargb without color.

#6
#7

130s · 2019-04-09T20:10:05Z

Including offline conversation reviewers think this is good to be merged.

Due to [por_upstream_sync branch] Tests failing #22, some tests on CI are expected to fail. Just notticed by looking at the latest result one failing test (points_cloud_1) somehow passed on this PR.

-----------------------------------
test name                      score : message                                                       
----------------------------  ------   -------------------------------------------------------------
depth_w_cloud_1                   OK : 
points_cloud_1                    OK : 
depth_avg_decimation_1            OK : 
non_existent_file                 OK : 'num_channels'
align_depth_color_1               OK : 
align_depth_ir1_1             FAILED : 'num_channels'
vis_avg_2                         OK : 
depth_avg_1                       OK : 
static_tf_1                   FAILED : Tf is None for couple camera_infra1_frame->camera_color_frame
depth_decimation_1            FAILED : 'num_channels'
align_depth_ir1_decimation_1  FAILED : 'num_channels'

schmidtp1 and others added 24 commits March 12, 2019 10:51

Update README.md

825d209

- restore T265 camera launch instructions - add launch instructions for T265 RViz demo

Adjust unit of SR300's depth image into 0.001 meter (same as D435's).…

35e86ce

… This unit adjustment is needed for rgbd_launch package and it's point cloud value.

fix depth scale (IntelRealSense#682)

2fd5438

* fix depth scale to always follow ROS convention of 1mm * incorporates PR#605

fix PR#682 (IntelRealSense#683)

1afae55

* add example for checking the depth at the center of the image. * fix bug: did not fix depth scale for single frame.

fix Twist in odometry problem (IntelRealSense#676)

19a0c08

Add dependency on nav_msgs (IntelRealSense#674)

8c65cc9

thanks.

Export dynamic_reconfigure to dependent packages (IntelRealSense#673)

22c2c7b

@efernandez , thanks for noticing. @stwirth thanks for the explanation.

Fixed distortion coefficients bug. (IntelRealSense#662)

b7ae88f

Set config defaults from ROS param server

f002a7b

fixes IntelRealSense#609 Only if the ROS params are set

Use std::replace_if in create_graph_resource_name

e04d568

with equivalent to ros::names::isValidCharInName(char c)

Transform to lower in create_graph_resource_name

9d5f45e

Fix validation check for enum options

6a22fba

by finding the ROS (static) param in the enum dictionary, which can take values in the min:step:max range (not just 0 <= value < enum_dict.size()). Also remove the check when the option is taken from the sensor, which should always be correct.

Cast min, step, step to int in get_enum_method

923869e

so we don't loop with float values.

Merge branch 'efernandez-set_defaults_from_param_server' into develop…

d3d8768

…ment

fix compilation bug with OMP (IntelRealSense#692)

b59aad5

add flag publish_odom_tf (default to true)

51494b6

Merge branch 'disable_odom_tf' into development

6bb8383

Merge branch 'patch-2' of https://github.com/schmidtp1/realsense into…

e6c56af

… schmidtp1-patch-2

Merge branch 'schmidtp1-patch-2' into development

fd8bdf3

update version to 2.2.2

5ff2cd3

update README.md link to librealsense v2.19.2

update rqt_reconfigure image in README.md

de6e825

Add hole_filling filter.

eea5b15

update version to 2.2.3

merging upstream changes from v2.2.3 to add important feature f002a7b …

3e326df

…and eea5b15

modifed the logic for adding a stream profile, to enable RGB for IR s…

6a54825

…tream

joshuaplusone reviewed Apr 9, 2019

View reviewed changes

130s requested a review from joshuaplusone April 9, 2019 14:48

130s merged commit 432cf23 into plusone-robotics:por_upstream_sync Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Por upstream sync v2.2.3 #23

Por upstream sync v2.2.3 #23

abhijitmajumdar commented Apr 5, 2019

abhijitmajumdar commented Apr 5, 2019 •

edited

Loading

130s commented Apr 8, 2019

abhijitmajumdar commented Apr 8, 2019

joshuaplusone Apr 9, 2019

abhijitmajumdar Apr 9, 2019

joshuaplusone commented Apr 9, 2019

joshuaplusone commented Apr 9, 2019

abhijitmajumdar commented Apr 9, 2019 •

edited

Loading

joshuaplusone commented Apr 9, 2019

abhijitmajumdar commented Apr 9, 2019

joshuaplusone commented Apr 9, 2019

joshuaplusone commented Apr 9, 2019

abhijitmajumdar commented Apr 9, 2019 •

edited

Loading

joshuaplusone commented Apr 9, 2019

130s commented Apr 9, 2019

Por upstream sync v2.2.3 #23

Por upstream sync v2.2.3 #23

Conversation

abhijitmajumdar commented Apr 5, 2019

abhijitmajumdar commented Apr 5, 2019 • edited Loading

130s commented Apr 8, 2019

abhijitmajumdar commented Apr 8, 2019

joshuaplusone Apr 9, 2019

Choose a reason for hiding this comment

abhijitmajumdar Apr 9, 2019

Choose a reason for hiding this comment

joshuaplusone commented Apr 9, 2019

joshuaplusone commented Apr 9, 2019

abhijitmajumdar commented Apr 9, 2019 • edited Loading

joshuaplusone commented Apr 9, 2019

abhijitmajumdar commented Apr 9, 2019

joshuaplusone commented Apr 9, 2019

joshuaplusone commented Apr 9, 2019

abhijitmajumdar commented Apr 9, 2019 • edited Loading

joshuaplusone commented Apr 9, 2019

130s commented Apr 9, 2019

abhijitmajumdar commented Apr 5, 2019 •

edited

Loading

abhijitmajumdar commented Apr 9, 2019 •

edited

Loading

abhijitmajumdar commented Apr 9, 2019 •

edited

Loading