search

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
136 stars 79 forks source link

Compiler warnings and Rule of Zero on SensorInterface #25

Closed tpanzarella closed 4 years ago

tpanzarella commented 4 years ago

I was getting some ugly compiler warnings building the code (I'm using g++ 9.2.1 on Ubuntu 18.04). This was as a result of having empty implementations for the declared methods on the SensorInterface. I think the intent was to define an ABT / interface and to that end I made those functions pure virtual. There was also an issue with a control flow path that did not provide a return value, so I fixed that too.

In addition to the warnings, while in the SensorInterface I saw that we necessarily have to declare the dtor virtual, and so to be in compliance to the Rule of Zero, I added explicit implementations for the other four special functions -- namely those related to copy and move semantics -- per guidance in the C++ Core Guidelines. More info and references in the commit messages.

tpanzarella commented 4 years ago

No problem Steve, thanks for getting an open-source ROS2 interface up for the Ouster LIDARs.

I will likely be working quite extensively with the Ousters in ROS2 and to that end will have several features, optimizations, etc. that I would like to make. Of course, I'd like to upstream them back to you so that the community as a whole may benefit -- my expectations are that the features we (Box Robotics) would like to see in the Ouster interface are not unique to us. With that said, how would you like that managed? Do you simply prefer a PR with the implementation ready to go? File an issue on the issue tracker to facilitate discussion of the feature and proposed implementation? Something else? Let me know your thoughts.

FWIW, I've read your design document, so I understand your your vision for the architecture as it is presented there. Also, in terms of h/w access, I currently have an OS1-16 with the "regular" beam spacing configuration (the one that most closely resembles the Puck). We will be looking at other configurations in the coming weeks/months as well.

SteveMacenski commented 4 years ago

Do you simply prefer a PR with the implementation ready to go?

For smaller things or items that are clear "this is necessary", I think that's fine. For larger items, I think filing a ticket for discussion would be good. I'd rather have that discussion before you get too far into your work to make sure its the right way to go.

I have a OS1-64 but I may be getting a OS-0 at some point to help support that in these drivers as well.