Skip to content
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

Push to multiple subscriber #30

Merged
merged 2 commits into from
May 16, 2012
Merged

Conversation

maxired
Copy link
Contributor

@maxired 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.

@baalexander
Copy link
Owner

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.

@baalexander
Copy link
Owner

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.

@maxired
Copy link
Contributor Author

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.

@baalexander
Copy link
Owner

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.

@maxired
Copy link
Contributor Author

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

@baalexander
Copy link
Owner

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.

@baalexander
Copy link
Owner

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.

@maxired
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish to multiple subscribers
2 participants