search

zivid / zivid-ros

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

Separation of message definitions #40

Open dave992 opened 3 years ago

dave992 commented 3 years ago

Would it be possible to separate the Zivid message definitions in a zivid_camera_msgs package, i.e. as done in https://github.com/ensenso/ros_driver? This would be very helpful as this allows for only building the relevant (sub)packages instead of the complete driver and subsequently installing all of its dependencies.

This is especially of interest if you want to communicate with the driver from a node running on a separate system, as well as from a node running in a docker container. In these cases, it does not make sense to build the driver and install all dependencies on a system that will not actually run the driver, but instead only communicates with the system running the Zivid camera driver.

Please let me know if I can assist in this by submitting a PR with these proposed changes.

apartridge commented 3 years ago

Hi @dave992, thanks for reporting this issue.

As of now, we don't have any custom Zivid message definitions, we are only using the sensor_msgs package. However, I suppose we could have an "empty" zivid_camera_msgs, which depends on sensor_msgs. We can then add potential new custom messages in the future into this package. Is this your suggestion? We do have some custom .srv files, do you think these also should be in a zivid_camera_msgs?

If so, this sounds like a reasonable change to me, and we would absolutely be open to adding this change, as long as we do it in a way that does not break the API. In other words, it should be possible to build the Zivid ROS driver as before, without any changes to client code or setup.

It would be very useful if you assisted a PR for the proposed changes. However, due to being busy on release of a new product, we may not be able to follow up on that PR in the short term. We would try to take it in the next time we do more development on the ROS driver (not exacly sure when that would be).

dave992 commented 3 years ago

This would indeed now only be for the service message definition files (.srv). Future custom messages (.msg) or action (.action) definitions could also then be added to the *_msgs package. The message definitions themselves do not have many dependencies, so separating them allows us to use them independently from the driver.

If so, this sounds like a reasonable change to me, and we would absolutely be open to adding this change, as long as we do it in a way that does not break the API. In other words, it should be possible to build the Zivid ROS driver as before, without any changes to client code or setup.

Future code should import the messages differently, i.e. change: C++: #include <zivid_camera/Capture.h>, Python: from zivid_camera.srv import Capture to: C++: #include <zivid_camera_msgs/Capture.h>, Python: from zivid_camera_msgs.srv import Capture

To be backward compatible I think it should be possible to have aliases that point to the new package names for these messages for the near future (and add a deprecated warning if you do not want to support the alliases anymore on the long term).

It would be very useful if you assisted a PR for the proposed changes.

Sure, I will start working on it and make a PR for it. Just wanted to make sure that the work I put into this will not end up being stuck in PR limbo.

apartridge commented 3 years ago

Thanks @dave992. As I mentioned we will not be able to follow up on the PR on the short term. But this sounds like something that is pretty small, so we can take it in the next time we do changes to the ROS driver (which will not be for some months).

nirwester commented 2 years ago

Hi, another thought on the topic: what about using std service definitions where possible? For example using http://docs.ros.org/en/api/std_srvs/html/srv/Empty.html Instead of Capture or http://docs.ros.org/en/api/std_srvs/html/srv/Trigger.html for others. In this way the packages interfacing with the camera wouldn't need to depend on the package.

apartridge commented 2 weeks ago

Hi. In the ROS2 preview branch we have moved the srv files to a separate zivid_interfaces package, and we have used std_srvs/srv/Trigger for the Capture requests. Let us know if this makes sense and solves this issue for you. https://github.com/zivid/zivid-ros/tree/ros2-preview