Updated ROS2 PR: pcl_conversion only to ROS2 (dashing) #244

klintan · 2019-08-21T02:43:13Z

@LucidOne I made another branch on my fork for (dashing compatible) which only contains the pcl_conversion part and not pcl_ros (COLCON_IGNORE added).

Maybe this could be a first step to a ROS2 version, before the type masquerading etc is implemented in ROS2 ?

Would love some feedback on this,

klintan · 2019-08-23T00:30:09Z

Perhaps someone can take a quick look at the updated travis config: It seems it's just picking up the old master-branch (aka ros1). Haven't used travis much so not sure where and how the code is fetched.

mabelzhang · 2019-09-12T19:43:41Z

There is currently a version of pcl_conversions that is released into Dashing (Debian ros-dashing-pcl-conversions).

As mentioned here ros2/pcl_conversions#5, the opinion from the creator @clalancette of that version is that, that version can be somehow merged with this PR and deprecated, before this repo can be released into Dashing. We can continue to port the rest of this repo (pcl_ros) to Dashing.

mabelzhang · 2019-09-25T22:49:22Z

I made a PR to merge the ros2/pcl_conversions that is officially released into Dashing but a bit dated, into this PR.
klintan#1

Merge with pcl_conversions officially released in Dashing

klintan · 2019-09-25T23:10:16Z

I made a PR to merge the ros2/pcl_conversions that is officially released into Dashing but a bit dated, into this PR.
klintan#1

Awesome thanks! I merged that one without any review so we can keep the review here (or wherever the final review takes place)

pcl_conversions/CMakeLists.txt

SteveMacenski · 2019-09-26T18:27:38Z

pcl_conversions/include/pcl_conversions/pcl_conversions.h

  {
-    fromPCL(pcl_header.stamp, header.stamp);
-    header.seq = pcl_header.seq;
+    auto time_stamp = std::make_shared<rclcpp::Time>(header.stamp);


You make a shared pointer to deference it, you just need the object. If for some reason you need a pointer, use the unique_ptr

I was thinking just to make sure it gets freed when we go out of the scope. Is there another way without using new and delete ?

Why do you need a pointer at all? You just dereference it soon after

No probably not. I was just thinking that if I use a smart pointer, it will automatically delete the object after the function scope is gone, no ?

Constructing a new object would mean I need to do something like:

auto time_stamp = new rclcpp::Time(header.stamp); fromPCL(pcl_header.stamp, time_stamp); delete time_stamp;

But maybe that is not the way smart ptrs are intended to be used. Do you have any other recommendations on how to write this or is the code sample above an OK approach?

You can just make a stack variable:

fromPCL(pcl_header.stamp, rclcpp::Time(header.stamp));

No smart pointers, pointers, or heap allocations required, and a single line of code :).

There's a number of reasons to do one versus the other, but that is one reason, yes.

There's a real performance downside with using heap memory so its best to not do it unless you require it. Plus if you look at what you actually ended up doing:

create pointer

dereference pointer to pass to a method

There wasn't anything that used it in pointer format, so there wasn't anything to be derived from the allocation

Typically, if you are only going to use it in this function (i.e. this stack frame), you do a stack allocation. If it needs to live beyond this function, then you do a heap allocation (either with a "raw" pointer or a smart pointer). Size can also come into it, as on Linux the default stack size is limited by default (8MB if I remember correctly, but don't quote me).

Not to pile on, but also if you're concerned about real-time or software certifications, heap allocations are the devil

Thanks for all elaborate examples and explanations, really valuable!

Stack variable worked, however couldn't get it to compile using the one-liner
fromPCL(pcl_header.stamp, rclcpp::Time(header.stamp));

pcl_conversions/include/pcl_conversions/pcl_conversions.h

pcl_conversions/package.xml

perception_pcl/CMakeLists.txt

perception_pcl/package.xml

SteveMacenski · 2019-09-26T18:30:38Z

pcl_conversions/include/pcl_conversions/pcl_conversions.h

-  } // namespace ros::serialization
-
-} // namespace ros
+//     template<>


This is for type masquerading?

Hm good question, I don't have an answer to that.

@clalancette that's what this is for, correct?

SteveMacenski · 2019-09-26T18:33:00Z

I'm going to assume all the time conversions with ns / us / s were done correctly or my brain will start hurting from looking at docs...

SteveMacenski · 2019-09-26T18:34:08Z

Another good question is where are the maintainers here? @wjwwood @wkentaro do you guys need some assistance to keep this going in ROS2?

clalancette · 2019-09-26T18:39:20Z

I'll point out that CI is failing here, because of this message:

ERROR: the following packages/stacks could not have their rosdep keys resolved

to system dependencies:

pcl_conversions: Cannot locate rosdep definition for [pcl_msgs]

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Function 'install_target_dependencies' returned with code '1' after 0 min 1 sec

The command ".industrial_ci/travis.sh" exited with 1.

It looks like someone needs to release pcl_msgs into dashing first.

SteveMacenski · 2019-09-26T18:41:50Z

@clalancette this is sounding a whole lot like my robot_localization ongoing effort. You just want to add me to ros-perception and I can hammer this all out?

I dont like the idea of blocking process for msgs ;-) also I'm already maintaining image_pipeline and there's a bunch of PRs / issues in the SLAM projects I want to fix

SteveMacenski · 2019-09-26T20:13:04Z

So I'm trying to understand where things are at right now and I'm a bit confused.

@mabelzhang you did some work to update @klintan branch, which as been merged and is represented in this PR, correct?

@mabelzhang your PR homologates the ros2/pcl_conversions repository, correct?

So now if we merge this (into what needs to be a new ros2/dashing branch, not crystal) and release it, we can nuke the ros2 fork (IMO not even archive it, delete it from existence since pcl_conversions perception_pcl were confusing enough in ROS1, now add in ROS2 some forks, some branches, etc....)

Is that a correct assessment?

Action item I can take

It doesnt look like there have been maintainers around in this for a while, I can try to get on that
PCL msgs aren't released, I can get on that
Make ros2/dashing branches
Merge this PR
Either begin porting pcl ros, which I probably can't commit to doing right at this moment, or make active inquires to parties I know have resources to do it

clalancette · 2019-09-26T20:37:08Z

So now if we merge this (into what needs to be a new ros2/dashing branch, not crystal) and release it, we can nuke the ros2 fork (IMO not even archive it, delete it from existence since pcl_conversions perception_pcl were confusing enough in ROS1, now add in ROS2 some forks, some branches, etc....)

Just FYI, I'll do with the old ros2/pcl_conversions what I've done elsewhere, which is to do a "final" commit on the repository, removing all of the content and adding a README pointing to here. Then I'll archive it. At the very least we still need to keep it around for historical purposes and since we still support Crystal.

SteveMacenski · 2019-09-26T20:38:08Z

When's crystals cut off date?

Fair enough

mabelzhang · 2019-09-26T21:03:13Z

I'm going to assume all the time conversions with ns / us / s were done correctly or my brain will start hurting from looking at docs...

I will leave that for @klintan to answer. The only changes I made to time were the rclcpp::Time(uint sec, uint nanosec) things in the test script, adding 0s to the calls to ros::Time(double) till there are 9 digits after the decimals for nanoseconds.

It looks like someone needs to release pcl_msgs into dashing first.

That explains why I had to check out the pcl_msgs repo in order for colcon to find its headers.

@mabelzhang you did some work to update @klintan branch, which as been merged and is represented in this PR, correct?

Yes.

@mabelzhang your PR homologates the ros2/pcl_conversions repository, correct?

Yes.
I thought it might be hard to review the diffs since GitHub doesn't show diff between two different repos... So here's a list for reference. The only 6 differences between the two versions of pcl_conversions.h are as follows, and are resolved in my PRs:

Add #include for <rclcpp/rclcpp.hpp> and message_filters (present in perception_pcl PR, not present in pcl_conversions)
Use rclcpp::Time (in perception_pcl PR) instead of builtin_interfaces::msg::Time (in pcl_conversions). The latter used that type because rclcpp::Time didn't exist when that was ported. melodic-devel uses ros::Time, so the better replacement is rclcpp::Time.
In PCLHeader <=> Header toPCL(), set seq field to 0 (present in pcl_conversions, not present in perception_pcl PR)
Uncommented everything related to pcl_msgs (commented out in pcl_conversions, uncommented in perception_pcl PR)
Minor change in this comment (in perception_pcl PR), added msg:: (in pcl_conversions):
/** Provide to/fromROSMsg for sensor_msgs::PointCloud2 <=> pcl::PointCloud<T> **/
Added commented code (in pcl_conversions) for the last block related to message_traits, because the convention for porting is to comment out ROS 1 code rather than to remove, so that when it's ready to port, there's an existing framework to base off of.

The test script, package.xml, and CMakeLists.txt were also merged.

Either begin porting pcl ros, which I probably can't commit to doing right at this moment, or make active inquires to parties I know have resources to do it

We might be able to do this. That's my next thing to do.

When's crystals cut off date?

Looks like Crystal EOL is Dec 2019 https://index.ros.org/doc/ros2/Releases/

SteveMacenski · 2019-09-26T21:26:23Z

@mabelzhang I'll take your word on the porting, everything I saw in this composite PR looks fine to me except for the things I pointed out, I just wanted to know if that was still a pending item on top of this PR or not. I assume you ran those tests and they came out clean?

With December 2019 being the cut off, and its essentially October 1 before this gets merged it, I'd even say we can just delete the pcl_conversions fork, but I don't really care that much to fight one way or another. Whatever's easiest.

mabelzhang · 2019-09-26T21:30:25Z

I assume you ran those tests and they came out clean?

Yes, those tests in test/ passed.

klintan · 2019-09-26T22:16:11Z

I've been super busy this week, but will fix all the outstanding issue on Saturday.

mabelzhang · 2019-09-27T02:10:55Z

I won't be able to get back to this till next week if there are things specific to my merge.

klintan · 2019-10-01T04:24:07Z

I'm going to assume all the time conversions with ns / us / s were done correctly or my brain will start hurting from looking at docs...

Which ones are you referring to?

I think the rest should be addressed except for the few follow up question I posed. (Namespace question and if everything looks OK in the CMakelists.txt file)

Let me know if there is still something outstanding on this.

SteveMacenski · 2019-10-01T15:19:28Z

Releasing PCL msgs to https://github.com/ros2-gbp/pcl_msgs-release

@clalancette @wjwwood can you add me to the pcl_msgs repo so I can cut a new release on the ROS2 branch? Or just do it yourself if you have access

mabelzhang · 2019-10-02T15:32:39Z

@paulbovbel @wkentaro This PR is almost ready to go. The pcl_conversions fork officially released in Dashing is merged to here too. We want to get perception_pcl released in Eloquent, even if it requires putting COLCON_IGNORE in pcl_ros. We're just waiting on dependency pcl_msgs to be released. Could you take a look at this PR and add any comments regarding merging it?

SteveMacenski · 2019-10-15T04:06:13Z

PCL msgs are released now for dashing and eloquent. Still need a maintainer here to review this

SteveMacenski · 2019-10-15T19:26:58Z

Created dashing branch and retargeted PR to it, @klintan I see another draft PR, is that necessary?

I have write access to this repo now to help get this over the line

SteveMacenski

nice nice, we're super close!

SteveMacenski · 2019-10-16T20:02:48Z

pcl_conversions/CMakeLists.txt

+  std_msgs
+)
+
+set(includes


I dont think you require this with the dependencies variable and the ament depends call

Had to include the non-ROS package here (PCL) otherwise I got an error, couldn't find the header.

Maybe I'm missing something ? since I had the same issue for the navsat_transform_node in the robot_localization package.

pcl_conversions/CMakeLists.txt

pcl_conversions/include/pcl_conversions/pcl_conversions.h

pcl_conversions/package.xml

klintan · 2019-10-24T04:55:35Z

Created dashing branch and retargeted PR to it, @klintan I see another draft PR, is that necessary?

I have write access to this repo now to help get this over the line

We could close that one, that one contains some changes to pcl_ros as well to try to convert to ROS2, but the effort stalled a bit (mostly because of the type masquerading not being available yet). My c++ skills are not on the level yet to be able to workaround that.

SteveMacenski · 2019-10-24T17:04:48Z

Got it, I guess just leave it there for now and I can close the draft PR when there's motion to getting pcl ros ported. An example still have alot of value

pcl_conversions/CMakeLists.txt

SteveMacenski · 2019-10-24T17:08:48Z

pcl_conversions/include/pcl_conversions/pcl_conversions.h

  namespace message_traits
  {
-    template<>


Sorry maybe I was too non-specific, can you add ALL of this stuff back in, but then just comment it ALL out since there is no type masquerading yet? Revert to namespace ros {...} and just block comment it all out with a TODO on type masquerading.

pcl_conversions/package.xml

SteveMacenski · 2019-10-24T17:10:02Z

pcl_conversions/package.xml


-  <build_export_depend>eigen</build_export_depend>
+  <build_depend>eigen3</build_depend>


Oops this should be eigen please change back :-)

You can always check here https://github.com/ros/rosdistro/blob/master/rosdep/base.yaml

pcl_conversions/package.xml

SteveMacenski · 2019-10-24T17:13:20Z

Last few things to dot our 'I' and cross out 't'

…ded maintainer

klintan · 2019-10-26T20:13:24Z

Last few things to dot our 'I' and cross out 't'

Yeah thanks for hanging in there, I think I got most of the kinks ironed out now, but if you find something new or if I missed something let me know.

SteveMacenski · 2019-10-26T20:36:31Z

This looks good to me. Let me look at this on Monday and play with on my machine.

CI is failing, which seems related to switching to industrial ci. If you could make that pass that’d be great but I’ll also look into it

klintan · 2019-10-27T15:53:05Z

This looks good to me. Let me look at this on Monday and play with on my machine.

CI is failing, which seems related to switching to industrial ci. If you could make that pass that’d be great but I’ll also look into it

Yeah I saw that
pcl_conversions: No definition of [ament_cmake_gtest] for OS [debian]
failing on debian.
Do you know if there is a docker for debian and dashing? Looking here https://github.com/osrf/docker_images/tree/1eb48f1275f0a9153d0b0ed088bed338f1a79ead/ros and docker hub I could only find ubuntu :( So I couldn't test it ( I mean I can always build my own, but just wanted to ask first if there might be one already out there, I'm guessing Travis is using one for instance)

SteveMacenski · 2019-10-27T21:22:02Z

I know alot of repos use the industrial CI so there must be a way to stop that. Otherwise everyone would be broken, those ament cmake packages are super low dependencies. For right now just targeting ubuntu is fine

SteveMacenski · 2019-10-28T00:29:00Z

@klintan just remove - OS_NAME=debian OS_CODE_NAME=stretch from your updated CI file. You don't need to run that

SteveMacenski

Just remove the matrix build for debian and make the matrix builds expandable, and we can merge this guy

.travis.yml

SteveMacenski

@clalancette want to give this another once over? I'm happy with it

clalancette · 2019-10-29T13:33:27Z

@clalancette want to give this another once over? I'm happy with it

Honestly, I'm not going to have time this week with ROSCon. I've verified that cartographer_ros still builds with it, so that's going to have to be good enough. Anything else we can fix in a follow-up.

pcl_conversion to ros2

ff306b4

klintan changed the title ~~pcl_conversion to ROS2 (dashing)~~ Updated ROS2 PR: pcl_conversion only to ROS2 (dashing) Aug 21, 2019

klintan added 5 commits August 22, 2019 11:29

travis for ros2 and fixed cmakelists

e5d6900

exporting dependencies

c8cd006

updated travis

3f9d5dd

fixed smaller things

7d8a2c3

added colcon ignore to meta package

a8add54

mabelzhang added 3 commits September 25, 2019 03:33

merge pcl_conversions.h with version released in Dashing

1364379

merge test script, CMakeListst, package.xml

2ac649d

append authors, clean up

3c28513

Merge pull request #1 from mabelzhang/dashing-devel

b0cd15b

Merge with pcl_conversions officially released in Dashing

mabelzhang mentioned this pull request Sep 26, 2019

Repo obsolete or move into perception_pcl branch -- Archive (or delete as to not confuse)? ros2/pcl_conversions#5

Closed

SteveMacenski requested changes Sep 26, 2019

View reviewed changes

PR feedback

4cacf5f

clalancette mentioned this pull request Oct 10, 2019

Update for Eloquent ros2/cartographer_ros#38

Closed

SteveMacenski changed the base branch from crystal-devel to dashing-devel October 15, 2019 19:26

SteveMacenski requested changes Oct 16, 2019

View reviewed changes

fixed PR comments

ea4e59e

SteveMacenski requested changes Oct 24, 2019

View reviewed changes

fixed PR comments, cmakelists message_filters, reverted type masq, ad…

acdf1ce

…ded maintainer

SteveMacenski requested changes Oct 28, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

removed dashing and made ros_distro matrix env

a44ac0c

SteveMacenski approved these changes Oct 29, 2019

View reviewed changes

SteveMacenski merged commit a1fd4d2 into ros-perception:dashing-devel Oct 29, 2019


		<build_export_depend>eigen</build_export_depend>
		<build_depend>eigen3</build_depend>

Updated ROS2 PR: pcl_conversion only to ROS2 (dashing) #244

Updated ROS2 PR: pcl_conversion only to ROS2 (dashing) #244

Conversation

klintan commented Aug 21, 2019

klintan commented Aug 23, 2019

mabelzhang commented Sep 12, 2019

mabelzhang commented Sep 25, 2019

klintan commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Sep 26, 2019

SteveMacenski commented Sep 26, 2019

clalancette commented Sep 26, 2019

SteveMacenski commented Sep 26, 2019 • edited Loading

SteveMacenski commented Sep 26, 2019 • edited Loading

clalancette commented Sep 26, 2019

SteveMacenski commented Sep 26, 2019

mabelzhang commented Sep 26, 2019 • edited Loading

SteveMacenski commented Sep 26, 2019

mabelzhang commented Sep 26, 2019 • edited Loading

klintan commented Sep 26, 2019

mabelzhang commented Sep 27, 2019

klintan commented Oct 1, 2019

SteveMacenski commented Oct 1, 2019 • edited Loading

mabelzhang commented Oct 2, 2019 • edited Loading

SteveMacenski commented Oct 15, 2019

SteveMacenski commented Oct 15, 2019 • edited Loading

SteveMacenski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klintan commented Oct 24, 2019

SteveMacenski commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Oct 24, 2019

klintan commented Oct 26, 2019

SteveMacenski commented Oct 26, 2019

klintan commented Oct 27, 2019

SteveMacenski commented Oct 27, 2019

SteveMacenski commented Oct 28, 2019

SteveMacenski left a comment

Choose a reason for hiding this comment

SteveMacenski left a comment

Choose a reason for hiding this comment

clalancette commented Oct 29, 2019

SteveMacenski commented Sep 26, 2019 •

edited

Loading

SteveMacenski commented Sep 26, 2019 •

edited

Loading

mabelzhang commented Sep 26, 2019 •

edited

Loading

mabelzhang commented Sep 26, 2019 •

edited

Loading

SteveMacenski commented Oct 1, 2019 •

edited

Loading

mabelzhang commented Oct 2, 2019 •

edited

Loading

SteveMacenski commented Oct 15, 2019 •

edited

Loading