search

zivid / zivid-ros

Official ROS driver for Zivid 3D cameras
BSD 3-Clause "New" or "Revised" License
55 stars 43 forks source link

Add support for loading capture settings via YML file #59

Closed danielcranston closed 2 years ago

danielcranston commented 2 years ago

Hello, thanks for making this repository! Here's a PR adding some functionality I thought could be useful.

Summary

This PR adds the ability to load and apply Zivid Settings via a .yml file, both at startup (via a rosparam) and at runtime (via a service).

This probably addresses https://github.com/zivid/zivid-ros/issues/33.

Risks

  1. No tests added for this functionality.
  2. Requires a full path to the settings file.
  3. Only tested on Zivid Two. I can't say for sure this works as intended on Zivid One since I don't have access to that hardware.
  4. Currently, failure to load the .yml file at startup is not an error (will not throw), while failure when calling the service is. Maybe you have some preferences on this, please let me know :+1:
apartridge commented 2 years ago

Thanks @danielcranston. Zivid can take over this PR and complete the missing features, if that is OK for you? In addition to the things you mention, we should also add support for 2D-Settings.

Are you currently using the rosparam way of specifying Settings during startup of the node? I wonder if we should only support the service-call way, as it is a bit simpler, easier to test, and we avoid two ways of doing the same.

apartridge commented 2 years ago

Currently, failure to load the .yml file at startup is not an error (will not throw), while failure when calling the service is. Maybe you have some preferences on this, please let me know

It would be preferable that it did throw an error during startup if the provided .yml file was not loadable.

danielcranston commented 2 years ago

Hi @apartridge,

Zivid can take over this PR and complete the missing features, if that is OK for you?

Yeah that's fine with me, thanks for following up!

Are you currently using the rosparam way of specifying Settings during startup of the node? I wonder if we should only support the service-call way, as it is a bit simpler, easier to test, and we avoid two ways of doing the same.

I am, but your argument for removing it makes sense so I'm not against it :+1:

apartridge commented 2 years ago

Thanks @danielcranston - I have started on a new branch for this here: https://github.com/zivid/zivid-ros/tree/add_support_for_loading_settings_from_file. I will hopefully make a PR for this early next week.

apartridge commented 2 years ago

Made a separate PR here: https://github.com/zivid/zivid-ros/pull/60. Feel free to review it and test it @danielcranston Closing this PR