Skip to content

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

Merged
maloel merged 23 commits into
realsenseai:ddsfrom
maloel:ddsros
Feb 26, 2023
Merged

dds-stream-sensor-bridge and on-demand streaming, with very basic ROS2 compatibility#11446
maloel merged 23 commits into
realsenseai:ddsfrom
maloel:ddsros

Conversation

@maloel

@maloel maloel commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

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
Comment thread third-party/realdds/src/dds-stream-sensor-bridge.cpp
Comment thread third-party/realdds/src/dds-stream-sensor-bridge.cpp Outdated
Comment thread third-party/realdds/include/realdds/dds-stream-base.h
Comment thread third-party/realdds/include/realdds/dds-stream-sensor-bridge.h Outdated
Comment thread third-party/realdds/src/dds-participant.cpp Outdated
Comment thread tools/dds/dds-server/lrs-device-controller.cpp
Comment thread tools/dds/dds-server/lrs-device-controller.cpp
Comment thread tools/dds/dds-server/lrs-device-controller.cpp
Comment thread tools/dds/dds-server/lrs-device-controller.cpp
@OhadMeir

Copy link
Copy Markdown
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.

Comment thread unit-tests/dds/test-stream-sensor-bridge.py
Comment thread unit-tests/dds/test-stream-sensor-bridge.py
Comment thread unit-tests/dds/test-stream-sensor-bridge.py

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Comment thread unit-tests/dds/test-stream-sensor-bridge.py
Comment thread third-party/realdds/src/dds-stream-server.cpp
Comment thread third-party/realdds/include/realdds/dds-device-server.h
Comment thread third-party/realdds/src/dds-stream-sensor-bridge.cpp
Comment thread third-party/realdds/src/dds-stream-sensor-bridge.cpp

@OhadMeir OhadMeir left a comment

Copy link
Copy Markdown
Contributor

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 realsenseai: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