Single package.xml file for both ROS1 and ROS2 #48

Timple · 2021-07-12T09:11:12Z

Single package.xml file for both ROS1 and ROS2

This saves the trouble of moving around files.

Basically this is a combination of the package_ros1.xml and the package_ros2.xml since they are largely identical.
The deviating parts have a conditional guard as supported by <package format="3"> since ros kinetic.

Timple · 2021-10-21T07:58:53Z

Any update on this? It's blocking #33

Timple · 2021-11-29T09:30:35Z

If you need me to re-target this to the dev branch, please let me know.

baoxianzhang · 2021-12-03T02:20:28Z

ROS1 and ROS2 xml are different in build_depend and build_tool_depend.

Timple · 2021-12-03T05:07:21Z

Yes they are. That is why I put conditional guards around them in this PR

baoxianzhang · 2021-12-03T13:10:43Z

I got it. Good job. Test on ROS1 is OK. I will continue to test on ROS2.

Timple · 2021-12-06T09:51:25Z

Great, let me know the outcome!

Timple · 2021-12-20T07:10:17Z

Subtle ping, I felt like this was getting a bit of traction 🙂

baoxianzhang · 2022-01-03T06:30:28Z

Test on ROS2 is OK. We will update soon.

ronzheng1006 · 2022-01-10T06:34:21Z

Hi Timple,

It is a cool solution.

However, some of other clients wish not to mixture ros1's package.xml and ros2's, so we have to keep two separate config files.

Thanks.

Timple · 2022-01-10T07:08:05Z

That is a bummer. There are guards in the CMakeLists.txt, guards in the code already. So interesting that a guard in the package.xml is too much.

But good that you took a look at the existing customer base before merging!

ronzheng1006 · 2022-01-20T08:47:38Z

Hi Timple,

Some clients are still working on Ubuntu 16.04/ROS Kinetic, which doesn't know depend unfortunately.
We'll merge this PR after saying bye to Kinetic.

Timple · 2022-01-20T09:30:01Z

It should support depend actually. It was introduced for Kinetic here: ros/catkin#1025

But nonetheless, kinetic is EOL: https://dlu.github.io/ros_clock/index.html
Do you have a time-estimate on when you plan to say bye to Kinetic? Because until that time we (and lots of other developers I assume?) need to patch this repository on every deployment.

One could easily tag the current master branch with a kinetic tag. All clients using legacy software can keep using that tag indefinitely (or point them to the v1.3.0 tag).

Timple · 2022-02-11T15:30:41Z

I rebased the feature against dev. Seems all development efforts are going there.

JWhitleyWork · 2022-12-06T02:53:51Z

ROS Kinetic was EoL in April of 2021. Can we please get this merged?

Timple · 2022-12-13T07:25:31Z

These lidars are decent hardware. However software was unable to run at 10hz in ROS2, can happen.

However: no response to build fixes, no response to PR's improving code performance in ROS2, no response to this PR which improves ROS workflow.
We are evaluating other lidar manufacturers.

I will keep this PR open for those still interested in RS lidars.

JWhitleyWork · 2022-12-13T14:49:22Z

@Timple I am beginning to agree with your assessment. It's unfortunate because some of thier hardware is pretty decent.

fix(fake_perception): check for existence of map frame

rafa-martin · 2023-11-21T20:18:09Z

Is there any update for this PR?. I would be nice use original repository instead of fork

jjannik · 2024-01-03T09:09:00Z

Is there any update?

alexg-k · 2024-05-29T15:33:45Z

ROS1 for the default package configuration becomes more and more outdated. Please merge the efforts of the community into your products. We have just begun buying your products for evaluation purposes and software support is one part.

rafa-martin

Tested in noetic

Timple · 2024-07-26T06:29:15Z

So we actually did find an alternative for the rslidar lidars.

However I'm reviving an old robot and discovered this PR is still not merged...

I retargeted the PR to main as now this seems to be where development is going on.

rafa-martin · 2024-08-23T13:05:09Z

@Timple it seems that this branch coudn't be merged

@FelixHuang18 could you check it??

Timple · 2024-08-23T13:32:33Z

@rafa-martin do you have any intention of merging if the conflicts are resolved?

rafa-martin · 2024-08-23T14:32:58Z

Unfortunately, I'm not the owner of this repository. :(

Timple · 2024-08-23T14:45:46Z

Then I'm going to leave it as is...
It's functional as far as I know.

It's been three years without a response...

FelixHuang18 · 2024-08-26T09:32:25Z

@Timple
Hi Timple,

Thank you so much for your pull request and your contribution to the project! I want to sincerely apologize for the delay in addressing it.

Unfortunately, due to some recent changes, there are now conflicts that prevent me from merging it directly. Could I kindly ask you to resolve the conflicts and push the updated changes?

Once the conflicts are resolved, I will make it a priority to review, verify, and merge your PR as soon as possible.

I appreciate your understanding and patience, and again, thank you for your contribution!

Best regards,
Felix Huang

Timple · 2024-08-26T09:52:57Z

Here you go!

I do not speak, nor write Chinese. So any help there is appreciated.

Timple · 2024-08-26T09:54:41Z

node/rslidar_sdk_node.cpp

@@ -88,6 +88,15 @@ int main(int argc, char** argv)

   config_path += "/config/config.yaml";

+// Pick up --config-path argument from command line


If you can provide an example on how to provide this via ROS2 parameters, this snippet can probably be gone.

But this does not work at the moment:

<?xml version="1.0" encoding="UTF-8"?> <launch> <node pkg="rslidar_sdk" exec="rslidar_sdk_node" output="screen" > <param name="config_path" value="/path/to/rslidar.yaml" /> </node> </launch>

Is the function you want to implement is to load the configuration path from ROS2 parameters?

Yes
But the rslidar_sdk seems to consist of multiple nodes. So I am unsure how to pass a parameter to this.

I modified the start.py file to pass the config_path parameter.
`from launch import LaunchDescription
from launch_ros.actions import Node
from ament_index_python.packages import get_package_share_directory
def generate_launch_description():

rviz_config=get_package_share_directory('rslidar_sdk')+'/rviz/rviz2.rviz' config_file = '/home/felix/felix/ws/github/test/src/rslidar_sdk/config/config_ros2.yaml' return LaunchDescription([ Node(namespace='rslidar_sdk', package='rslidar_sdk', executable='rslidar_sdk_node', output='screen', parameters=[{'config_path': config_file}]), Node(namespace='rviz2', package='rviz2', executable='rviz2', arguments=['-d',rviz_config]) ])`

Timple · 2024-08-26T12:57:25Z

Ah, that does work. Not sure where my confusion came from.

There are mutiple nodes however:

$ ros2 node list
/param_handle
/rslidar_points_destination_0

But that's out of scope for this PR I guess.

Timple · 2024-08-26T12:57:58Z

Would you like to drop the manual argument parsing? Or would it still be convenient to do this?

FelixHuang18 · 2024-08-26T13:03:38Z

Yes, I don't think this code is needed， can you delete it?

rafa-martin · 2024-08-26T17:51:59Z

😄

FelixHuang18 · 2024-08-27T01:56:17Z

CMakeLists.txt

@@ -63,12 +58,7 @@ if (CMAKE_BUILD_TYPE STREQUAL "")
  add_definitions(-O3)
 endif()

-if($ENV{ROS_DISTRO} STREQUAL "humble")  # the ros2 humble requires c++17


In addition, why is the restriction of ros version removed here?

Im not sure, but as this software is C++17 compatible, @Timple set it c++17 as default.

For new ROS2 distros you would need to add it here (iron, jazzy and rolling) if new distros are released.

Also, as you are using cmake version > 3.1. Is better define these variables:
set (CMAKE_CXX_STANDARD 17)
https://cmake.org/cmake/help/latest/variable/CMAKE_CXX_STANDARD.html

In addition, why is the restriction of ros version removed here?

It's not removed, it is still implicitly there. But the statement overlooked iron, jazzy and rolling. Currently humble is the oldest supported distro at the moment: ros_clock. So all can use c++17

Timple force-pushed the ros12 branch from 39bb755 to 8cbf420 Compare July 12, 2021 09:56

Timple mentioned this pull request Jul 23, 2021

Release as package into ROS #33

Open

Timple changed the base branch from release to dev February 11, 2022 15:20

Timple added 4 commits February 11, 2022 16:29

Single package.xml file for both ROS1 and ROS2

bbcad31

Adhere to rosdep definitions

6c6e27a

pcl_ros is also a dependency

9cc5af0

pcl dependencies only for ROS1

abcb799

Timple force-pushed the ros12 branch from 8cbf420 to abcb799 Compare February 11, 2022 15:29

Timple added 3 commits February 11, 2022 16:47

Fix rebase

21e10d0

Merge branch 'dev' into ros12

f046c54

No need to set COMPILE_METHOD

a373e15

Timple mentioned this pull request Mar 18, 2022

Update CMakeLists.txt #84

Closed

Update steps in README

e4e5095

robertkurvits pushed a commit to DUT-Racing/rslidar_sdk that referenced this pull request Feb 14, 2023

Merge pull request RoboSense-LiDAR#48 from DUT-Racing/fix/fake_perc-map

b6e2030

fix(fake_perception): check for existence of map frame

Timple mentioned this pull request Sep 26, 2023

Insert ros2 dependencies into package.xml #132

Closed

Timple mentioned this pull request Jan 5, 2024

Delete build.sh & merge ROS1/2 into one package Livox-SDK/livox_ros_driver2#46

Open

rafa-martin approved these changes May 29, 2024

View reviewed changes

Timple added 2 commits July 25, 2024 15:03

roslib only for ROS1

e058d25

Merge remote-tracking branch 'robosense/main' into ros12

7bdf9f8

Timple changed the base branch from dev to main July 26, 2024 06:27

Timple added 2 commits July 26, 2024 09:53

Restore libpcap as it's header is included

f22e467

Allow --config-path as argument for now as ROS2 nodes are inconsistent

d3f8cad

Merge branch 'main' into ros12

2ed132e

Timple commented Aug 26, 2024

View reviewed changes

Drop manual argument parsing

7198d6a

FelixHuang18 reviewed Aug 27, 2024

View reviewed changes

FelixHuang18 merged commit 9ed16d9 into RoboSense-LiDAR:main Aug 27, 2024

		@@ -88,6 +88,15 @@ int main(int argc, char** argv)

		config_path += "/config/config.yaml";

		// Pick up --config-path argument from command line

Single package.xml file for both ROS1 and ROS2 #48

Single package.xml file for both ROS1 and ROS2 #48

Conversation

Timple commented Jul 12, 2021 • edited Loading

Timple commented Oct 21, 2021

Timple commented Nov 29, 2021

baoxianzhang commented Dec 3, 2021

Timple commented Dec 3, 2021

baoxianzhang commented Dec 3, 2021

Timple commented Dec 6, 2021

Timple commented Dec 20, 2021

baoxianzhang commented Jan 3, 2022

ronzheng1006 commented Jan 10, 2022

Timple commented Jan 10, 2022

ronzheng1006 commented Jan 20, 2022

Timple commented Jan 20, 2022 • edited Loading

Timple commented Feb 11, 2022

JWhitleyWork commented Dec 6, 2022

Timple commented Dec 13, 2022

JWhitleyWork commented Dec 13, 2022

rafa-martin commented Nov 21, 2023

jjannik commented Jan 3, 2024

alexg-k commented May 29, 2024

rafa-martin left a comment

Choose a reason for hiding this comment

Timple commented Jul 26, 2024

rafa-martin commented Aug 23, 2024

Timple commented Aug 23, 2024

rafa-martin commented Aug 23, 2024

Timple commented Aug 23, 2024

FelixHuang18 commented Aug 26, 2024

Timple commented Aug 26, 2024

Timple Aug 26, 2024

Choose a reason for hiding this comment

FelixHuang18 Aug 26, 2024

Choose a reason for hiding this comment

Timple Aug 26, 2024

Choose a reason for hiding this comment

FelixHuang18 Aug 26, 2024

Choose a reason for hiding this comment

Timple commented Aug 26, 2024

Timple commented Aug 26, 2024

FelixHuang18 commented Aug 26, 2024 • edited Loading

rafa-martin commented Aug 26, 2024

FelixHuang18 Aug 27, 2024

Choose a reason for hiding this comment

rafa-martin Aug 27, 2024

Choose a reason for hiding this comment

Timple Aug 27, 2024

Choose a reason for hiding this comment

Timple commented Jul 12, 2021 •

edited

Loading

Timple commented Jan 20, 2022 •

edited

Loading

FelixHuang18 commented Aug 26, 2024 •

edited

Loading