search

PepperlFuchs / pf_lidar_ros_driver

ROS driver for Pepperl+Fuchs R2000 and R2300 laser scanners
https://www.pepperl-fuchs.com/global/en/23097.htm
Apache License 2.0
39 stars 38 forks source link

[WIP] Porting ros2 #83

Closed hsd-dev closed 1 year ago

hsd-dev commented 2 years ago

Originally from https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/82 started by @wawanbreton

wawanbreton commented 2 years ago

I got permission denied when pushing to this branch, can you give me some rights ?

hsd-dev commented 2 years ago

The current code seems to be valid only for foxy. We might need to have separate branch for galactic and upwards https://github.com/PepperlFuchs/pf_lidar_ros_driver/runs/6810174179?check_suite_focus=true#step:4:685

For now, I will disable galactic and upwards

wawanbreton commented 2 years ago

The current code seems to be valid only for foxy. We might need to have separate branch for galactic and upwards

What makes you say that ? I'm working with galaxy only

hsd-dev commented 2 years ago

@wawanbreton please check the CI output linked above. Didn't you get that error?

Edit: the error exists for foxy as well actually https://github.com/PepperlFuchs/pf_lidar_ros_driver/runs/6810315345?check_suite_focus=true

wawanbreton commented 2 years ago

No I didn't get the error, pcl-conversion is actually available for galactic

hsd-dev commented 2 years ago

right, we have commented out the deps https://github.com/PepperlFuchs/pf_lidar_ros_driver/blob/porting-ros2/pf_driver/package.xml#L13. Since CI starts with a clean docker, these packages have not been installed.

hsd-dev commented 2 years ago

OK the CI seems to be working now: https://github.com/PepperlFuchs/pf_lidar_ros_driver/runs/6811233956?check_suite_focus=true#step:4:1582

wawanbreton commented 2 years ago

My commit containing the DAE files instead of the STL has been wiped out, was it intentional ?

hsd-dev commented 2 years ago

sorry about that. I think I pushed commits around the same time as you. Could you fetch the latest commits, rebase and push that commit again?

hsd-dev commented 2 years ago

Also, take a look at the CI output linked above

/root/target_ws/src/pf_lidar_ros_driver/pf_driver/src/pf/pf_interface.cpp:220:25: error: missing template arguments before ‘feed_time’
    220 |   std::chrono::duration feed_time = std::min(duration, std::chrono::milliseconds(std::chrono::seconds(60)));
        |                         ^~~~~~~~~
  /root/target_ws/src/pf_lidar_ros_driver/pf_driver/src/pf/pf_interface.cpp:221:46: error: ‘feed_time’ was not declared in this scope
    221 |   watchdog_timer_ = node_->create_wall_timer(feed_time, std::bind(&PFInterface::feed_watchdog, this));
wawanbreton commented 2 years ago

Commit restored. Please don't force-push unless you are alone on a repos :)

wawanbreton commented 2 years ago

Build error should be fixed, I used a declaration that is only available with C++17 but foxy targets C++14

hsd-dev commented 2 years ago

better to rename the branch to foxy-devel

hsd-dev commented 2 years ago

Should we remove these two commits for now? https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/83/commits/3f1385c7fd3d02e49c5014705406aa031fbfd073 and https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/83/commits/c6c290fb428674dd6ca729039dc04990b9546594. For this PR I want to focus on porting existing functionality to ROS2 and not add new features mid-way. I can create a new PR with those 2 commits stacks behind this one.

wawanbreton commented 2 years ago

I completely agree, I was wondering about splitting them to an other PR. Maybe this should be the case for https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/83/commits/d75100945368923990de207e8ce2ad479ae97bdf too ?

wawanbreton commented 2 years ago

Hi @ipa-hsd, I'm starting to rework the parameters stack because some things are not properly done at initialization. I'm wondering about the parameters philosophy, I can see two possible options :

  1. All parameters given to driver are set to the LIDAR, other parameters are forced to default values. LIDAR previous configuration is not taken care of.
  2. All parameters given to driver are set to the LIDAR, other parameters are read from the LIDAR and stored into actual parameter values.

Option 1 is more repeatable because whatever you did before, the LIDAR will always start in the same configuration. Option 2 considers you may want to store some configuration into the LIDAR definitely. Personnally, I prefer option 1 because we can easily set all the required parameters into a config file, so I don't really see a need to store configuration on long-term. What do you think should be implemented ?

hsd-dev commented 2 years ago

Thanks for taking a look into this.

some things are not properly done at initialization

Anything specific?

Maybe @ptruka has something to say about this based on the customer feedback?

@wawanbreton also I am starting to think about the fact the base cpp code (not ROS specific parts) is starting to diverge from the code for ROS1. I am not sure to what extent it is possible to reuse the same classes, but something we might want to keep in mind. One possibility could be whatever inconsistencies / bugs you have fixed in this PR, we could propagate it back to the ROS1 code as well.

wawanbreton commented 2 years ago

Anything specific?

Yesterday I implemented start_angle parameter, but then figured out that it was not applied at startup. I'm working on it.

One possibility could be whatever inconsistencies / bugs you have fixed in this PR, we could propagate it back to the ROS1 code as well

Until now, I only fixed the bugs I created by changing the parameters description. The management is very different from the one with ROS1, so I'm not sure we could have the same code base for both. I'd be happy to really refactor this part, but I would understand you want to keep consistency between versions. However, I only changed the ROS-related code but all low-level communication code is still the same.

hsd-dev commented 2 years ago

As long as we ensure that the behaviour is the same in ROS1 and ROS2, it should be fine. I can think of some tests for that.

wawanbreton commented 2 years ago

As long as we ensure that the behaviour is the same in ROS1 and ROS2, it should be fine. I can think of some tests for that.

Great, I'll keep going this way. And It would make me feel better to have some tests indeed :)

hsd-dev commented 2 years ago

@wawanbreton it would be better to remove the commit https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/83/commits/1244f3d42f1cd3584cb84fd5127f365a69f4f52f because there are 2 PRs open https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/77 and https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/81 which are changing quite some cpp code. Bringing those changes to this PR would not be possible with a simple rebase if we do a major refactoring. As mentioned in https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/83#issuecomment-1152143475, it would make sense to use this PR to make only ROS-specific changes and make a new PR for cpp code improvements.

wawanbreton commented 2 years ago

it would make sense to use this PR to make only ROS-specific changes and make a new PR for cpp code improvements.

OK, I didn't understand this. The thing is, proper management of parameters will require some architectural changes, so I thought it would be the right time. But I can do something a bit less intrusive yet, even if it is a bit dirty, and make it clean later, if that's what you want.

The protocol-specific code is intended to be used like a pure cpp library with no ROS dependency, apart from logging

Noted, it perfectly makes sense !

hsd-dev commented 2 years ago

The thing is, proper management of parameters will require some architectural changes, so I thought it would be the right time.

I agree, but some logic has changed and couple of new features have been introduced in those PRs. I have added that commit as part of TODO in this PR's description.

wawanbreton commented 2 years ago

Ok, I will wait for your go to work on this.

hsd-dev commented 2 years ago

The CI is failing because the code is not formatted properly https://github.com/PepperlFuchs/pf_lidar_ros_driver/runs/7018472659?check_suite_focus=true#step:4:174 Could you apply the clang formatting? (If you are using VS Code, right click on the file and "Format document with...").

The link above shows which documents need to be updated.

wawanbreton commented 2 years ago

I'm not using VS Code, but I tried applying formatting using clang-format. However, the whole file is changed and becomes very different... Is there a formatting profile you are implicitely using ?

hsd-dev commented 2 years ago

This one: https://github.com/PepperlFuchs/pf_lidar_ros_driver/blob/main/.clang-format

wawanbreton commented 2 years ago

Seems I got it right :) I configured clang-format to style "FIle" so it probably found it by itself. Thank you for pointing it to me, I'd like to use it for my own projects !

hsd-dev commented 2 years ago

@wawanbreton both https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/77 and https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/81 have been merged now. Could you apply https://github.com/PepperlFuchs/pf_lidar_ros_driver/commit/1244f3d42f1cd3584cb84fd5127f365a69f4f52f to the latest main branch? Please make a new PR to the main branch with those changes. Then we can rebase this PR from main.

wawanbreton commented 2 years ago

Ok, I have started but that's a lot of work, because commit https://github.com/PepperlFuchs/pf_lidar_ros_driver/commit/1244f3d42f1cd3584cb84fd5127f365a69f4f52f is based on the ROS2 version and the main branch is based on the ROS1 version. So I have to backport the changes to finally re-port them to the ROS2 version. Is that what you intend to ? By the way, I'm leaving on holiday this evening for a few weeks, so it may not be available soon.

hsd-dev commented 2 years ago

No worries, I can make those changes based on https://github.com/PepperlFuchs/pf_lidar_ros_driver/commit/1244f3d42f1cd3584cb84fd5127f365a69f4f52f and you can carry on from there.

hsd-dev commented 1 year ago

@wawanbreton I have rebased this PR from https://github.com/PepperlFuchs/pf_lidar_ros_driver/pull/87. Now we are good to go.

chenwu-cc commented 1 year ago

我在推送到这个分支时被拒绝了，你能给我一些权利吗？

can you get the points in rviz? my ROS2 version is galactic .when I using the porting ros2 I can get messages but can't get points. So what should I do to solve this problem thank you.

hsd-dev commented 1 year ago

@chenwu-cc thanks for testing the branch. I thought the problem was only on my PC. I will test it today again and get back to you.

chenwu-cc commented 1 year ago

May I ask if I use the galactic version could I run porting ros2.

hsd-dev commented 1 year ago

May I ask if I use the galactic version could I run porting ros2.

yes I am working with galactic as well

hsd-dev commented 1 year ago

@chenwu-cc could you please try this PR again? I fixed the issue.

ptruka commented 1 year ago

Many thanks to everyone for their support! Due to the general changes to the code and the resulting differences to the ros1 driver, a repository for ros2 was created: https://github.com/PepperlFuchs/pf_lidar_ros2_driver