ros-drivers / ros2_ouster_drivers

ROS2 Drivers for the Ouster OS-0, OS-1, and OS-2 Lidars
https://ouster.com/
Apache License 2.0
139 stars 79 forks source link

Adding a Tins-based driver implementation #77

Closed MattSYoung closed 3 years ago

MattSYoung commented 3 years ago

General Changes

TinsDriver

SteveMacenski commented 3 years ago

Let me know when you want me to review this again (also you have a conflict)

MattSYoung commented 3 years ago

@SteveMacenski I'm ready for you to review the PR again.

It is feature complete, however there are still some issues with the output rate of the /points topic depending on the throughput/replay rate of the incoming packets. This is mostly an issue with the high-rate OS0-128 that I have access to, and may not be an issue at all for other Ouster models. In most cases this simply means the output rate of the /points topic will be as low as 3.5 Hz instead of the target 10 or 20 Hz.

Since this is fundamentally an issue with the replay rate/throughput of the user's ethernet device, and not the Tins based driver, I would rather address this problem separately in issue #78.

SteveMacenski commented 3 years ago

Did you compile with release flags? that's often a quick fix of such issues others have reported.

SteveMacenski commented 3 years ago

Looks like you have some new merge conflicts. I'm doing another pass through the code, but so far the only thing is the linting in the non-client files

SteveMacenski commented 3 years ago

Once the conflicts / linting is done, happy to merge it!

MattSYoung commented 3 years ago

Ok, after navigating the minefield that is git rebase, I think we're good to go. I will run one last QA/bug check tomorrow and then let you know it's ready to merge.