Backport rmw callbacks implementation to Humble [ros2-73] #157
Conversation
* Add support for listener callbacks. * Fix wrong debug assertion when converting DDS_Duration values * Clarify interactions between count_unread_samples() and take_next() * Notify on changed matched status Signed-off-by: Christopher Wecht <[email protected]> Signed-off-by: Andrea Sorbini <[email protected]> Signed-off-by: Taxo Rubio <[email protected]>
afa5055) Signed-off-by: Andrea Sorbini <[email protected]> Signed-off-by: Taxo Rubio <[email protected]>
There was a problem hiding this comment.
In general, this looks good to me when CI is green.
For running the CI, you'll need to do some slightly different things. In particular, you should use https://ci.ros2.org/job/ci_launcher/, which will output a snippet for you to paste into this PR. You will also need to make sure to run it with the correct arguments; in this case, it will have to be a ros2.repos file from Humble (with this PR in place for rmw_connextdds), you'll need to select "humble" as the ROS_DISTRO, you'll need to set the UBUNTU_DISTRO to jammy, and you'll need to set the RHEL_DISTRO to 8.
|
Thank you for the guide @clalancette! These jobs should be correct now. Note that I've removed RHEL as it doesn't build our rmw. Linux |
Signed-off-by: Taxo Rubio <[email protected]>
|
This fix should be enough I think. I don't think the Qt failures on Windows are related to this project. Could you confirm it, please? Linux |
Yeah, those Qt failures are separate. |
| std::mutex new_event_mutex_; | ||
| rmw_event_callback_t new_event_cb_[RMW_EVENT_INVALID] = {}; | ||
| const void * user_data_[RMW_EVENT_INVALID] = {}; | ||
| uint64_t unread_events_count_[RMW_EVENT_INVALID] = {0}; | ||
|
|
||
| bool triggered_inconsistent_topic{false}; | ||
|
|
||
| struct DDS_InconsistentTopicStatus status_inconsistent_topic; |
There was a problem hiding this comment.
There changes obviously break the ABI compatibility, are we allowed to do that in this particular time?
There was a problem hiding this comment.
There changes obviously break the ABI compatibility, are we allowed to do that in this particular time?
It's true that this breaks ABI. In this case, I approved it anyway because the ABI is between rmw_connextdds_common and rmw_connextdds; it seems doubtful someone is relying on that. Also, for some other RMWs we have broken ABI in this way before if it was "internal" to the RMW.
But it is a conversation worth having; should we allow this kind of ABI breakage in stable distributions?
There was a problem hiding this comment.
i think that is okay in this case to break the ABI, i do not expect there could be someone relying on rmw implementation APIs...
But it is a conversation worth having; should we allow this kind of ABI breakage in stable distributions?
IMO, i think rmw implementation can break the internal ABI in stable distributions. as long as RMW interface is compatible, there should be no problem? agree that this is worth to bring up for the discussion.
There was a problem hiding this comment.
Would you be ok with this change then? I prefer to be sure than to revert.
As I see it, this would only break compatibility from programs that directly call the Connext RMW API, which would be defeating the purpose of the RMW in the first place. The ABI of the rmw_connextdds and rmw packages is still stable, so it shouldn't be an issue.
There was a problem hiding this comment.
Would you be ok with this change then? I prefer to be sure than to revert.
Let's hold off until next week; we'll have a discussion in the ROS PMC meeting about it.
|
@mjcarroll Friendly ping in this issue. We decided to merge it. |
|
👨🌾 Thanks for pushing this one @trubio-rti, can this also be backported to Iron? EDIT: Can you help us on this one @fgallegosalido? |
|
friendly ping @trubio-rti @fgallegosalido |
|
Hi @Blast545. As Iron's EoL is close (shy of a month), we think it's not worthy investing resources into it. Is it really necessary to backport this issue to Iron? |
|
@fgallegosalido I brought this subject up on the ROS PMC meeting and the decision was to not make the forward port on this for Iron. I will close the related issue since it has been solved for Humble. Thanks for the work here ! |
This fixes #155. I've also sneakily backported the fix to allow Humble to build with Connext 7.3.0.
ci_linux:ci_linux_rhel:ci_windows: