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

feat: add time_offset parameter #37

Open jadhm opened 3 years ago

jadhm commented 3 years ago

Hi,

I sent a pull request #36 to add a new parameter time_offset in case there is latency.

Regards, Jad

hsd-dev commented 3 years ago

https://github.com/PepperlFuchs/ROS_driver/pull/36#issuecomment-736545604

jadhm commented 3 years ago

@ipa-hsd could you please take a look at the pull request ?

jadhm commented 3 years ago

ping @ipa-hsd

hsd-dev commented 3 years ago

Though time offset is an issue, but the solution is not acceptable. It does not make sense to add a user-defined time offset.

jadhm commented 3 years ago

@ipa-hsd I see your point but if you looked at other drivers of other sensors you would see that they also have this parameter for solving latency. This is an example of the sick driver https://github.com/SICKAG/sick_safetyscanners/blob/cfa456e78af42b82c1031354864837a798add4a7/config/SickSafetyscannersConfiguration.cfg#L20

hsd-dev commented 3 years ago

Yes I have seen that and realized that was the basis of this PR. But does not necessarily mean it is a good solution.

Also ROS time is completely different from the device time. A ROS system is made up not just of laser scanner, but might involve many hardware and software components. Imagine if every hardware and software component modifies ROS time as per it's own clock.

The packet header is being published on /r2x00_header topic which has the same sequence number as in the PointCloud2 message. Probably you can use message_filters to filter messages from both topics as per the sequence number.

hsd-dev commented 3 years ago

I might be wrong. The REP does mention calibrate_time and time_offset.

jadhm commented 3 years ago

@ipa-hsd dealing with latency is a bit tricky but a parameter for fine tuning should be possible. Would you be open for re opening my PR ?

reinzor commented 3 years ago

@ipa-hsd , just to clarify: Nobody is altering the ros::Time. The ros::Time is determined by the system clock if /use_sim_time parameter in the global namespace is false. If true, the ros::Time is determined by a ClockPublisher that publishes on the /clock topic. This additional time_offset parameter is only required for manually adjusting the timestamps of the incoming sensor readings. If ros::Time::now() is used, it means that the sensor reading was created at the current time. However, this is ofcourse not the case since the message was created x seconds ago due to a acquisition and communication delay. The time_offset in sensor drivers is often introduces to cope for these "unmeasurable" delays. Hope this clears things up.

reinzor commented 3 years ago

@ipa-hsd any updates on this?