Support image transport publishers in nodelet lazy #75

efernandez · 2018-04-22T23:56:42Z

No description provided.

efernandez · 2018-04-22T23:57:09Z

efernandez · 2018-04-23T00:00:23Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

-      for (size_t i = 0; i < publishers_.size(); i++)
+      BOOST_FOREACH (const ros::Publisher& pub, publishers_)
+      {
+        if (pub.getNumSubscribers() > 0)


This code is the same for both types of subscribers.
I can factorize it into a templated method to avoid code duplication. I didn't do it b/c I think this way the intention and changes are cleaner in the diff. I can do that in another PR.

efernandez · 2018-04-23T00:02:50Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

+            const std::string& topic, int queue_size, bool latch=false)
+  {
+    boost::mutex::scoped_lock lock(connection_mutex_);
+    image_transport::SubscriberStatusCallback connect_cb


I'm reusing the connect_cb callback for connection and disconnection.

I've changed the other advertise method to do the same and also remove the _1 placeholder.
Note that the connectionCallback argument isn't used, and it's actually shadowed in the code (in the for loop).

mikaelarguedas · 2018-04-27T17:16:28Z

Thanks @efernandez
I'm a bit hesitant about introducing a dependency on image_transport in nodelet (not sure how @dirk-thomas feels about it ?).

I will try to go over this PR soon and give feedback.

As this breaks API / ABI, it will need to be released in Melodic or ROS-N but not in the already released distributions. At which point it may make sense to use c++11/14 features rather extending our use of boost.

efernandez · 2018-04-27T18:11:37Z

@mikaelarguedas I can give it another thought to use some template magic and avoid breaking API / ABI, but at first sight looks hard to me, or at least it'd be too verbose.
And after all, I'm happy if this gets into Melodic. I targeted indigo-devel branch here b/c it looks to me you're using that branch for >= Indigo. If you create melodic-devel I'd target it with a new PR and close this one.

Re. C++11/14, I'd be happy to contribute another PR with that, but I wouldn't do it in this one.

Re. image_transport dependency, I tried to inherit from this class and extend it for Image Transport publishers, but then I need another callback because the current one is taking a ros::Publisher that doesn't use and is also hidden by a local variable.
And I think even in that case I need additional changes to avoid duplicating all the code in the callback to include the Image Transport publishers (in particular https://github.com/ros/nodelet_core/blob/indigo-devel/nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h#L168, so it's only checked after checking there's no subscriber for all publishers, of both types).
And I still need to make the ros::Publisher templated advertise method use the overridden callback from the child class, which means it should be virtual here in the base class.
IMHO unless there's a strong reason to avoid the dependency on image_transport, I think this would be cleaner. Either way, I'm open to suggestions for an alternative implementation, like splitting the current callback for dis/connect.

dirk-thomas · 2018-04-27T18:17:01Z

unless there's a strong reason to avoid the dependency on image_transport, I think this would be cleaner

I haven't looked too much into the details of the API and this patch. But in my opinion the question is not if there is "a strong reason to avoid the dependency". The question is why should nodelet_topic_tools contains image_transport specific API? image_transport is just one custom thing - the same would potentially apply to others. I would argue that the API should be designed that it is possible to do what you are aiming for outside of this package. So image transport should be able to be implemented ontop rather then require to be integrated into nodelets.

If certain information is currently not exposed or additional API is needed that can certainly be changed / added in nodelet to accommodate the requirements.

efernandez · 2018-04-27T18:45:45Z

Thanks @dirk-thomas . I'll try to see how far I can go, b/c the current implementation is specific for ros::Publisher and ros::NodeHandle.

@mikaelarguedas Are you ok with me doing changes to this PR so it allows to extend NodeletLazy to support different types of publishers, and not only ros::Publisher?

I've just sent another commit the makes the connectCallback publisher-agnostic. It adds this helper templated method that would check all publishers before doing any action into the state of the object: https://github.com/ros/nodelet_core/pull/75/files#diff-dd9335fec549f588f8a69073e5e1f970R264
The idea would be to allow to overload this method or another one that calls it, from the children classes. But well, I still need to spend more time on this.

Do you mind pointing to things you'll be willing to accept from my changes as a separate PR, meanwhile?

efernandez · 2018-04-27T21:46:56Z

@mikaelarguedas @dirk-thomas I've updated the PR so now it doesn't depend on image_transport.

efernandez · 2018-04-27T21:48:19Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

@@ -57,6 +58,25 @@ enum ConnectionStatus
  SUBSCRIBED
 };

+/**


This is a helper function, that can also be used in the child class.

efernandez · 2018-04-27T21:49:34Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

@@ -135,7 +155,7 @@ class NodeletLazy: public nodelet::Nodelet
  /** @brief
    * callback function which is called when new subscriber come
    */
-  virtual void connectionCallback(const ros::SingleSubscriberPublisher& pub)


This argument isn't used. It's not even in the doxygen documentation.

And I had to remove it because otherwise I can use it with an image_transport::Publisher callback.

efernandez

I left a few comments to help with the review.

efernandez · 2018-04-27T21:51:53Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

@@ -144,28 +164,23 @@ class NodeletLazy: public nodelet::Nodelet
    if (lazy_)
    {
      boost::mutex::scoped_lock lock(connection_mutex_);
-      for (size_t i = 0; i < publishers_.size(); i++)
+      if (hasSubscribers())


This is a very small and cosmetic change, but the diff is large because of one if block getting into the method. I recommend hiding whitespace changes.

Now this method can be overridden by the child class to also check for the image_transport::Publishers.

efernandez · 2018-04-27T21:53:07Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

        }
      }
-      if (connection_status_ == SUBSCRIBED)
+      else if (connection_status_ == SUBSCRIBED)


The rest remains the same, but I changed the return; inside the if block to and if-else block, which is equivalent and easier to read in my opinion. If you don't like it, I can remove this from the PR.

efernandez · 2018-04-27T21:54:05Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

@@ -217,22 +232,29 @@ class NodeletLazy: public nodelet::Nodelet
    */
  template<class T> ros::Publisher
  advertise(ros::NodeHandle& nh,
-            std::string topic, int queue_size, bool latch=false)
+            const std::string& topic, int queue_size, bool latch=false)


The change to const & is in a separate commit, so if you don't like it I can remove it from this PR, but we shouldn't be passing a std::string by copy.

efernandez · 2018-04-27T21:55:39Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

-      = boost::bind(&NodeletLazy::connectionCallback, this, _1);
-    ros::SubscriberStatusCallback disconnect_cb
-      = boost::bind(&NodeletLazy::connectionCallback, this, _1);
+      = boost::bind(&NodeletLazy::connectionCallback, this);


Note that I remove the _1 placeholder here, which is important after the change in connectionCallback.

The change to use connect_cb for both disconnection and connection is optional, but I think it's cleaner, since this was already declaring disconnect_cb the same way as connect_cb.

efernandez · 2018-04-27T21:59:38Z

nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h

+   * @brief Checkes if at least one publisher has subscribers.
+   * @return True if at least one publisher has subscribers; False otherwise.
+   */
+  virtual bool hasSubscribers() const


A child class can overload this to support other type of publishers with something like this: return NodeletLazy::hasSubscribers() || nodelet_topic_tools::has_subscribers(image_transport_publishers_);

@dirk-thomas Any idea where can I contribute the child class that I have to support both ros::Publisher and image_transport::Publisher? For now I have it on a private repo.
Maybe image_transport, but then it'd have to depend on nodelet_topic_tools, which doesn't sound good.

Maybe image_transport

Sounds like the best option to me.

@vrabaud Could you comment on this?

Long story short, I've done some changes here so it's possible to extend the nodelet_topic_tools::NodeLazy class to support both ros::Publisher and image_transport::Publisher.

Now I'm looking for a repo where I can contribute the derived class that provides that functionality. If you think image_transport is a good place, where in there could it be? Maybe a new package, b/c this will need a dependency on nodelet_topic_tools.

efernandez · 2018-04-27T22:03:55Z

I think the test for bionic is failing on something unrelated with this PR:

14:46:29 Traceback (most recent call last):
14:46:29   File "/usr/lib/python3/dist-packages/apt/cache.py", line 218, in __getitem__
14:46:29     return self._weakref[key]
14:46:29   File "/usr/lib/python3.6/weakref.py", line 137, in __getitem__
14:46:29     o = self.data[key]()
14:46:29 KeyError: 'ros-melodic-pluginlib'

If you don't consider the change on connectionCallback could impact existing code, it'd be great if we could get this in lunar, not only in melodic. It seems to me that only the NodeletLazy uses that method.

efernandez · 2018-05-03T17:30:49Z

@mikaelarguedas What do you think of my latest changes?

I've just rebased on top of indigo-devel so the test is kicked again (no new changes).

Add hasSubscribers and remove not-needed argument from connectionCallback, so we can overload the hasSubscriber check and support image_transport::Publisher. Also share connect and disconnect cb.

efernandez · 2018-09-13T21:13:03Z

ping

efernandez commented Apr 23, 2018

View reviewed changes

efernandez force-pushed the support_image_transport_publishers_in_nodelet_lazy branch from f8b1db2 to 894ccfd Compare April 23, 2018 00:29

efernandez force-pushed the support_image_transport_publishers_in_nodelet_lazy branch from d4e8ed7 to 2938455 Compare April 27, 2018 21:43

efernandez commented Apr 27, 2018

View reviewed changes

efernandez force-pushed the support_image_transport_publishers_in_nodelet_lazy branch from 2938455 to d1ff6a7 Compare May 3, 2018 17:33

efernandez mentioned this pull request May 22, 2018

Support image transport publishers in nodelet lazy clearpathrobotics/nodelet_core#1

Merged

efernandez added 2 commits May 24, 2018 18:30

Allow to extend w/ support for image_transport

b13b6e5

Add hasSubscribers and remove not-needed argument from connectionCallback, so we can overload the hasSubscriber check and support image_transport::Publisher. Also share connect and disconnect cb.

Use const reference for std::string

52a66c6

efernandez force-pushed the support_image_transport_publishers_in_nodelet_lazy branch from d1ff6a7 to 52a66c6 Compare May 24, 2018 22:30

furushchev mentioned this pull request Feb 2, 2024

image_transport: add nodelet_lazy for image_transport ros-perception/image_common#72

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support image transport publishers in nodelet lazy #75

Support image transport publishers in nodelet lazy #75

efernandez commented Apr 22, 2018

efernandez commented Apr 22, 2018

efernandez Apr 23, 2018

efernandez Apr 23, 2018 •

edited

Loading

mikaelarguedas commented Apr 27, 2018

efernandez commented Apr 27, 2018

dirk-thomas commented Apr 27, 2018

efernandez commented Apr 27, 2018

efernandez commented Apr 27, 2018

efernandez Apr 27, 2018

efernandez Apr 27, 2018

efernandez left a comment

efernandez Apr 27, 2018

efernandez Apr 27, 2018

efernandez Apr 27, 2018

efernandez Apr 27, 2018

efernandez Apr 27, 2018

dirk-thomas Apr 27, 2018

efernandez Apr 27, 2018

efernandez commented Apr 27, 2018

efernandez commented May 3, 2018 •

edited

Loading

efernandez commented Sep 13, 2018

Support image transport publishers in nodelet lazy #75

Are you sure you want to change the base?

Support image transport publishers in nodelet lazy #75

Conversation

efernandez commented Apr 22, 2018

efernandez commented Apr 22, 2018

Choose a reason for hiding this comment

efernandez Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

mikaelarguedas commented Apr 27, 2018

efernandez commented Apr 27, 2018

dirk-thomas commented Apr 27, 2018

efernandez commented Apr 27, 2018

efernandez commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efernandez commented Apr 27, 2018

efernandez commented May 3, 2018 • edited Loading

efernandez commented Sep 13, 2018

efernandez Apr 23, 2018 •

edited

Loading

efernandez commented May 3, 2018 •

edited

Loading