move notification out of device; more generic; uses json #11042

maloel · 2022-10-30T06:04:10Z

There's still a uid but it's no longer transmitted with profiles. To be removed in another PR.
Notifications are now topics::notification (no longer inside device::).
Notifications now primarily have a data_type and version, nothing else. The data can be in the format specified in the data_type (which currently supports CUSTOM, JSON, and CBOR). The data is bound at 4096 bytes (from what I saw in the code, it was at 100 before, but that obviously wasn't a limit -- this needs more looking into). Testing shows that, for a L515, the color stream has the most profiles (114) and this comes out to 6741 in raw json or 5034 in CBOR.
Notifications now communicate using JSON. This code mostly resides in the device and device-server. Should be a lot more flexible.

Additionally:

guard against device-server::init() failures
add L515 stream-format
move expected stream number out of device-impl (only used in init())
notification-server no longer starts automatically; on-discovery notifications can only be added when stopped
- I actually saw this happen: we saw the reader before all on-discovery messages were defined, so only some of them got sent and the rest got lost
change device destruction in device-watcher to another thread
- This was another weird bug that still needs to be understood
- I got hangups when exiting python for some reason, and traced it to device destruction
- A remote participant crashed (ctrl+c, on purpose) and, when dropping in the current process, the device writer first get removed. This triggers the callback in the watcher and, since it's the only holder of the device, the device gets destroyed...
- Placing the destruction in a separate thread seems to alleviate this
- Tried using a dispatcher to do this, which didn't work for some weird reason...

…be added only when stopped

Nir-Az · 2022-10-30T07:12:44Z

There's still a uid but it's no longer transmitted with profiles. To be removed in another PR. Notifications are now topics::notification (no longer inside device::). Notifications now primarily have a data_type and version, nothing else. The data can be in the format specified in the data_type (which currently supports CUSTOM, JSON, and CBOR). The data is bound at 4096 bytes (from what I saw in the code, it was at 100 before, but that obviously wasn't a limit -- this needs more looking into). Testing shows that, for a L515, the color stream has the most profiles (114) and this comes out to 6741 in raw json or 5034 in CBOR. Notifications now communicate using JSON. This code mostly resides in the device and device-server. Should be a lot more flexible.

Additionally:

guard against device-server::init() failures

add L515 stream-format

move expected stream number out of device-impl (only used in init())

notification-server no longer starts automatically; on-discovery notifications can only be added when stopped

I actually saw this happen: we saw the reader before all on-discovery messages were defined, so only some of them got sent and the rest got lost

change device destruction in device-watcher to another thread

This was another weird bug that still needs to be understood

I got hangups when exiting python for some reason, and traced it to device destruction

A remote participant crashed (ctrl+c, on purpose) and, when dropping in the current process, the device writer first get removed. This triggers the callback in the watcher and, since it's the only holder of the device, the device gets destroyed...

Placing the destruction in a separate thread seems to alleviate this

Tried using a dispatcher to do this, which didn't work for some weird reason...

About the data side, I saw a d4xx device with more than 200 profiles,
Not sure I understand you say the previous notification size was 100 bytes? it was a dynamic size and the max profiles was 400 IMO no? or I misunderstood your comment.

So you say you have currently 5034/6741 but the limit is 4096?

third-party/json_fwd.hpp

third-party/realdds/include/realdds/topics/notification/notification-msg.h

OhadMeir · 2022-10-31T12:56:40Z

third-party/realdds/include/realdds/dds-device-server.h


 // Forward declaration
 namespace topics {
+class notification;


Can you explain when is a topic "raw"?
Why is device_info raw but notification not?

This is the non-raw notification you're looking at. There's a raw::notification, too

OhadMeir · 2022-10-31T13:04:56Z

third-party/realdds/include/realdds/topics/notification/notification.idl

    {
        module raw
        {
-            module device


Notification topic name does not include the device name? e.g. /realsense/D455/notification?

Topic name? Not sure what you mean. This is a general-purpose notification topic. What does it have to do with a device?

OhadMeir · 2022-10-31T13:11:31Z

third-party/realdds/include/realdds/topics/notification/notification-msg.h

-    static void construct_raw_message( msg_type msg_id, const T& msg, raw::device::notification& raw_msg )
+    // Get the custom data with casting to the desired type
+    // Syntax: auto stream_info = msg.get< STREAM_INFO >();
+    //         if ( msg ) do something with it;


Not sure I understand. What is get? Shouldn't it be
// Syntax: auto stream_info = msg.custom_data< STREAM_INFO >();
// if ( stream_info ) do something with it;

Copy paste issue - get was renamed to custom_data. I'll update.

OhadMeir · 2022-10-31T13:12:48Z

third-party/realdds/include/realdds/topics/notification/notification-msg.h

-    using type = raw::device::notificationPubSubType;
+    using type = raw::notificationPubSubType;
+
+    enum class data_type


I think data_formatting will be a better name

It's the type of the data... data_format is OK I guess but why?

Sounds better to me :-)
Data type might mean other stuff as well. Format just means "the way in which something is arranged or set out."
Also when looking JSON and CBOR, wikipedia specifies they are data formats

OhadMeir · 2022-10-31T13:24:00Z

third-party/realdds/include/realdds/dds-notification-server.h

    ~dds_notification_server();

+    // By default we're not started, to avoid on-discovery before all discovery messages have been collected
+    void start();


In other classes it is called run()

OK, will change.

OhadMeir · 2022-10-31T13:31:53Z

third-party/realdds/src/dds-device-impl.cpp

+                                   "more streams than expected (" + std::to_string( n_streams_expected )
+                                       + ") received" );
+                    auto type = utilities::json::get< std::string >( j, "type" );
+                    auto stream_name = utilities::json::get< std::string >( j, "name" );


Are the messages defined somewhere, like a readme? Can we pass it to Yoram's group so they will know what we expect?

Sure, eventually. :)

Just making sure, this PR still keeps the profiles transfer works server --> client right?

What other option is there? Everything is always server --> client...

I mean that the functionality still works? like before this PR?

Unit-tests pass. Functionality is still there. Notifications are sent, processed, etc. and streams are created on the client.
I don't understand the question, I guess.

OhadMeir · 2022-10-31T16:41:52Z

third-party/realdds/src/dds-device-server.cpp

+        profiles += sp->to_json();
+    json msg = {
+        { "id", "stream-header" },
+        { "type", stream->profiles().front()->type_to_string() },


To support rs2 features we will need to add a dds_stream_xxx type system and a conversion back to rs2_stream+index (like we convert dds_stream_format to and from rs2_format)

This will be able to replace the changes I made in my PR ( #11045, you did add some comments about it)

Will also need some factory to create all the different stream types

Once both our PRs are in, I'll add all the other classes

OhadMeir · 2022-10-31T16:50:02Z

third-party/realdds/src/dds-device-impl.cpp

+                    auto & stream = _streams[stream_name];
+                    if( stream )
+                        DDS_THROW( runtime_error, "stream '" + stream_name + "' already exists" );
+                    stream = std::make_shared< dds_video_stream >( stream_name, sensor_name );


Hard to see in this UI
Where is dds_motion_stream created?

This was a bug. Once we have it working we need to add some unit-tests with motion streams...

Added a test-case.
Moved profile type_to_string() into stream-base type_string().
Added type-check: when creating a stream in device-impl, it checks that its type is now the same as that given in the json.

…e_string()

maloel added 12 commits October 28, 2022 10:32

moved json util functions into utilities/json.h

2629630

fix bug in topics/readme

db1539a

added third-party/json_fwd.hpp

71a80d0

fix initialization bug in sensor-base

6a18ed5

added stream-profile::to_json()

80b70b3

move notification out of device; more generic; uses json

d89b175

guard against device-server::init() failures

9de6858

add L515 stream-format

c612aba

move expected stream number out of device-impl

e0850ef

notification-server no longer starts automatically; on-discovery can …

957b23e

…be added only when stopped

change device destruction in device-watcher to another thread

94df05c

added naming functions to dds-participant python

dacc0f8

maloel requested a review from OhadMeir October 30, 2022 06:04

fix license statement in json_fws.hpp

eaf6047

Nir-Az reviewed Oct 30, 2022

View reviewed changes

third-party/json_fwd.hpp Show resolved Hide resolved

Nir-Az reviewed Oct 30, 2022

View reviewed changes

third-party/realdds/include/realdds/topics/notification/notification-msg.h Show resolved Hide resolved

fix linux compilation issue with json +=

7aacdd9

OhadMeir reviewed Oct 31, 2022

View reviewed changes

maloel added 3 commits November 1, 2022 11:03

CR: data_format, run(), move profile::type_to_string() -> stream::typ…

7bc4028

…e_string()

attempt at fixing linux ambiguous call compilation error

623eb3e

another attempt at fixing linux ambiguous call compilation error

84b7f96

OhadMeir approved these changes Nov 3, 2022

View reviewed changes

maloel merged commit 792e4df into realsenseai:dds Nov 3, 2022

move notification out of device; more generic; uses json #11042

move notification out of device; more generic; uses json #11042

Uh oh!

Conversation

maloel commented Oct 30, 2022

Uh oh!

Nir-Az commented Oct 30, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OhadMeir Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maloel Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OhadMeir Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OhadMeir Oct 31, 2022 •

edited

Loading

maloel Nov 1, 2022 •

edited

Loading

OhadMeir Oct 31, 2022 •

edited

Loading