Open osrf-migration opened 8 years ago
Original comment by nampi (Bitbucket: nampi).
Here is a brief summary of a discussion:
#!c++
bool WaitForSubscribers(const std::string &_topic, const int _timeout = -1);
* A [branch](https://osrf-migration.github.io/ignition-gh-pages/#!/ignitionrobotics/ign-transport/branches/compare/wait_for_subscribers%0Ddefault#diff) to start with
Original comment by nampi (Bitbucket: nampi).
@caguero
std::find(advTopics.begin(), advTopics.end(), topic) == advTopics.end()
? Is is better than advTopics.find(topic) != advTopics.end()
?bool TopicWatcher::Blocked() const
?Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).
My thoughts:
std::vector
are you sure that you can use advTopics.find()
method? I don't see it in the API.Original comment by nampi (Bitbucket: nampi).
std::vector<std::string> AdvertisedTopics() const;
but std::unordered_set<std::string> &TopicsAdvertised() const;
. Why do you use different data structure for this function?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.
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;
}
}
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.
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?
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.
1)
#!c++
/// \class TopicWatcherPrivate TopicWatcher.cc
/// ignition/transport/TopicWatcher.cc
/// \brief Private data for TopicWatcher class.
class TopicWatcherPrivate
2)
#!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.
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?
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.
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;
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?
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.
Original report (archived issue) by Nate Koenig (Bitbucket: Nathan Koenig).
Pull request #130 introduced a
sleep
here.The sleep should be replace with a
WaitForSubscribers
call that is under development.