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

QoS Issues with rmw_connextdds #16

Closed
cwecht opened this issue Mar 15, 2023 · 2 comments
Closed

QoS Issues with rmw_connextdds #16

cwecht opened this issue Mar 15, 2023 · 2 comments

Comments

@cwecht
Copy link

cwecht commented Mar 15, 2023

If I use EventsExecutors with rmw_connextdds and these chagnes (ros2/rmw_connextdds#76), which enable event listeners, I get many warnings regarding incompatibly QoS. For example, if I execute the events execturos tests, I get these warnings for each test case:

grafik

I have no idea whether this is an issue of the events executor or these changes to rmw_connextdds. But I suppose that it makes sense if both sides would have a look at this issue.

@mauropasse
Copy link
Collaborator

mauropasse commented Mar 15, 2023

I think this is the same issue we had both on rmw_cyclonedds and rmw_fastdds, that we were storing a single "event callback" for all the possible listener callbacks. So first a callback is set for "new message" events, but then it is overridden with the callback for an "incompatible QoS". Thus, when you get a new message, the callback for "incompatible QoS" is wrongly called.
This is happening on rmw_connextdds, see here.
Connext should have an array of callbacks for each listener event type (new message, new QoS event, etc).
See what rmw_fastdds has:

https://github.com/ros2/rmw_fastrtps/blob/926b3a1472fc5ba74972ca8384b054c94571b27c/rmw_fastrtps_shared_cpp/src/custom_subscriber_info.cpp#L278

And rmw_cyclonedds:

https://github.com/ros2/rmw_cyclonedds/blob/76572ebf1032f359ed302d7017471a295738fa1e/rmw_cyclonedds_cpp/src/rmw_node.cpp#L641

@cwecht
Copy link
Author

cwecht commented Mar 16, 2023

@mauropasse Thank you very much for the quick response! I was able to adapt rmw_connextdds acordingly.

@cwecht cwecht closed this as completed Mar 16, 2023
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

No branches or pull requests

2 participants