Backport #1202 to melodic-devel: fix performance metrics #1203
Conversation
This reduces duplication of the version checking logic for performance metrics in gazebo. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
We are currently subscribing to the /gazebo/performance_metrics topic even if there are no subscribers to the ROS topic forwarding this data. The link_states and model_states topics currently use an advertise mechanism with callbacks when a subscriber connects or disconnects, so I've used that same pattern for the performance_metrics topic. This also helps workaround the deadlock documented in ros-simulation#1175 and gazebosim/gazebo-classic#2902. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
There was a problem hiding this comment.
The change LGTM, though I'm not sure if we care about maintaining ABI compatibility in this case. @chapulina ?
| @@ -376,6 +391,7 @@ class GazeboRosApiPlugin : public SystemPlugin | |||
| ros::Publisher pub_performance_metrics_; | |||
| int pub_link_states_connection_count_; | |||
| int pub_model_states_connection_count_; | |||
| int pub_performance_metrics_connection_count_; | |||
There was a problem hiding this comment.
Adding a new member is not ABI compatible.
There was a problem hiding this comment.
do we need to revert that change from noetic-devel too?
There was a problem hiding this comment.
I didn't think about it before. Noetic is an interesting case since it is the last ROS 1 release; maybe an exception can be made.
There was a problem hiding this comment.
In terms of the impact of an ABI break: because this is a plugin, I think breaking its ABI will only cause a problems for binaries of a plugin that links against GazeboRosApiPlugin. I don't know of any plugins that do this.
In terms of working around the ABI break: because this is a SystemPlugin, I think there will only ever be one instance open at a time. So I think we could safely make this a static variable. I'll check with @iche033 to see if he concurs.
There was a problem hiding this comment.
I made it a static variable in 043e5dd, which I think will work. I can port this fix to noetic-devel if merge it here
There was a problem hiding this comment.
We haven't been strict about ABI compatibility in ROS 1, right @j-rivero ?
Especially with this plugin, I don't think many users may be extending it, probably we shouldn't be installing the header... I wouldn't block on the ABI here...
I think there will only ever be one instance open at a time.
Yeah I think that's the usual use case for this plugin, even though I think technically it can be added more than once, but that could cause other issues with duplicate topics and services...
There was a problem hiding this comment.
so it sounds like we could merge this either way I think
There was a problem hiding this comment.
If we're okay with ABI breaks, it might be nice to keep the change the same as the one on noetic-devel for consistency.
I don't mind either way.
There was a problem hiding this comment.
ok, I'll revert 043e5dd to maintain consistency with noetic
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
This reverts commit 043e5dd. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters
force-pushed
the
fix_performance_metrics_melodic
branch
from
9d0e5ab to
15e4771
Compare
|
I think the CI test failures are unrelated. I'm going to merge |
Backport of #1202 to
melodic-devel:We are currently subscribing to the
/gazebo/performance_metricstopic even if there are no subscribers to the ROS topic forwarding this data. Thelink_statesandmodel_statestopics currently use an advertise mechanism with callbacks when a subscriber connects or disconnects, so I've used that same pattern for theperformance_metricstopic.This also helps workaround the deadlock documented in #1175 and gazebosim/gazebo-classic#2902.
I've also added the
GAZEBO_ROS_HAS_PERFORMANCE_METRICSmacro to de-duplicate the gazebo version checking logic and made some minor doc-string and spelling fixes.