-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add documentation for RealSense ROS2 Wrapper Windows installation #3056
Add documentation for RealSense ROS2 Wrapper Windows installation #3056
Conversation
eb640e6
to
1172637
Compare
@@ -115,6 +115,7 @@ find_package(nav_msgs REQUIRED) | |||
find_package(tf2_ros REQUIRED) | |||
find_package(tf2 REQUIRED) | |||
find_package(diagnostic_updater REQUIRED) | |||
find_package(OpenCV REQUIRED COMPONENTS core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cv-bridge is not enough?
Linux never complain, does it pop up in Win only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add only on WIN32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in Linux jammy/focal, opencv libs are installed by default. We need them to make some functions works, for example CV_ASSERT in our code.. this is the only usage I saw for it. Maybe if we remove this assert we can remove this dependency..
README.md
Outdated
@@ -161,6 +163,76 @@ | |||
|
|||
<hr> | |||
|
|||
# Installation on Windows | |||
**PLEASE PAY ATTENTION: RealSense ROS2 Wrapper is not meant to be supported on Windows by our team, since ROS2 and its packages are still not fully supported over Windows. We are putting these installation steps below in order to try and make it easier for users who already started working with ROS2 on Windows and want to take advantage of the capabilities of our RealSense cameras** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putting
--> adding
?
README.md
Outdated
**Please choose only one option from the two options below (in order to prevent multiple versions installation and workspace conflicts)** | ||
- Manual install from ROS2 formal documentation: | ||
- [ROS2 Iron](https://docs.ros.org/en/iron/Installation/Windows-Install-Binary.html) | ||
- [ROS2 Humble](https://docs.ros.org/en/humble/Installation/Windows-Install-Binary.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tab? looks a bit misaligned
README.md
Outdated
Step 2: Download RealSense™ ROS2 Wrapper and RealSense™ SDK 2.0 source code from github: | ||
</summary> | ||
|
||
- Download the latest [Intel® RealSense™ ROS2 Wrapper 4.54.1](https://github.com/IntelRealSense/realsense-ros/tree/4.54.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we point to a specific version if we instruct to take the latest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the latest released.. what you propose here ? to take the master from librealsense and ros2-development ? or what ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can point to the ros releases page: https://github.com/IntelRealSense/realsense-ros/releases
And tell them to also get the matching LibRS supported release from the releases page: https://github.com/IntelRealSense/librealsense/releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, please review last commit
README.md
Outdated
</summary> | ||
|
||
- Download the latest [Intel® RealSense™ ROS2 Wrapper 4.54.1](https://github.com/IntelRealSense/realsense-ros/tree/4.54.1) | ||
- Download the latest [Intel® RealSense™ SDK 2.0 2.54.1](https://github.com/IntelRealSense/librealsense/tree/v2.54.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
README.md
Outdated
|
||
- Download the latest [Intel® RealSense™ ROS2 Wrapper 4.54.1](https://github.com/IntelRealSense/realsense-ros/tree/4.54.1) | ||
- Download the latest [Intel® RealSense™ SDK 2.0 2.54.1](https://github.com/IntelRealSense/librealsense/tree/v2.54.1) | ||
- Put the librealsense folder inside the realsense-ros folder, to make the librealsense package set beside realsense2_camera, realsense2_camera_msgs and realsense2_description packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put
--> Place
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition, few small comments
Added win32 for opencv related lines, and fixed the readme |
realsense2_camera/CMakeLists.txt
Outdated
@@ -115,7 +115,10 @@ find_package(nav_msgs REQUIRED) | |||
find_package(tf2_ros REQUIRED) | |||
find_package(tf2 REQUIRED) | |||
find_package(diagnostic_updater REQUIRED) | |||
find_package(OpenCV REQUIRED COMPONENTS core) | |||
|
|||
if(WIN32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If OpenCV is indeed needed let's install it always.
Sorry for the confusion.
Let's remove all the new if(WIN32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Also fixing LRS-1041