Skip to content

Conversation

chfritz
Copy link
Contributor

@chfritz chfritz commented Sep 16, 2016

Further unified the interfaces of on-the-fly and gennodejs.

See the updated example_turtle.js for the updated syntax. Basically, the specification of which messages and services to use is no longer necessary in the on-the-fly approach. We just generate all message and service definitions on start, which only takes about 500ms. After that they can be accessed just like the gennodejs generated ones using rosnodejs.require.

Fixes #14.

Also implemented uint64 and int64 support.

  • tested with rostopic; it works, with the usual caveat that
    javascript only uses 53bit precision, so really large numbers get
    "blurry". But the way I've implemented this it is stable, i.e., yes
    large numbers get blurry, but there is no singularity (which there
    could be at the 32bit boundary if one implemented the conversion
    from int64 to unit32 naively).

@chris-smith
Copy link
Collaborator

This is awesome!

I'm a little concerned about automatically generating all messages on the fly. It takes about 3.5 seconds for me to finish generating all the messages against our stack here (compared with ~150ms to pull in all the pre-generated messages - this is when we're actually loading all those files too which we typically don't). It's not really a large amount of time in the scheme of things, but I think devs here would start to get annoyed if every time they relaunch they have to wait 3.5seconds for ROS to find messages. Could we maybe add an init option to do this instead of making it happen automatically? rosnodejs.initNode('/my_node', {onTheFly: true}) or something.

My other worry is about 64 bit numbers. If we implement a "blurry" serialization/deserialization scheme behind the scenes then people won't really be able to unblur, will they? There is a very large range of numbers over which this won't be a problem but at some point I would expect someone to do a comparison that won't work as expected or to send an ID that gets deserialized into a different number and basically be hosed. I've generally opted to just accept and return 8 byte Buffers because of this - it definitely makes it more annoying in a lot of cases where you're not hitting precision limits but it also makes it possible to support the other cases. I'm not really eager to push some npm "big number" package on people either. I've been torn about the correct choice for this though...

@chfritz
Copy link
Contributor Author

chfritz commented Sep 21, 2016

3.5 seconds? wow, and I thought we had a lot of messages! :-) In that case, yeah, I'll definitely make it optional. I may also try to shave off a bit more time. Another option would be to cache the result, similar to what gennodejs provides, but without the need for a build process. But for now I'll just add the option.

Regarding int64s: I share your concern. This is a problematic issue for javascript as such. Either one uses a big number package (or creates ones own), or one needs to take good care never to get into large numbers. I believe this is the choice any javascript developer needs to be aware of and make. With that in mind I could imagine a middle ground solution: We could make it an option for users to require the bignum package, under that name, or not. If they do, and we see that bignum is defined (and looks right), then we return a bignum, otherwise we return a regular number. In the latter case, we could print a warning, or even throw an exception when the number is too large to be accurate.

My concern with using buffers is that it is quite tedious to use without any additional support. Most people won't want to learn about two's complement and little-endianness just to receive or send messages that use int64 fields (which a message developer may have picked a little too lightly).

So yeah, it seems that we agree that there is no silver bullet here. It's a matter of making a good compromise between protecting users from pitfalls they may be unaware off, and providing them with options on the accuracy vs. convenience spectrum.

@chris-smith
Copy link
Collaborator

We're actually currently on ROS indigo (hoping to move to kinetic soon) but still wanted to transition to rosnodejs and didn't want to pull all the message packages we rely on into our workspace so I adapted the existing on-the-fly stuff to generate code like gennodejs does into an indigo-devel branch - that code is currently here. I'm hoping to pull a cleaned up version of that branch over in the next week and then to integrate it with kinetic-devel as well (relies on some gennodejs stuff that's been hanging around a while and I may have broken some of the existing on-the-fly stuff so I need to fix that). The branch also includes some back end changes to improve master-slave communication, message queues, and a few other things.

I have a script in that branch (npm run generate or node tools/generate) that lets you optionally only generate certain message packages (and their dependencies) and specify an output directory for the results. Generating all the code for our stack takes about 4.5 seconds - we cache it where rosnodejs already looks for messages. I'm not entirely clear what you would be interested in as far as handling when to use the cache, clear the cache, etc - at that point it feels like a build step to me, albeit one that could be run outside a full catkin environment or without catkin_make.

Leaving buffers as-is for int64s is very tedious... I got rid of as many int64s in our stack as I could to not have to deal with them. I think your plan sounds reasonable though. Would you try to require('bignum') and then either use it or not depending on whether you were able to?

@chfritz
Copy link
Contributor Author

chfritz commented Sep 22, 2016

Very cool! Yes, I was mostly hoping to get by without catkin. I like the idea of using npm package scripts the way you do.

I was thinking that the getAll result could be cached to disk no matter whether it was explicitly called or as part of another program. That would mean that all you need is your program. First time it starts, it would fill the cache (generate all messages), but the second time you start it will just read it from disk. I suppose one might need to do a make-like check of file dates to see whether the cache needs updating. I hadn't thought that far.

Re. int64 and bignum: oh yes, that's even better. I was thinking we would expect users to provide the package in a specific global variable, but require-ing it ourselves in a try-catch seems like a much better option. I'll do that.

@chfritz chfritz force-pushed the unify_interface_further branch from 21f3068 to 03ff929 Compare September 22, 2016 04:11
@chfritz
Copy link
Contributor Author

chfritz commented Sep 22, 2016

ok, done.

@chfritz chfritz force-pushed the unify_interface_further branch from 61b200a to bd22b99 Compare September 24, 2016 23:47
@chris-smith
Copy link
Collaborator

Looks good! Can you just rebase against kinetic-devel? Happy to keep the action client and unification commits separate but the first commit was merged in #31

Implemented uint64 and int64 support

Now generating all messages and services on the fly on start (if requested)

- message.getAll works and is very fast (takes less than a second)

- Fixes RethinkRobotics-opensource#14.

Corrected implemention for [U]Int64 in on-the-fly approach

Using bignum for [u]int64 if available, otherwise usual javascript
numbers are used but a warning is printed if numbers are sent or
received that are outside of the precise range of JS for integers (>=
1e15).
- Added getActionClient to Rosnodejs (previously the action client wasn't exported in any way)

- ActionClient: added "cancel" function
@chfritz chfritz force-pushed the unify_interface_further branch from bd22b99 to 1a22dd6 Compare September 26, 2016 03:13
@chfritz
Copy link
Contributor Author

chfritz commented Sep 26, 2016

good catch! Done.

@chris-smith chris-smith merged commit ee5b7b1 into RethinkRobotics-opensource:kinetic-devel Sep 27, 2016
@chfritz chfritz deleted the unify_interface_further branch September 27, 2016 04:16
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