Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for accessing and mutating the timestamp mode on the LiDAR #36

Merged
merged 4 commits into from
Apr 13, 2020
Merged

Conversation

tpanzarella
Copy link
Member

The on-LiDAR timestamp_mode can now be set in the config file. Additionally,
its state is fedback in the GetMetadata service.

I HIL-tested these changes via visual inspection and everything seems to
be working correctly. I see that in #10 there is a plan add some
automated testing. I think this is a very good idea. I'll add some comments
to that issue shortly.

Closes: #34

The on-LiDAR `timestamp_mode` can now be set in the config file. Additionally,
its state is fedback in the `GetMetadata` service.

I HIL-tested these changes via visual inspection and everything seems to
be working correctly. I see that in #10 there is a plan add some
automated testing. I think this is a *very good* idea. I'll add some comments
to that issue shortly.

Closes: #34
@SteveMacenski
Copy link
Member

Looks good- the only other ask I think is also supporting a ROS time as well. It won’t be well synchronized but if they’re not using PTP then the clocks could diverge either way.

@tpanzarella
Copy link
Member Author

In terms of implementation for a ROS timestamp... I suggest we add a separate parameter (bool flag, default False) like use_reception_timestamp whose semantics indicate to explicitly ignore the LiDAR timestamp. Additionally, when running in this way, we should emit a log message at WARN level (once) alerting that we are applying timestamps in this way. I say that because, we know that the ROS time is explicitly incorrect and we have no real good way (without using something like PTP) to model how incorrect the stamp is. I'm happy to do this, but, wanted to share how I would plan to implement it. Do you agree?

Also, I would suggest we separate that out into its own PR as it sort of conflates issues. This PR focuses on which stamp is applied by the LiDAR. The subsequent one would handle whether or not use use the LiDAR stamp or a ROS timestamp (at time of data reception, not when the data are sampled).

LMK what you think.

@tpanzarella
Copy link
Member Author

I guess we could add something like TIME_FROM_ROS_RECEPTION in the timestamp_mode enum to keep the code cleaner. But, I'd still suggest we emit a warning since we know the stamp we are using is explicitly wrong.

@SteveMacenski
Copy link
Member

SteveMacenski commented Apr 12, 2020

That sounds reasonable. I mostly wanted in enum to be clean, I’m fine with a warning though. It also may be valuable in the readme to explicitly describe what those 3 modes do and why time syncing is important. I’ve run into PTP before from having clock skew issues but I wonder if its something we should specially mention the importance of and a one liner to enable on the host machine so the lidar can be a slave.

@tpanzarella
Copy link
Member Author

tpanzarella commented Apr 13, 2020

I’m working on a write up that I’ll post publicly on this topic specifically — PTP time sync using a Linux host as the PTP grandmaster and an Ouster LIDAR as a slave. It will be easiest to link to it from the README once I publish, it’s more than a one-liner (it will happen this week). PTP claims accuracy to w/in +/- 1 usec while the Ouster docs claim +/- 50 usec. I want to characterize the OS1 specifically and see how to best tune it. What I have seen so far is that it oscillates rather wildly (+/- 200 usec) around the set point (0, offset from master). I still have more analysis to do. Depending upon what I find out I may see if Ouster will expose some more knobs to the underlying ptp4l configuration so we can tune the servo params over the tcp interface.

@tpanzarella
Copy link
Member Author

@SteveMacenski There is a change I'd like to make to the last commit. Please hold-off on merging.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an update on the warning. I want it to be less scary looking.

ros2_ouster/src/ouster_driver.cpp Outdated Show resolved Hide resolved
… given

LiDAR scan will get the same ROS timestamp so they can be time correlated.
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this when you are.

@tpanzarella
Copy link
Member Author

I'm good.

@SteveMacenski SteveMacenski merged commit bcd032a into ros-drivers:eloquent-devel Apr 13, 2020
@SteveMacenski
Copy link
Member

Thanks! I also added you as a collaborator if you check your email linked to github. Welcome and thanks!

@tpanzarella
Copy link
Member Author

Thanks Steve. Happy to collaborate with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update drivers for multiple time sources
2 participants