Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR for composition implementation and message package name change #1415

Merged
merged 10 commits into from
Oct 8, 2020
Merged

PR for composition implementation and message package name change #1415

merged 10 commits into from
Oct 8, 2020

Conversation

JaehyunShim
Copy link

@JaehyunShim JaehyunShim commented Sep 23, 2020

@RealSenseSupport @doronhi

As I mentioned in this issue thread, I did the followings.

  1. Merged realsense2_node to realsense2_camera and implemented it as Composition. I remember Steve Macenski from Samsung Research also mentioned it in another thread.
  2. Changed message package name to realsense2_camera_msgs for the consistency purpose.

As I mentioned in the issue thread, please redirect this PR to your new branch, desirably 'ros2' branch, and please keep your original code in the current 'eloquent' branch (i suggest you change the name to 'eloquent-devel') since you already binary-released it this month.

Let me know if you have any comments on it,
Ryan

@JaehyunShim
Copy link
Author

@JaehyunShim
Copy link
Author

  1. I did some more work on the description package. I only added a launch file for the D435 model so please add the rest of the files for other models too.

@JaehyunShim
Copy link
Author

ping @doronhi

@JaehyunShim
Copy link
Author

ping @RealSenseSupport

@doronhi
Copy link
Contributor

doronhi commented Oct 7, 2020

Sorry for so far failing to check your PR. It includes a couple of topics and I won't be able to merge it as a whole.
Unaware of your work I also submitted #1429 for the description package so I'll have to sort all those changes out.
It may take a couple of days. No more, I hope..

@JaehyunShim
Copy link
Author

@doronhi

Thank you for your reply. Let me know if you find anything unclear while working on it.

Jaehyun

@doronhi doronhi merged commit 043c0f3 into IntelRealSense:eloquent Oct 8, 2020
@doronhi
Copy link
Contributor

doronhi commented Oct 8, 2020

Regarding the realsense2_description, if I understand correctly, it is not working yet. In the file view_d435_model.launch.py The keyword "executable" needs to be replaced with "node_executable" and there is no parsing of the urdf.xacro file. I'll skip the description package as I already have #1429 . If you manage to find the time to make some more PRs in the future it would help a lot if it separated into several PRs, each with a single scheme.
Regarding the conversion to Composition - Thanks a lot. I really appreciate these ROS2 specifics.

@JaehyunShim
Copy link
Author

@doronhi

Thank you. OK I will keep that in mind next time. Have a great day.

Jaehyun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants