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

(API improvement proposal) The code I would have like to write to move my turtle in turtlesim #21

Closed
temsa opened this issue Feb 28, 2012 · 7 comments

Comments

@temsa
Copy link
Contributor

temsa commented Feb 28, 2012

The original code is in #20

var ros = require('rosnodejs'),
    Velocity = ros.message('turtlesim/Velocity');  // if no callback is passed as a second argument, then run it in synchronous mode

// first argument can be an object like current api, but if a string, it will just be the name
ros.createNode('turtlenode' , function(err) {
  //'this' is a reference to the created node
  if(err)
    return console.error(err);

  //Velocity is still an object which is able to auto-instantiate if not an instance of the Velocity object yet.
  // we may want to add an optional callback below as a third argument, to know when it has been sent, but generally we just won't care at all, we could even listen for the event we just created instead

  this.emit('turtle1/command_velocity',  Velocity({ linear: 1.0, angular: 1.0 }));
})
.on('error', console.error)

// not in the code above, but would be powerful too
//creates the topic /turtlenode/hello and registers a callback for it
.on('turtlenode/hello', function(name) {console.log('hello, ', name)})
@temsa temsa mentioned this issue Feb 28, 2012
@baalexander
Copy link
Owner

Thanks for the API ideas. My thoughts:

I like the synch/asynch ros.message function. Are you thinking of caching these definitions so a later call won't have to process the message?

I am a little confused by the auto-instantiating of Velocity. Why this over a new call? And what type of logic is happening inside this call (I had assumed most of the hard work was happening in the ros.message call earlier).

I like the idea of interfacing with on() and emit(). Since you mentioned more event-based last week, it has been on my mind. In #19, I changed up where the event calls are happening. Instead of events happening on the node object directly, it's on a TCPROS (or UDPROS) socket. Calling publisher() and subscriber() is intended to get the XML-RPC master interaction out of the way as to decouple the master registration from TCPROS (and eventually UDPROS) implementation. The idea was similar to .of('namespace') in socket.io.

@temsa
Copy link
Contributor Author

temsa commented Feb 29, 2012

I'm not 100% sure about the real usage of a synchronous api of message, I guess we could use things like promises to make the user feel it's like synchronous, rather than being synchronous. Messages won't be used before a node has been created, so this shouldn't be a problem.

For example, I don't know any filesystem walker which is synchronous and is able to be stopped in it's walk to ignore certain sub-folders if it's parent contains some kind of file, and a walker is pretty much required to get message definitions and parse them. We can write it (have a look at walker code), it's not that much to write, but still it's to be done.

About auto-instantiating object, it's just practical and less verbose during usage, and JavaScript native objects like Array, Date, RegExp, etc. are already able to be used either as Date or new Date, with the same signification.
Walker, which I mentioned earlier for other purposes, uses also this pattern, you find it at the first line of the constructor : if (!(this instanceof Walker)) return new Walker(root), so you can call it with either new Walker('./') or simply Walker('./')

I'm not sure publishers and subscribers have to be explicit, the xml-rpc registration could simply be done at the first emit/listen, it doesn't have to impact the api, despite it may require to use promises (again) to achieve a very reliable way to do this (in order not to try to emit a message while not yet registered, but registration has been made).
I would advise to use futures promises library for such usages, as I've notably happily used it in my sandbox system

What I like with publishers/subscribers, is that it feels like a lower level api, and it brings more hackability. What I like with no publisher/subscriber, is that most of the time we don't really care how it is done, we just wan't to emit a message in the easiest way possible, so it's a higher level api.

I guess we can do both a high and low level api, the minimum is the low level, maybe a third party library can create the high level api, but if we want adoption, we need a high level api. So, the work may be to provide a low level yet simple api, and help as much as possible third party libraries to be developed and promoted on top of rosnodejs, or the work may be to create a high level api wih interconnexion points (such as events) to improve hackability so you can manage anything by yourself if you are not happy with what the library does by default (like registering to the xml-rpc lazily)

@baalexander
Copy link
Owner

Hi @temsa, I wanted to give you an update on things.

I've been getting a good amount of feedback from the ros.js team on an API that would be similar for rosnodejs and ros.js. There's still parts of the API we're waiting for consensus on (at which point, I'll post it in Issue #19 for additional input by you and any others).

But the reason I bring it up in this particular issue is I'm after your feedback on message type inclusions.

What do you think of:

ros.types([
  'geometry_msgs/Twist'
, 'people_tracker/ImagePosition'
, 'gun_srv/Gun'
], function(Twist, Pos, Gun) {
  // Rest of code goes in here
});

The idea is ros.types() (or whatever the function ends up being called) would take a single message type or an array of message types and return them in the callback after the classes have been generated.

The syntax feels similar to something like define() in require.js. The big advantage here is that you can generate and fetch all your message type classes in one call without several nested callbacks or some other async sequencing.

@temsa
Copy link
Contributor Author

temsa commented Mar 22, 2012

In the principle I'm pretty ok, it looks a lot like Asynchronous Module Definition ( CommonJS AMD ), btw I'd prefer something a bit more smooth, and refer this more explicitly as "message" rather than "types", even if type maybe more "technically correct".

All of this should work, IMHO :

ros.message([  // yes arrays can be very handy for handling a list of messages we may have already stored somewhere
  'geometry_msgs/Twist'
, 'people_tracker/ImagePosition'
, 'gun_srv/Gun'
], function(Twist, Pos, Gun) {
  // Rest of code goes in here
});
ros.message(  // no bracket, just check the type between Array / String / Function to know how to handle it
  'geometry_msgs/Twist'
, 'people_tracker/ImagePosition'
, 'gun_srv/Gun'
, function(Twist, Pos, Gun) {
  // Rest of code goes in here
});
ros.message( 'geometry_msgs/Twist', function(Twist) { //same as above, but with only one message
  // Rest of code goes in here
});

I guess we could make a nice fluid api as well, but I have no time to think of it right now, I'll try some proposal later, but this one seems already pretty ok to me

@temsa
Copy link
Contributor Author

temsa commented Mar 22, 2012

P.S. : @baalexander #23 should help implementing this API :)

@baalexander
Copy link
Owner

As mentioned in #19, new API is in master branch.

@temsa
Copy link
Contributor Author

temsa commented Apr 16, 2012

great ! :)

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

No branches or pull requests

2 participants