Skip to content

Allow bidirectional traffic #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Oct 31, 2016

Added support for bidirectional topics.

I'm basing this on #3, but if you only want these changes, it should not be difficult to cherry-pick them.

@xqms
Copy link
Member

xqms commented Oct 31, 2016

Interesting feature. Could you please separate this PR from #3? I am still a bit unsure about #3, but I have no conceptual problems with this feature.

Copy link
Member

@xqms xqms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some minor coding style comments. We should keep things consistent ;-)

@@ -114,6 +119,9 @@ TCPSender::TCPSender()
#endif
}

if (m_nh.hasParam("ignored_publishers"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indent using tabs.

}
}

this->send(topic, flags, event.getMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please get rid of this->.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it in the new PR. Just curious, why is it better?

void send(const std::string& topic, int flags, const topic_tools::ShapeShifter::ConstPtr& shifter);
void send(const std::string& topic, int flags, topic_tools::ShapeShifter::ConstPtr shifter,
const bool reconnect = true);
void messageCallback(const std::string& topic, int flags,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent using tabs.

@peci1
Copy link
Contributor Author

peci1 commented Oct 31, 2016

Yes, of course. It's just gonna take some time, since now we're hurrying up on end-user evaluation meeting :) I hope to get back to it in a week or so.

I had one more idea, but am not sure if I want to do that. It would be a kind of "autoconfiguration". There would be a local ros parameter, to which all receivers would store their nodename (in an array). The sender could then read this parameter and use it as the ignore list. I'm almost sure this is not the nicest solution (prone to concurrency issues), just the first one I came up with. If you have a better idea, please share it.

@xqms
Copy link
Member

xqms commented Oct 31, 2016

Yes, of course. It's just gonna take some time, since now we're hurrying up on end-user evaluation meeting :) I hope to get back to it in a week or so.

No problem. I'm pretty busy with other stuff right now as well ;-)

to which all receivers would store their nodename (in an array).

I'm not sure about that. In our setups, we have many machines in a relay-like position (receiving from one machine, sending to another machine). In that case we need the receiver-sender connection.

@peci1
Copy link
Contributor Author

peci1 commented Oct 31, 2016

Well, it could be constrained only to publisher-receiver pairs communicating with the same host. But even then I'm not sure it's the way I'd take. I just want to simplify the config, because now it has several parameters that are not that easy to fill in (e.g. you can't use anonymous nodes for senders).

@peci1
Copy link
Contributor Author

peci1 commented Nov 7, 2016

Superseded by #5 .

@peci1 peci1 closed this Nov 7, 2016
@peci1 peci1 deleted the allow_bidirectional_traffic branch November 7, 2016 16:16
xqms added a commit that referenced this pull request Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants