baalexander / rosnodejs

A ROS client library using node.js
http://baalexander.github.com/rosnodejs/
MIT License
36 stars 8 forks source link

Push to multiple subscriber #30

Closed maxired closed 12 years ago

maxired commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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

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

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

maxired commented 12 years ago

Hi,

What's your plan about splitting protocols ?

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 ?