-
Notifications
You must be signed in to change notification settings - Fork 42
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
Support Events #103
Support Events #103
Conversation
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
…not implemented Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
* Nest TopicQoSMap within TopicDataMap Signed-off-by: Yadunund <[email protected]> * Count matched pub/subs based on qos compatibility Signed-off-by: Yadunund <[email protected]> * Rename TopicDataMap to TopicTypeMap Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
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 did a first pass review. I didn't review the changes to the GraphCache in any detail; I'll do that next.
My biggest concern has to do with the class hierarchy we are introducing here. I've left a more detailed comment inline.
Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Yadu <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
…ager objects Signed-off-by: Yadunund <[email protected]>
Support selective QoS based events
@clalancette i've overhauled this PR and updated the description as well. Keeping track of messages lost is the only major event yet to be supported but I think this PR is getting way too big. I will work on that once this is reviewed and merged as it may also require changes from your GID PRs. |
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
}; | ||
|
||
/// Helper value to indicate the maximum index of events supported. | ||
#define ZENOH_EVENT_ID_MAX rmw_zenoh_event_type_t::ZENOH_EVENT_PUBLICATION_MATCHED |
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.
This is pretty minor, but our typical style here is to add a "SENTINEL" value at the end of the enum, and check against that instead. So this would change to:
}; | |
/// Helper value to indicate the maximum index of events supported. | |
#define ZENOH_EVENT_ID_MAX rmw_zenoh_event_type_t::ZENOH_EVENT_PUBLICATION_MATCHED | |
ZENOH_EVENT_INVALID | |
}; |
And then the checks in event.cpp
would change to >= ZENOH_EVENT_INVALID
.
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 see. If we adopt this approach, the array lengths of these members would now be ZENOH_EVENT_INVALID
, eg std::condition_variable * event_conditions_[ZENOH_EVENT_INVALID]{nullptr}
which presents a readability problem imo. If you think it's valid, I'm happy to make the change as suggested.
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@clalancette this is ready for another review. |
Signed-off-by: Yadunund <[email protected]>
The PR adds support for two independent event mechanisms
EventsExecutor
.MatchedEvent
. Support selective QoS based events #118How events function in rmw
rmw
layer when a new message/request/response arrives. Then, the client library can callrmw_event_take()
to get the event instead of waiting on condition variables inrmw_wait()
.rmf/events.h
. Herermw_publisher_event_init
andrmw_subscription_event_init
methods will instantiate the event based on theevent_type
supplied. The RMW implementation has full discretion to initialize/reject (by returning anullptr
) an event initialization request based on what events the implementation supports. The rmw_events_type_t definition enumerates the types of events that can be supported while an internalevents_map
data structure defines which events are supported byrmw_zenoh
. Once an event is initialized,rcl
will callrmw_event_set_callback()
to register a callback for the event initialized previously. Then inrmw_wait
, the RMW implementation should attach a condition variable to the event. The RMW implementation's job is to then track changes to the ROS Graph that are relevant to the pub/sub entities created in the current session and for which the callbacks have been registered. When a change is detected, an "Event instance" is to be added to an "event queue" and the attached condition variable is used to notifyrmw_wait
that an event has occurred. Finally, upon notification,rcl
will invokermw_take
to take ownership of the event.Changes
This PR makes a big change to the internal data structures used to store publisher, subscription, service, and client data. All of these classes now inherit anEventsBase
class. While this approach was discussed earlier, it was apparent during the implementation that casting these different classes to the sameEventsBase
class is necessary as the rmw_events_type_t definition bundles events for publishers and subscriptions. It also minimizes the code duplication when implementingrmw_take
and while setting/triggering callbacks.This PR adds several new features and fixes bugs while making some big changes:
GraphCache
for local pub/subs.liveliness::Entity
entriesstruct TopicStats
is removed to simplify nested data structures.rclcpp
but atleast the consequence is now properly handled).liveliness.cpp
. Enums are used to keep track of segments in the liveliness tokens.RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE
: We support it but this will never be triggered since there are no QoS incompatibilities in zenoh.RMW_EVENT_SUBSCRIPTION_MATCHED
: A match is reported when a sub is added to the graph and there are other pubs with the same topic and type. QoS does not need to be checked for same reasons as above.RMW_EVENT_OFFERED_QOS_INCOMPATIBLE
: Supported but never triggered as stated above.RMW_EVENT_PUBLICATION_MATCHED
: A match is reported when a pub is added to the graph and there are other subs with the same topic and type. QoS does not need to be checked for same reasons as above.TRANSIENT_LOCAL
pub andVOLATILE
sub durabilities. All other combinations areCOMPATIBLE
to reflect zenoh protocol's behavior.zenoh session id
,unique id of the entity within the session
andthe unique id of the node
. If the entity is a node, this id is appended twice to keep the number of segments consistent. This allowszenoh session id
,node id
andentity id
.talker
nodes into the samerclcpp_components::ComponentManger
EventsBase
🎉EventsManager
class. (There are no events defined for services and clients inrmw/events.h
.DataCallbackManager
class. (Publishers do not need to trigger any callbacks).Testing
DataCallback tests: This functionality is utilized by the
experimental::EventsEcecutor
inrclcpp
.EventsExecutor
support can be tested by running any of the demo executables fromdemo_nodes_cpp
. This PR is helpful for building the executables to run theEventsExecutor
.Matched events
Follow instructions to run
matched_event_detect
fromdemo_nodes_py
: https://github.com/ros2/demos/tree/rolling/demo_nodes_cpp#matched-event-detectNodes with same names from the same zenoh session
Save this text into a python file, source ROS 2, and execute it
In a second terminal run
ros2 node list
. You should be able to see two/my_node
nodes. Without the changes here, this would not be the case as theGraphCache
was only capable of handling a singe node name from the same zenoh session.$ ros2 node list WARNING: Be aware that there are nodes in the graph that share an exact name, which can have unintended side effects. /my_node /my_node
Incompatible QoS
The nodes in
ros2/demos
are not relevant here since Zenoh does not have the same QoS behaviors as DDS and hence the logic in the executables does not apply.Future work
Some other takeaways:
rmw
API should really provide two separate enums,rmw_publisher_events_type_t
andrmw_subscription_events_type_t
as the current API does not enforce the fact that events registered for publishers should only be of the "publisher types" and vice versa for subscriptions.