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

dds-stream-sensor-bridge and on-demand streaming, with very basic ROS2 compatibility #11446

Merged
merged 23 commits into from
Feb 26, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Feb 16, 2023

The goal was:

  • Have ROS see our streams
  • Have rs-dds-server automatically start streaming when a stream is opened, while allowing maximum flexibility when then trying to stream other streams
  • Keep the viewer working as before

Currently:

  • ROS sees our streams, but can only understand the format of the RGB stream

Tracked on [LRS-385]

@maloel maloel requested a review from OhadMeir February 16, 2023 14:06
@maloel maloel marked this pull request as draft February 16, 2023 14:06
@OhadMeir
Copy link
Contributor

I still don't completely understand the bridge, and lots of the names are not clear enough.
There are public methods like add_implicit_profiles and commit that even with the comments I don't feel like I understand when to call them and if it is needed to call them at all.
Aren't stream_bridge::is_explicit and stream_bridge::is_implicit opposites?

I kind of feel like the bridge should be dds_sensor_based_device_sensor inheriting from dds_device_sensor, and maybe even in the application level not realdds.


void dds_stream_sensor_bridge::init( std::vector< std::shared_ptr< dds_stream_server > > const & streams )
{
if( streams.empty() )
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@OhadMeir
Copy link
Contributor

I don't have any more comments at the moment.

I still don't like the name dds_stream_sensor_bridge and the naming of some of it's methods but I don't have better names.

@maloel maloel marked this pull request as ready for review February 23, 2023 09:32
@maloel maloel requested a review from OhadMeir February 23, 2023 10:20
Copy link
Contributor

@OhadMeir OhadMeir left a 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

@maloel maloel merged commit 14d33e7 into IntelRealSense:dds Feb 26, 2023
@maloel maloel deleted the ddsros branch February 26, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants