Need a new, different model name to support HDL-64E S2 and S2.1

[jack-oquin]

Those models produce data at about 1.333 MB/s, different from the 1.0 MB/s of the older HDL-64E. That makes the packet_rate calculation wrong.

References:

• Velodyne HDL-62E S2 and S2.1 product manual (PDF)
• http://answers.ros.org/question/67320/packet-rate-of-velodyne-hdl-64e-in-velodyne_driver/

For user convenience, we should construct the new model parameter string with no spaces, perhaps something like 64E_S2.x.

[gabor-meszaros]

Hello Jack,

I am working on it, but I have a question. I cloned the rosbuild branch and I tested my modifications with that. It works fine.

For this patch I think I have to use the master branch and for that I have to download also the pcl_conversions because it is required by velodyne_pointcloud. I downloaded it but I cannot compile it because catkin_make cannot find the pcl/conversions.h header file.

The error message is:

In file included from /home/gabor/Development/catkin_ws/velodyne/src/velodyne_pointcloud/src/conversions/transform.cc:22:0:
/home/gabor/Development/catkin_ws/velodyne/src/pcl_conversions/include/pcl_conversions/pcl_conversions.h:43:29: fatal error: pcl/conversions.h: No such file or directory

I am not so familiar with ros-pcl makefile settings. Did I do something wrong? Is this error also exists at you?

Thank you,
Gabor

[jack-oquin]

The catkin version in master is for Hydro. Unfortunately, PCL 1.7 was recently released in Hydro, with incompatible API changes that broke velodyne_pointcloud and several other packages. I had to quickly release a fix, because that broke the build farm.

Before that, master worked on Groovy as well as Hydro. Unfortunately, the new fix only works with PCL 1.7, and no earlier versions. Since PCL 1.7 is not yet released to Hydro, building the current master requires installing it and pcl_conversions from the ros-shadow-fixed repository. You do that by updating /etc/apt/sources.list.d/ros-latest.list. I'll provide detailed instructions, if you need them.

I regret this mess, although it is not my fault. If I can figure out a way to make the same source work with both versions, I'll probably do that. But, I don't think it's possible right now.

[jack-oquin]

Issue redirected here: ros-perception/pcl_conversions#3.

[gabor-meszaros]

Thank you for your patient. Now, I can build with the new groovy-devel pcl_conversions but the transform part of the velodyne_pointcloud do not work.

The velodyne_driver is executed and it publishes on the /velodyne_packets topic. The cloud_nodelet can display it on the /velodyne_points.

Unfortunately the transform_nodelet do not, it does not publish anything on the /velodyne_points. If i set the frame_id to odom directly it writes to the ERROR log the following:

[ERROR] [1374234040.902849074]: Frame id /odom does not exist! Frames (1): 

The error message is written by this line in the transform.cc:

pcl_ros::transformPointCloud(config_.frame_id, inPc_, tfPc_, listener_);

I checked the frames with tf_monitor, view_frame and it is right, there is not any frame. But it still works with the rosbuild/groovy version.

I use the master branch of velodyne and the groovy-devel branch of pcl_conversions.

Do you have any suggestion what can the problem be?

[jack-oquin]

There is a unit test for the transform nodelet. You can run it this way:

$ roscd
$ cd build
$ make tests       # builds tests and downloads some PCAP data files
$ roscd velodyne_pointcloud
$ rostest tests/transform_nodelet_hz.test

That defines a static transform between /velodyne and /odom. It will log a large number of error messages until the transform becomes available, then it should complete successfully. Because it takes a long time for the tf information to get in sync with the messages, the frequency of messages published is not reliable. Consequently, that test was often failing on the build farm and is currently commented out (see: issue #2).

Please try that unit test and see if it works for you (despite error messages). I just ran it here on Groovy with the current master and the new groovy-devel branch of pcl_conversions in my workspace. It worked for me.

The idea on a real vehicle is that once the /odom transform syncs up with the packets coming from the device, the tf information should be available in a timely fashion. We transform each packet on arrival to minimize data smearing due to vehicle movement while the device rotates. But, sometimes tf makes that tricky.

[gabor-meszaros]

I have tried to run the tests but it did not work for me. The output of the rostest is:

[FATAL] [1374484265.328814083]: Error opening Velodyne socket dump file.
[FATAL] [1374484265.404724326]: Service call failed!
[FATAL] [1374484265.404762514]: Service call failed!

If I delete the build and devel directories and I run the catkin_make command, it writes that the velodyne_driver and velodyne_pointcloud unit testing is disabled. After catkin_make the output of the make test in the build directory is the following:

Scanning dependencies of target tests
Built target tests

I think it should download the necessary files in this step but it does not download anything. I have tried the catkin_make with the -DCATKIN_ENABLE_TESTING=1 argument but it results an error because cmake does not recognizes the catkin_download_test_data command:

-- ==> add_subdirectory(velodyne_driver)
velodyne_driver unit testing enabled
CMake Error at velodyne_driver/CMakeLists.txt:44 (catkin_download_test_data):
  Unknown CMake command "catkin_download_test_data".


-- Configuring incomplete, errors occurred!
Invoking "cmake" failed

Maybe it is useful if I share my current workspace with you. I did the following:

cd ~/Development/catkin_ws/
mkdir velodyne
cd velodyne
git clone https://github.com/ros-drivers/velodyne.git src -b master
cd src
catkin_init_workspace
git clone https://github.com/ros-perception/pcl_conversions.git -b groovy-devel # I want to delete this folder before check in
cd ..
catkin_make
source devel/setup.bash

Do I setup my workspace correctly?

[jack-oquin]

Most of what you tried looks sensible. The workspace setup looks wrong, packages should be always be checked out in the src/ subdirectory. I recommend using wstool to maintain workspaces. This tutorial should get you started.

Make sure you have the latest catkin version installed. Mine is 0.5.70.

You should probably follow the tutorial and make a fresh workspace with -DCATKIN_ENABLE_TESTING=1 set. That should be the default now, but I believe previous catkin versions did not set it.

[gabor-meszaros]

Thank you, it works! Every unit test passed. I had to update my catkin.

For others, I did the following:

source /opt/ros/groovy/setup.bash
cd ~/Development/catkin_ws
mkdir velodyne
cd velodyne
mkdir src
cd src
wstool init
wstool set velodyne --git https://github.com/ros-drivers/velodyne.git -v master
wstool update
wstool set pcl_conversions --git https://github.com/ros-perception/pcl_conversions.git -v groovy-devel
wstool update
cd ..
catkin_make
cd build
make tests
make test

I finished the patch and now I am working on the unit test. Unfortunately I have only a ~40 MB pcap file. Is it still acceptable? I cannot record another one in the near future.

Beside that I found something strange and I do not know if it is normal on LiDARs. It seems like every second scan line is bad at edges. (hdl-64e S2, rpm is 300, the lidar does not move)

I have checked the 32e unit test pcap file and it seems much better (rpm 780, lidar is moving) :

I do not want to check in a wrong unit test file. What do you think? Was some kind of post-processing applied on the 32e test file?

Thank you!

[jack-oquin]

Please do not check the PCAP file into github.

If you look at the other tests, they download the test data from http://pr.willowgarage.com/data/velodyne/ using catkin_download_test_data(). We'll have to get one of the OSRF people to upload it there for us, so the build system can reach it quickly and reliably.

Meanwhile, please e-mail me a copy of your PCAP, or put it on some public server from which I can download it. 40MB is fine to start with. We might want to record a smaller one at some point to save download time for testing, but it's not a big problem. We just want a raw packet dump from the device, no post-processing required.

I can't tell what the problem is from your image above. Why don't you take what you have and create a github pull request (minus the PCAP file)? That way I can run it myself and observe what happens over time. It may be that the npackets calculation is not quite right, or those may just be shadows cast by some close-up obstacle. Remember that the min_range parameter of velodyne_pointcloud defaults to two meters, so foreground obstacles can disappear.

[gabor-meszaros]

Sorry for the misleading compounding. I did not want to check in the test file itself. I meant I did not want to check in a catkin_download_test_data() call to a wrong test file.

I created the pull request some minutes ago and I uploaded the files to my Dropbox account because I could not find your email address. I sent the link in the pull request comment.

If you think, I gladly create the unit test if the patch works on your PC.

[jack-oquin]

Sorry for misunderstanding. You are right not to check in a version that depends on an inaccessible test file. We'll have to comment out that test or make it conditional until the file is uploaded. Wrapping it like this is probably best:

if (USE_64E_S2)
   ...
endif ()

That way it won't run by default on the build machines, but we can easily turn it on.

About the model name: are we ever likely to want to distinguish the S2 from the S2.1? I know we don't at the moment, but I already made that mistake once with the 64E, not knowing any better. Should we check them both explicitly instead of a generic 64E_S2.x, which feels rather clumsy now that I look at it again.

What do you recommend?

[gabor-meszaros]

I agree with you about the model name. I think we should use the explicit model name. We can easily add other model names if it is necessary. What we should try to avoid is to rename these model names (e.g 64E_S2.x to 64E_S2.1 and 64E_S2.2) in the future because it may break our users' existing code.

Our LiDAR's version is 2.1 but the 2.0 and 2.1 LiDAR's manual are the same. In this case we can introduce 2 versions: the 64E_S2.0 and the 64E_S2.1. If we explore some differences between them we can fix it without the "API" change. Of course the names are not the most beautiful ones...

The decision is yours. :)

Can I have another question? I think I found a bug in the velodyne_pointcloud's transform.cc file (processScan member function, in the body of the for):

// clear input point cloud to handle this packet
inPc_.points.clear();
inPc_.width = 0;
inPc_.height = 1;
inPc_.header.frame_id = scanMsg->header.frame_id;
std_msgs::Header header;
header.stamp = scanMsg->packets[next].stamp;
pcl_conversions::toPCL(header, inPc_.header);

The toPCL overwrites the inPc_header.frame_id with the header variable's frame_id which is an empty string by default.

My question is should I add the fix with this issue's pull request or I must separate them into two pull request? I do not know the policy.

[jack-oquin]

Good catch. I'm opening a separate issue for that bug. It appears to have been caused by the PCL 1.7 fix @wjwwood created, and it may affect more packages than just this one.

I checked the other pcl_conversion updates in this package, and they all look correct. So, go ahead and fix it in your pull request, in a separate commit.

To be certain: can you verify that the standard Groovy version of the driver works correctly for a similar test?

Re: model names: I agree that a specific name for each model seems best. Following the Velodyne names but substituting underscore for the blank gives: 64E_S2 (they don't seem to call it S2.0) and 64E_S2.1. This way, we explicitly list the models we support. When something new comes along it will need to be tested and added.

[gabor-meszaros]

Thank you! I forgot to send this comment on Friday (sorry):

I checked in the hdl-64e 2.x modifications that we talked about. I hope the pull request will be automatically updated. I also checked in the calibration file and two unit tests. I made these tests and the pcap downloading optional (and disabled by default) in the CMakeLists.txt.

I have tested with USE_64E_S2.1 and without it. All the tests were run correctly. (Groovy)

Could you also try to run them please? Thank you!

I also checked in the pcl_conversion fix.

Have a nice weekend! :)

[jack-oquin]

Thanks, I did. 😄

The tests work fine on my system. The PCAP file is now on pr.willowgarage.com, so I made the S2.1 test unconditional and kicked off a pre-release test to check for problems on the build farm.

If that runs cleanly, I'll probably make a new Hydro release 1.1.1 tomorrow. I do not plan to update the Groovy version, as it still uses rosbuild, but the latest master does work on Groovy when built from source.

[gabor-meszaros]

I have written an email to them too, but it looks like you were faster. Thank you! :)

I would like to create and issue about velodyne_pointcloud, but I think we need more time for that. I am writing it currently.

[jack-oquin]

fixed in release 1.1.1, submitted today