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

Already on GitHub? Sign in to your account

Push to multiple subscriber #30

Merged
merged 2 commits into from May 16, 2012

Conversation

Projects
None yet
2 participants
Contributor

maxired commented Apr 30, 2012

Hi,

This is not perfect, but it is a good start to fix #28
I identified an issue when publishing to multiples subscribers : we start a new connection to all nodes in the publisherUpdate message, even those to which we are already connected.
Otherwise, everything seems to work.

I will propably improve it, but I was thinking that you might want this version.

Owner

baalexander commented Apr 30, 2012

Thanks @maxired. I'll try taking a more detailed look tonight or Wednesday night and merge the changes in.

It's failing an automatic merge right now, but I assume it's because I moved the function order around in topic.js in master.

Owner

baalexander commented May 3, 2012

I merged the changes plus a few more into the multiple clients branch.

I need to make some changes before merging into master, including:

  • Update the unregister functions (unsubscribe and unpublish) to work with multiple clients
  • Split this.protocols into subscribers[] and publishers (I think this will help with function that want just subscribers or just publishers)
  • Get publish() and subscribe() to work without calling registerPublisher() or registerSubscriber() before hand.

I'm hoping to have time to work on this Saturday or Sunday.

Contributor

maxired commented May 3, 2012

Hi,

What's your plan about splitting protocols ?

  • A double reference for topic both subscriber and publisher ?
  • Split TCPROS with subscribing and publishing parts ?

About the third point, I'm not sure to understand.
As you can see, registerPublisher and registerSubscriber methods are called directly by the topic himself.
You don't need to call them in an application using rosnodejs.
Going back to the previous model where the registering is done when we publish, as explained before, would results in some messages lost.

Owner

baalexander commented May 3, 2012

I don't have a final plan on splitting up protocols yet. I was thinking about splitting up Topic's this.protocols['TCPROS'] to have a publishers array and a subscribers array.

As for the third point, I think I can make it work so no messages would be lost on publish. My idea shouldn't remove your option, though, of being able to register as a publisher or subscriber before calling publish() or subscribe().

Both of my points are a little vague because I still need to play around with the code and investigate more.

Contributor

maxired commented May 3, 2012

Thanks for this information.

After I re-read the code, I've seen that in TCPROS, about the we still use this.socket in the subscribing parts.
(The fact is that it we can received message from multiple nodes because the object is not garbage collected while the socket is connected)

That's mean that also inside TCPROS we have to split the sockets between publishers and subscribers.
It could be done like this 48f099810c45aacb20a805d2ac14adcbf3498c03 (Carreful : ugly and not tested code, just to give an idea about what's should propably be done)

Moreover, I'm not sure if we should firstly make some choice for the design, or begin to implements Services

Owner

baalexander commented May 6, 2012

I realized where a source of some of our confusion with the multi client stuff was and tried to fix it with the latest commits to the multiple_clients branch.

Basically, instead of having some multi logic in Topic and some multi logic in TCPROS, I put all the multi client handling logic in Topic. Each instance of TCPROS is meant to be a publisher to a single socket or a subscriber to a single socket. Topic will create instances of TCPROS as needed for each additional subscriber or publisher.

The code still needs work, but it should give an idea of what's happening.

Owner

baalexander commented May 6, 2012

As for services, I definitely agree this needs to get done. Issue #7 is dedicated to services implementation. There needs to be more work in defining the services API, but things like parsing the srv file could be done sooner. I left comments in the issue.

Contributor

maxired commented May 6, 2012

Hi agree with you that with your last work , everything seems simpler.
Nevertheless, I had a thought about it, and I don't like the fact that we create a server for each TCPROS publisher.
It's for this reason that I previously chose to mutualize connection to subscribers in the same TCPROS.

What do you think about that ?
Should we try to mutualized the server for multiple TCPROS ?

@baalexander baalexander merged commit af26a76 into baalexander:master May 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment