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

Delete build.sh & merge ROS1/2 into one package #46

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Ericsii
Copy link

@Ericsii Ericsii commented May 8, 2023

No description provided.

@Ericsii
Copy link
Author

Ericsii commented May 8, 2023

#26

@justinberi
Copy link

I've tested this on ROS noetic with catkin_make from the top level and believe this is the correct way to do building. Please follow the ROS conventions rather than using a build script as @Ericsii has done.

The PR required the following modification to package.xml

git diff package.xml
diff --git a/package.xml b/package.xml
index 19b5804..ff7cf81 100644
--- a/package.xml
+++ b/package.xml
@@ -11,9 +11,9 @@
 
     <!-- ROS1 dependencies -->
     <buildtool_depend condition="$ROS_VERSION == 1">catkin</buildtool_depend>
-    <build_depend condition="$ROS_VERSION == 1">roscpp</build_depend>
-    <build_depend condition="$ROS_VERSION == 1">rospy</build_depend>
-    <build_depend condition="$ROS_VERSION == 1">message_generation</build_depend>
+    <depend condition="$ROS_VERSION == 1">roscpp</depend>
+    <depend condition="$ROS_VERSION == 1">rospy</depend>
+    <depend condition="$ROS_VERSION == 1">message_generation</depend>
     <exec_depend condition="$ROS_VERSION == 1">message_runtime</exec_depend>
     <exec_depend condition="$ROS_VERSION == 1">rosbag</exec_depend>

@justinberi
Copy link

justinberi commented Jul 12, 2023

I've also tested under ROS2 humble with colcon build from the workspace dir and it works.

Copy link
Author

@Ericsii Ericsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update package.xml

package.xml Outdated Show resolved Hide resolved
package.xml Outdated Show resolved Hide resolved
package.xml Outdated Show resolved Hide resolved
mourams pushed a commit to mourams/livox_ros_driver2 that referenced this pull request Aug 2, 2023
package.xml Outdated Show resolved Hide resolved
@Timple
Copy link

Timple commented Jan 5, 2024

Awesome!

Although to be complete you might also update the instructions in the README.md.

Funny how these lidar manufacturers handle ignore these PRs: RoboSense-LiDAR/rslidar_sdk#48
They seem to think the whole software stack/build system revolves around the sensor driver.

@MCFurry
Copy link

MCFurry commented Jan 26, 2024

Thx for this!
One question though, ROS2 releases older than Humble are deprecated now: https://dlu.github.io/ros_clock/index.html
Hence, can we ditch the if-guard here?
https://github.com/Ericsii/livox_ros_driver2/blob/6f1a61e785452170a195887400f1d2950c5abfb8/CMakeLists.txt#L285
Which checks for Humble only, but in principle should also be used on Iron and Rolling as well.

@Ericsii
Copy link
Author

Ericsii commented Jan 27, 2024

Hence, can we ditch the if-guard here? https://github.com/Ericsii/livox_ros_driver2/blob/6f1a61e785452170a195887400f1d2950c5abfb8/CMakeLists.txt#L285 Which checks for Humble only, but in principle should also be used on Iron and Rolling as well.

Sure, I think this could be removed. But I need some tests on Iron and Rolling

@Timple
Copy link

Timple commented Feb 19, 2024

Verified on iron 🙂

@Ericsii
Copy link
Author

Ericsii commented Feb 23, 2024

@MCFurry I removed the condition for humble. It might work in newer versions of ros2

@MCFurry
Copy link

MCFurry commented Feb 23, 2024

We noticed! Thanks for that! Seems to work fine for Iron now as well.

Although we're also using https://github.com/pixmoving-auto/Livox-SDK2.git on update/robobus branch for the time being, since then both packages are ROS-packages and can now be easily automatically installed.

@Timple
Copy link

Timple commented Feb 23, 2024

That being said, that implementation can slightly be improved by keeping it a pure cmake package and only add a package.xml file 🙂

Copy link

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, build successfully tested on humble + rolling

@Ericsii Ericsii mentioned this pull request Feb 26, 2024
@Timple Timple mentioned this pull request Jul 1, 2024
TonyCooT added a commit to Polytech-VM-Team/livox_ros_driver2 that referenced this pull request Jul 9, 2024
- PR "Delete build.sh & merge ROS1/2 into one package"
Livox-SDK#46
- PR "Remove unnecesarry linking of boost"
Livox-SDK#51
- PR "<fix> arg frame_id for imu message"
Livox-SDK#100
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.

6 participants