-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
dds-stream-sensor-bridge and on-demand streaming, with very basic ROS2 compatibility #11446
Conversation
I still don't completely understand the bridge, and lots of the names are not clear enough. I kind of feel like the bridge should be |
|
||
void dds_stream_sensor_bridge::init( std::vector< std::shared_ptr< dds_stream_server > > const & streams ) | ||
{ | ||
if( streams.empty() ) |
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.
We should add this test to dds_device_server
too. No use of a device without streams.
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.
I think we have a test (see test-device-init.py
) where this is a valid scenario.
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.
Then we should reconsider if it is valid. Not a must for this PR, but a point to consider, is there a point in a device without any streams.
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.
How about a DFU device? I can see situations where a device is strictly for the control/notification mechanism. I agree it's then not strictly a "device" but we'll only know later if this makes sense or not.
I don't have any more comments at the moment. I still don't like the name |
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.
I am OK with this PR. If we will want to rename things we can do it some other time
The goal was:
rs-dds-server
automatically start streaming when a stream is opened, while allowing maximum flexibility when then trying to stream other streamsCurrently:
Tracked on [LRS-385]