ros-drivers / openni2_camera

ROS wrapper for openni 2.0
http://wiki.ros.org/openni2_camera
BSD 3-Clause "New" or "Revised" License
56 stars 96 forks source link

StringID instead of serialnumber? #100

Closed MatthijsBurgh closed 3 years ago

MatthijsBurgh commented 3 years ago

Is there any reason why the StringID of the device is used as serialnumber in the color/ir name instead of serialnumber? This breaks compared to openni, which does use the real serialnumber.

openni: https://github.com/ros-drivers/openni_camera/blob/indigo-devel/openni_camera/src/nodelets/driver.cpp#L142 openni2: https://github.com/ros-drivers/openni2_camera/blob/noetic-devel/openni2_camera/src/openni2_driver.cpp#L145

Because of this difference the camera_info_manager throws a warning: https://github.com/ros-perception/image_common/blob/noetic-devel/camera_info_manager/src/camera_info_manager.cpp#L288

Update: The implementation in openni2 produces names which are not unique. Multiple cameras of the same type get the same StringID, while the serial number is different of course. As these names are also used in calibration files, an unique name is needed. Therefore the solution of openni is the only correct one IMO.

MatthijsBurgh commented 3 years ago

@reinzor @warp1337 @awesomebytes what is your view on this issue?

awesomebytes commented 3 years ago

@MatthijsBurgh I haven't used openni_2 in a while, but what you say makes a lot of sense. The only thing I'd be worried by now is backwards compatibility. Maybe there is people that now depend on this behaviour.

Maybe there should be a parameter to actually use the serial number instead of the StringID and by default is false.

reinzor commented 3 years ago

I agree

MatthijsBurgh commented 3 years ago

I can do that. I think in the first new ros distro, we should make the serial the default again and the distro after it drop the option to choose.