WaitForSubscribers in ign.cc about gz-transport

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

Here is a brief summary of a discussion:

• A potential signature:

#!c++

bool WaitForSubscribers(const std::string &_topic, const int _timeout = -1);

• A branch to start with

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

@caguero

1. Why do you use this method: std::find(advTopics.begin(), advTopics.end(), topic) == advTopics.end()? Is is better than advTopics.find(topic) != advTopics.end()?
2. Why do you add this function bool TopicWatcher::Blocked() const?

[osrf-migration]

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

---

My thoughts:

1. Being advTopics an std::vector are you sure that you can use advTopics.find() method? I don't see it in the API.
2. When defining API functions for a class we need to thing about how the users of the class are going to use the functionality and create the appropriate functions. If we build an example, it is possible that it does not use all the functions in the APi. This does not mean that we should remove them.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

1. Thanks. I didn't notice that std::vector<std::string> AdvertisedTopics() const; but std::unordered_set<std::string> &TopicsAdvertised() const;. Why do you use different data structure for this function?

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

1. Uhmm. I think this is a mistake. For some reason, TopicsAdvertised(), TopicsSubscribed() and SrvsAdvertised() are duplicated. We should keep only one version of them. Probably in a separate pull request. Good catch.

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

2 . I don't quite remember the specific use case for Blocked() but it doesn't look to me like a crazy function. You can use it to check from a different thread if a topicWatcher is blocked. Keep it or not, it's your call.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

3 . There is a new block in example/publisher.cc:

#!c++

if (!node.WaitForSubscribers(topic, 10000))
{
  std::cerr << "No subscribers available" << std::endl;
  return 0;
}

I cannot use CTRL-C to interrupt publisher. I tried to use the same logic as in ReqHandler.hh to realize the functionality of WaitForSubscribers function. I can interrupt requester with CTRL-C. Could you give any recommendations how can I debug this problem, please? My current code is here. It is far from final version, but it works on my examples.

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

• set assignee_account_id to "557058:6509b6f8-6263-4504-9cee-d12b08b0b4e8"
• set assignee to "nampi (Bitbucket: nampi)"

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

• set component to "gsoc16"

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

3 . This is expected because we're capturing the CTRL-C in our signal handler and the only thing we're doing is setting a boolean variable g_terminatePub to true. This variable is only used in the while loop that sends messages every second. Our WaitForSubscribers() call is executed before the while loop, therefore changing the boolean variable doesn't have any effect.

You could modify the code for something like this:

#!c++

int counter = 0;
while (!node.WaitForSubscribers(topic, 1000))
{
  if (g_terminatePub)
    return 0;

  if (++counter == 10)
  {
    std::cerr << "No subscribers available" << std::endl;
    return 0;
  }
}

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

3 . Ok. I'm writing tests now. I prepared some tests, but I am not confident that they covered all cases.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

pr.

3 . I changed the place of signal handlers in publisher.cc. It is shorter in my opinion.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

4 . Why are some classes declared with IGNITION_TRANSPORT_VISIBLE? For instance, why TopicStorage is IGNITION_TRANSPORT_VISIBLE, but HandlerStorage not?

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

HandlerStorage should have IGNITION_TRANSPORT_VISIBLE, it's a bug. Good catch. This is for declaring the symbol visibility in the library.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

5 . I don't understand what to write in the \class section for documentation if I move TopicWatcherPrivate class inside TopicWatcher.cc file.

#!c++

 /// \class TopicWatcherPrivate TopicWatcher.cc
 /// ignition/transport/TopicWatcher.cc
 /// \brief Private data for TopicWatcher class.
 class TopicWatcherPrivate

#!c++

 /// \class TopicWatcherPrivate TopicWatcher.hh
 /// ignition/transport/TopicWatcher.hh
 /// \brief Private data for TopicWatcher class.
 class TopicWatcherPrivate

I find this template \class <name> [<header-file>] [<header-name>] and I think that the second case is right.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

Quote (@caguero) : So, after the 2.0.0 release, if your patch is ABI compatible you should target it to ign-transport2, otherwise to default.

6 . What is the right branch for WaitForSubscribers issue?

7 . How can I check is my patch is ABI compatible or not?

[osrf-migration]

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

---

5: yes the second one seems to be like the corret option.

6: as a general rule when implementing new features you should target to the latest released branch (in this case ign-transport2). Depending on the changes that you need to make in the code, if your code breaks the API/ABI (see 7) then you can not use the latest released branch and the pull request should be pointed to default branch (which will become the ignition transport 3 release at some moment in the future).

7: Theory: this kde page explains well what you can do and what you can not do in order to respect the ABI. For checking, you give a look at running the ABI complicance checker. For helping our developers and get rid of having to setup locally the ABI checker we have an automatic job in our building farm that can be used to check if two branches are ABI compatibles. Mail me if you want to use it and I will send you credentials.

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

8 . As I wrote before I should update the data structure of TopicWatcherStorage. What do you think about it?
std::map<std::pair<topic, topic type>, std::map<node uuid, std::vector<watcher uuid> > > data;

data type:
std::map<std::pair<std::string, std::string>, std::map<std::string, std::vector<std::string> > > data;

[osrf-migration]

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).

---

8: I feel that the TopicWatcher class should contain some members and accessors about the topic and type that is watching. Then, the data structure to store all watchers might look like:

std::map<std::string, std::map<std::string, std::vector<TopicWatcher>>>

The first string could be the topic and the second string could be the node UUID. What do you think?

[osrf-migration]

Original comment by nampi (Bitbucket: nampi).

---

I have a problem with TopicWatcherStorage::RemoveWatchersForTopic() function. I don't understand how to delete topic watcher.

#!c++

      /// \brief Remove all the watchers for the topic.
      /// \param[in] _topic Topic name.
      /// \param[in] _typeName Message type.
      /// \return True when at least one watcher was removed or false otherwise.
      public: bool RemoveWatchersForTopic(const std::string &_topic,
        const std::string &_typeName)
      {
        size_t count = 0;
        if (this->data.find(_topic) != this->data.end())
        {
          WatchersPerNode_M watchers = this->data.at(_topic);
          for (auto &node : watchers)
          {
            for (auto &watcher : node.second)
            {
              std::shared_ptr<TopicWatcher> watcherPtr = watcher;
              if (watcherPtr) {
                if (watcherPtr->TopicType() == _typeName) {
                  //TODO: delete topic watcher
                  //this->data[_topic][node].erase(watcherPtr);
                }
              }
              else
                std::cerr << "Topic watcher is NULL" << std::endl;
            }
          }
        }

        return count > 0;
      }

I create a new_watchers structure, but I can change this part of code.