Livox-SDK / livox_ros_driver2

Livox device driver under Ros(Compatible with ros and ros2), support Lidar HAP and Mid-360.
Other
201 stars 161 forks source link

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

Open Ericsii opened 1 year ago

Ericsii commented 1 year ago

26

justinberi commented 12 months ago

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 commented 12 months ago

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

Timple commented 6 months ago

Awesome!

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

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

MCFurry commented 5 months ago

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

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 commented 4 months ago

Verified on iron :slightly_smiling_face:

Ericsii commented 4 months ago

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

MCFurry commented 4 months ago

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 commented 4 months ago

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