Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image/PointCloud Snapshot as a service #105

Open
wants to merge 7 commits into
base: jade-devel
Choose a base branch
from

Conversation

andreucm
Copy link

@andreucm andreucm commented May 22, 2017

Some applications, such as industrial manipulation, do not require image/point_cloud streaming. Instead, they need image/point_cloud as a service, upon a request of some other client node, thus networking load is reduced, allowing other hardware to work better (specially that involving controllers).

This Pull Request defines image and point cloud snapshots as services. Examples of packages using these services can be found at:

The Pull Request is against jade-devel branch because it is the latest one available, but the services have been tested on kinetic. However, it seems that they could work on jade and indigo.

bool dense_cloud

# exposure in milliseconds (0 indicates autoexposure)
uint32 exposure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really expect to be setting the exposure time for a cloud? This seems to be breaking several abstractions that you must know about the sensor's parameters and lighting situation to request a snapshot?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to let the exposure config through another mechanism like, for instance, dynamic reconfigure, so keep the request even more simple ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This standalone argument means nothing unless you know a lot more about the system. It has implications for side effects too, such as will this modify the system if it's already running with the exposure setting? What does the exposure setting mean if this snapshot is implemented with a data source such as a laser?

# REQUEST

# exposure in milliseconds (0 indicates autoexposure)
uint32 exposure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's the same issue here. Unless you're going to provide all the settings providing just one doesn't make much sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. Providing one parameter is quite arbitrary and not general. I can modify both srv with an std_msgs/Empty in the request side.
Another possibility could be to request with an std_msgs/UInt8 indicating the number of snapshots to take, and reply with a vector of either sensor_msgs/PointCloud2 or sensor_msgs/Image.
By the way, thanks for reviewing this PR!

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking this as changes requested so we know there's outstanding updates needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants