Por upstream sync v2.2.3

abhijitmajumdar commented 5 years ago

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.

abhijitmajumdar commented 5 years ago

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

130s commented 5 years ago

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
$

abhijitmajumdar commented 5 years ago

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 commented 5 years ago

@abhijitmajumdar The MR looks good enough to eat.

joshuaplusone commented 5 years ago

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

abhijitmajumdar commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Is it possible to only publish ir and depth?

joshuaplusone commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

https://github.com/plusone-robotics/realsense/pull/6 https://github.com/plusone-robotics/realsense/pull/7

130s commented 5 years ago

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

Due to https://github.com/plusone-robotics/realsense/issues/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'

plusone-robotics / realsense

Por upstream sync v2.2.3 #23