Hdl32e time sync #53
Hdl32e time sync #53
Conversation
This commit attempts to update the hdl32e GPS/IMU node started by ddillenberger. It updates the time offset calculations between the velodyne's time-since-hour counter and the GPS NMEA string to provide the correct reconstructed timestamp. It also changes most of the message types. The sensor scan time is now published as a TimeReference message, so that it can be correlated with the imu and temperature messages via the publish time. It now publishes the NMEA string as a nmea_msgs/Sentence message for easier processing by the navsat drivers. It also changes the imu publishing from the geometry_msgs/TwistStamped message to sensor_msgs/Imu.
|
Is this proposed as a replacement for #36? It looks better in several ways. Thanks for continuing this effort. |
|
Yes it is. I started with the commits from #36, addressed some of the comments in the pull request, and tried to fix the timestamp issue. |
This commit fixes the coordinate frames in the IMU data to be consistent with the ROS forward-left-up coordinate frame convention. The X direction is straight opposite the HDL32e's cord, Y is to the left, and Z is up.
|
It looks like the time sync part is working, the second rollover in the TimeReference packet looks like: I need to double-check that an hour rollover works as well. I also fixed the coordinate frame of the IMU to match http://wiki.ros.org/geometry/CoordinateFrameConventions. Now X is forward opposite the HDL32's cord, Y is to the left, and Z is up. The gyros should also be correct in terms of positive and negative rotation about each axis. |
|
Looks good, but I thought the new "auto" specifier was only defined since C++11. The ROS target platforms REP still specifies C++03 even for Jade, except when "checked for at configure time and equivalent functionality can be provided with the extra compiler features". |
Remove in-line function definitions and the use of the "auto" keyword to allow the driver to be built by compilers without c++11 support
|
I removed the auto specifiers, as well as the in-line function definitions within the handlePacket function. Things should be C++03 compatible now. I still need to double-check the hour rollover. I have a bagfile recorded, but I need to find some time to analyze it. |
| <!-- <url type="website">http://wiki.ros.org/velodyne_gps_imu</url> --> | ||
| <author email="dillenberger@uni-koblenz.de">Denis Dillenberger</author> --> | ||
| <buildtool_depend>catkin</buildtool_depend> | ||
| <build_depend>geometry_msgs</build_depend> |
There was a problem hiding this comment.
Missing dependencies on nmea_msgs.
|
Your implementation looks clean to me, although I lack detailed knowledge of these sensors and their behavior. We need to add a PCAP-based unit test, similar to the velodyne_driver ones. I can help with that, but we need a small PCAP file that can drive it. Maybe the existing http://download.ros.org/data/velodyne/32e.pcap will suffice. I think it's essential to have an option to run from PCAP data, so we can reproduce any problems users report. |
|
Please add a copyright notice to the C++ files. It can be something similar to what we already do. Please use the modified BSD license, if allowable. |
Added BSD copyright headers to the source files, and updated the build and run dependencies in CMakeLists.txt and package.xml
|
I've added a copyright header consistent with the BSD license @ddillenberger specified in package.xml. I've also updated the build and run depends in CMakeLists and package.xml to reflect the new messages being used. I'm not really sure how to address the desire for a pcap file. The 32e is publishing packets containing NMEA strings and accelerometer info in parallel to the pointcloud data, over a different port. The driver is currently set up to read directly from the UDP port and parse the driver information. The 32e.pcap file you pointed to doesn't contain any of the accelerometer packets that this driver parses, it looks like it only contains scan packets. |
|
Back from ROScon now. Sorry for the delay. If you'll send me a small PCAP that contains the required packets, I'll see about getting it copied to the build farm site. You'll also need to add an option for your driver to read data from a PCAP file, similar to the way velodyne_driver does it. I don't have time to do that for you right now, but I can point you to the relevant parts of the code if needed. |
|
@jpgr87: The new VLP-16 model also provides position packets on port 8308.
|
|
Based on the user manuals, the structure of the position packets the VLP-16 emits is similar to the HDL-32e. The VLP-16 does not supply accelerometer, temperature, or gyro data, so this node will currently read whatever values are present at those positions in the packet (zeros according to the wireshark screenshot in the manual,) scale them and publish them to the Imu and Temperature topics. The TimeReference and NMEA sentence topics should work normally when a GPS is connected, as the GPRMC sentence is in the same place in the packet. When a GPS is not connected, the HDL-32e will generate its own GPRMC sentences starting at January 1, 2000, which the driver will parse and use to generate a bogus TimeReference. I don't know what the VLP-16 does when there is no GPS connected: if it publishes invalid or blank GPRMC messages then the driver will try to act on them anyway. The node should do some error checking in the case where there's no GPRMC message at all (e.g. not publish the TimeReference and NMEA sentence at all.) I can add that pretty easily, but I don't have access to a device to test with anymore. |
|
That sounds reasonable. We still need to add PCAP support both for testing and for further development, without needing constant access to real devices. I recently collected a small PCAP from a VLP-16 without a GPS. It does contain port 8308 position packets of some kind; I haven't looked at them very closely yet. They're probably just showing time since device power-on. I think I'll merge this PR into a separate branch, where we can conveniently work together on finishing it. The easiest way to add PCAP support is probably to export the |
|
I just merged this PR into a new |
|
@richmattes - Are you still interested in working on this? I know it's been some time but I've updated the branch to the latest |
|
Unfortunately I no longer have the time to support this, and even if I did I don't have access to any of the hardware anymore. |
|
Thank you. I'll see if I can get someone internally to take a look at it. |
|
Solved by #222. |
This pull request builds on the work in pull request #36. I've changed some of the message types and attempted to more robustly reconstruct the timestamp of each scan from the NMEA string and the counter in each message.
I haven't gotten in to the lab to test it yet, but I wanted to push it in case someone else gets a chance to test it before I do.