search

ros-drivers / pointgrey_camera_driver

ROS driver for Pt. Grey cameras, based on the official FlyCapture2 SDK.
128 stars 180 forks source link

Move the messages somewhere else? #4

Closed mikepurvis closed 10 years ago

mikepurvis commented 10 years ago

None of the three msg packages bundled in this repo really belong here—

image_exposure_msgs statistics_msgs wfov_camera_msgs

To put these in sensor_msgs, though, they should undergo a formal review beforehand, so it's probably not realistic for Indigo. Then again, maybe a new image_msgs or camera_msgs repo would be a better container for them, in which case they could go there now, and be reviewed for J-Turtle.

@chadrockey, @tfoote: Thoughts on this?

chadrockey commented 10 years ago

These are extra messages that contain non-standard information for part of the DARPA LS3 project application. I don't believe they are useful to the entire general community, so they most likely don't belong in common_msgs.

I would say either leave them if you think the information from the camera can be useful (I left the standard topics in place to use the normal ROS tools) or just remove these messages and remove that part of the ROS wrapper, it won't affect the underlying library.

mikepurvis commented 10 years ago

I think the wfov images are the most interesting and worthwhile:

https://github.com/ros-drivers/pointgrey_camera_driver/blob/master/wfov_camera_msgs/msg/WFOVImage.msg https://github.com/ros-drivers/pointgrey_camera_driver/blob/master/wfov_camera_msgs/msg/WFOVCompressedImage.msg

It'd be great if image_view could display these directly, but that's a bit of a tough sell if they're particular to the pointgrey driver. Of course, an alternative approach to these wrappers could be to simply supply the auxiliary data in a meta topic with matching header timestamps, and leave it to the recipient to subscribe to both and match them up.

chadrockey commented 10 years ago

Yeah, those do have useful and common information. Unfortunately, I can't go into detail beyond the existing comments as the original work was ITAR.

For this project, because synchronizing messages is a huge pain, I just wrapped sensor_msgs/Image with the new information. If you did add this, it would be 3 message streams that need synchronized: Image, CameraInfo, CameraMetaInfo. It will also be difficult to push through review since there is a lot of different meta information beyond what's in this messages that could be published for cameras.

We never did find a great way to have side-streams of data in ROS and changing the messages would trigger a terrible migration, so I feel chained to the legacy implementation here.

If you do feel strongly that this is worthwhile data, adding a sensor_msgs/CameraMetaInfo would be my recommendation. It would be required to modify ImageTransport to support the synchronization so that users won't have to struggle with the synchronization.

@jack-oquin, have you had any requests for messages containing actual shutter duration, gain, white balance, and so on in camera1394?

mikepurvis commented 10 years ago

I haven't tried to use it myself, but how bad is message_filters/TimeSynchronizer for dealing with this type of issue?

jack-oquin commented 10 years ago

@jack-oquin, have you had any requests for messages containing actual shutter duration, gain, white balance, and so on in camera1394?

Not that I recall. There are no open issues for anything like that.

tfoote commented 10 years ago

With the ability to depend on any released package I'd like to see more people using the message and let it iterate on its own for a while before considering it a common message.

mikepurvis commented 10 years ago

Sounds good, thanks all.