Fix for dynamic reconfigure ns clash. Closes #13

[piyushk]

I'm floating this non-backwards compatible fix for #13. This is the cleanest solution, and feels right to me. It will change the namespace of the dynamic reconfigure server for anyone using this driver.

If you'd like a backwards compatible fix, I can initialize server inside the constructor after checking a flag as to whether it should be launched using the private nodehandle. Let me know. Since this package is always built by source, I wasn't sure if it was maintaining API compatibility yet.

[mintar]

I finally found time to debug this. It took me a while to figure out what's going on, so I'm documenting it here for posterity.

These are the ROS params when using the launch file from this package:

before this PR

pico_flexx: 
  exposure_mode: 1
  exposure_mode_stream2: 1
  exposure_time: 27
  exposure_time_stream2: 1000
  max_noise: 0.07
  range_factor: 2.0
  use_case: 0
pico_flexx_driver: 
  automatic_exposure: true
  base_name: pico_flexx
  base_name_tf: pico_flexx
  exposure_time: 1000
  max_noise: 0.07
  queue_size: 5
  range_factor: 2.0
  sensor: ""
  use_case: 0

after this PR

pico_flexx_driver: 
  automatic_exposure: true
  base_name: pico_flexx
  base_name_tf: pico_flexx
  exposure_mode: 0
  exposure_mode_stream2: 0
  exposure_time: 1000
  exposure_time_stream2: 1000
  max_noise: 0.07
  queue_size: 5
  range_factor: 2.0
  sensor: ""
  use_case: 0

The bug that this PR fixes only happens when running the driver as a nodelet (as in the launch file). When running as a node instead, the dynamic_reconfigure server correctly gets the private node handle, since that's the default:

https://github.com/ros/dynamic_reconfigure/blob/17e1d4bf6a538d2def097a7f70b139ccd41e9f78/include/dynamic_reconfigure/server.h#L75

However, you can't do this in a nodelet; you'll have to use getPrivateNodeHandle() instead. That's why you have to explicitly pass the private node handle, which is exactly what this PR does.

Since the behavior before this fix was

• clearly a bug
• inconsistent between running it as a node or nodelet
• contrary to what the documented launch file was supposed to do,

... I don't think anyone relied on the buggy behavior before, so we can merge it as it is.

[mintar]

Thanks @piyushk !

[mintar]

Since this package is always built by source, I wasn't sure if it was maintaining API compatibility yet.

In general, we're trying to maintain API compatibility. The only reason this package is not released into binaries is because we rely on the proprietary libroyale, which we can't legally distribute with the package.

[piyushk]

Sounds good. Thanks Martin!

…

On Nov 24, 2017 10:56 AM, "Martin Günther" ***@***.***> wrote: Since this package is always built by source, I wasn't sure if it was maintaining API compatibility yet. In general, we're trying to maintain API compatibility. The only reason this package is not released into binaries is because we rely on the proprietary libroyale, which we can't legally distribute with the package. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABm7BxlUUH7SSyeakS71-fUApdOT_MYeks5s5ub1gaJpZM4QlMgs> .