-
Notifications
You must be signed in to change notification settings - Fork 190
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
base: jade-devel
Are you sure you want to change the base?
Conversation
…e both, the image and camera info. CMakeLists modified accordingly
bool dense_cloud | ||
|
||
# exposure in milliseconds (0 indicates autoexposure) | ||
uint32 exposure |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this 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.
Adding advanced snapshot
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.