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

updating 142 to add gennodejs as kinetic ros_core pkg #113

Merged
merged 2 commits into from
Apr 28, 2016

Conversation

IanTheEngineer
Copy link
Contributor

This adds genjs as a core ROS package for Kinetic:
https://github.com/RethinkRobotics-opensource/genjs

This will be used with rosjs Javascript ROS client library:
https://github.com/RethinkRobotics-opensource/rosjs

@dirk-thomas
Copy link
Member

Thanks for making the PR. I just sent an email to ros-users about the update of the REP: http://lists.ros.org/pipermail/ros-users/2016-April/069977.html

@syrnick
Copy link

syrnick commented Apr 21, 2016

I have 2 orthogonal comments.

  1. (minor) I don't know what's the policy for adding very new libs to the core. genjs seems to be pretty new. Can it work w/o being in the core?
  2. If rosjs targets nodejs exclusively, It'll probably be better to have nodejs in the package names so that it's clear for someone looking for a javascript client what's the scope of the library.

@gavanderhoorn
Copy link
Contributor

@syrnick wrote:

2 . If rosjs targets nodejs exclusively, It'll probably be better to have nodejs in the package names so that it's clear for someone looking for a javascript client what's the scope of the library.

I had a similar feeling: what is the relation to robot web tools for instance (is there even one)? If this is for nodejs, that should ideally be made more clear.

@rctoris
Copy link

rctoris commented Apr 21, 2016

I agree with the above. It looks like this implementation of rosjs is a native client which communicates directly with the roscore via XML RPC. For current users of roslibjs (which also is nodejs compatible) the distinction may be unclear and users might assume some compatibility when there isn't any.

@chris-smith
Copy link

(minor) I don't know what's the policy for adding very new libs to the core. genjs seems to be pretty new. Can it work w/o being in the core?

It works without being in core, but it requires users to pull message packages provided by ros into their workspace if they want to use them or depend on them.

If rosjs targets nodejs exclusively, It'll probably be better to have nodejs in the package names so that it's clear for someone looking for a javascript client what's the scope of the library.

@IanTheEngineer and I talked about this a bit actually. It is currently targeted at nodejs, but it should be possible to support running in the browser at some point. We didn't want to preclude that possibility by naming this so specifically now.

@syrnick
Copy link

syrnick commented Apr 21, 2016

I think focus on nodejs support is a good thing. The environment is pretty controlled and predictable, it's easy to spin up for tests, you have clean package management for node. Adding browser support later on is a bit complicated to say the least. With node-only you also have a path for binding to C++ ROS interfaces to manage the network communications. Javascript is sometimes fragile due to CPU locking.

@rctoris
Copy link

rctoris commented Apr 21, 2016

Agreed with @syrnick. nodejs only gives you the benefit of communicating directly with roscore via native bindings. Adding browser support would require some kind of browser-compatible network communication (e.g., WebSockets) for which rosbridge/roslibjs focused on over many iterations and feedback cycles. I see the projects as very separate use cases.

@gavanderhoorn
Copy link
Contributor

Yeah, the point of @syrnick's comment nr 2 (and my +1) was not that the generator itself isn't a good idea / implementation (if that even mattered), but that there is absolutely no mention of the nodejs connection anywhere and the resulting potential confusion @rctoris describes.

REP-144 asks for unambiguous naming of packages ("package names should be specific enough to identify what the package does") and that is a good convention. genjs doesn't seem to follow it.

@IanTheEngineer
Copy link
Contributor Author

Since this is a message generator, with no ties to NodeJS itself, we didn't want to unnecessarily bind it to Node. If you look at the other generator packages, they are named by the language intended to interpret the generated messages:

gencpp: Cpp
geneus: EusLisp
genlisp: Standard Lisp
genpy: Python

roslibjs is not a native client at all. The issue that I completely agree with is that we do not have any documentation thus far to differentiate from roslibjs. That is forthcoming, but we wanted the message generation to get in before the desktop full freeze, as it will lower the barrier to entry for anyone that wants to use Javascript and ROS in the same environment.

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Apr 21, 2016

If you look at the other generator packages, they are named by the language intended to interpret the generated messages:

yes, but none of those depend on an optional (essentially 3rd party) framework (although the two lisp variants would be a gray area, but they have the dialect in their name). It seems like genjs does, hence the questions about naming I guess.

@syrnick
Copy link

syrnick commented Apr 21, 2016

@IanTheEngineer genjs would probably require at least minor changes before it's usable in browser and if you want to support it you'd need a test suite for browser-specific coverage.

@IanTheEngineer
Copy link
Contributor Author

@IanTheEngineer genjs would probably require at least minor changes before it's usable in browser and if you want to support it you'd need a test suite for browser-specific coverage.

@syrnick I agree. This is why we mentioned a future improvement to the package to add browser support, but that would not change the core functionality of genjs for generating Javascript-readable messages. A test suite is a great idea and would likely be an external package that depends on genjs. As Dirk mentioned, keeping genjs's dependencies minimal is preferable.

@mikepurvis
Copy link
Contributor

mikepurvis commented Apr 22, 2016

roslibjs requires a bridge that translates messages to JSON format.

IIUC, this genjs proposal is more akin to rosserial, where the Javascript itself is consuming and producing the ROS serialized messages. This could absolutely be supported in some future version of roslibjs— it would just mean changing the bridge so that it skips over the reserialize-to-JSON step.

@tprk77
Copy link

tprk77 commented Apr 22, 2016

I would love to see this merged. I think a Node.js ROS client would be great. I've used rosbridge quite a bit, but there have definitely been times I wished I could talk ROS directly. I don't see this generator being at all disruptive to RWT, and I don't think people will be confused. (rosbridge is obviously dealing in JSON. It's all over the docs.)

@syrnick
Copy link

syrnick commented Apr 22, 2016

I looked through the code a bit. Looks like it's mostly browser-compatible. It uses node Buffer object for which there's a browser polyfill (https://github.com/feross/buffer).

The generators seem to have a bunch of assumptions that the code is used on the same machine that has the source installed. It'd be fine for development, but it'd be pretty painful to move it to a different machine. That might require making the base package with base_serialize/base_deserialize.js and making genjs actually generate some js-friendly package format (e.g. npm package).

@IanTheEngineer @chris-smith I think this would be fine to merge if you had a proof-of-concept of using this in a browser project with webpack. It might require generating more metadata for the messages themselves. The POC should be simple - e.g. just parsing a binary string into objects. If you find major roadblocks there, it'd be better to merge it as gennodejs.

It's definitely nice to have native binary encoding/decoding of messages! Thanks for doing that work!

@IanTheEngineer
Copy link
Contributor Author

@syrnick Thanks, we're excited to release this code to the community. I appreciate the enthusiasm on this naming subject, it tells me that you're going to be quite interested in contributing to rosjs / genjs in the future :)
Do you know of any execution environment that could run Javascript serverside aside from NodeJS or V8 (which Node is based on)? I think that is the real question here, as the serverside environment is the closest analog from the Javascript world to the ROS workspace world. The fact that genjs could be extended to run in the server is a feature that would be a nice to have, but not necessary to get this in the hands of people who want to include it in their ROS workspace environment.

@tprk77
Copy link

tprk77 commented Apr 23, 2016

@IanTheEngineer Possibly Rhino? But I think that's unlikely.

@mikeferguson
Copy link

Given all the discussion of future work on, one question I have is: how stable is genjs? How many releases are you expecting to put out over the lifecycle of Kinetic? As ros_comm will depend on it, this means that every genjs release will effectively force a complete rebuild of all ROS packages (consuming a lot of buildfarm time), and will also mean that users see a much larger update on those syncs (consuming a lot of bandwidth).

@IanTheEngineer
Copy link
Contributor Author

Hi Fergs. Yep, we are aware that any changes released in genjs will cause a
cascading rebuild of the build farm, and take that very seriously. In terms
of future improvement, I don't think this "runnable in a browser" feature
is imminent, but a desirable addition at some time in the future. We do
have one bugfix in the works right now, but after that, we anticipate
trying to keep things as stable as possible and minimize releases (possibly
coordinating them with new versions of ros_comm which itself will cause a
cascading rebuild).

Please correct me if I am wrong, but I don't see any reason for ros_comm to
depend on genjs. It only depends on roslisp to enable message generation
for "dry" rosbuild lisp packages:
https://github.com/ros/ros_comm/blob/kinetic-devel/ros_comm/package.xml

On Saturday, April 23, 2016, Michael Ferguson <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');> wrote:

Given all the discussion of future work on, one question I have is: how
stable is genjs? How many releases are you expecting to put out over the
lifecycle of Kinetic? As ros_comm will depend on it, this means that every
genjs release will effectively force a complete rebuild of all ROS packages
(consuming a lot of buildfarm time), and will also mean that users see a
much larger update on those syncs (consuming a lot of bandwidth).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#113 (comment)

@tfoote
Copy link
Member

tfoote commented Apr 23, 2016

Adding the generator as a default generator is actually even lower level than ros_comm to be present in the binary packages it needs to be added as a dependency of message_generation on which all message packages are required to build depend upon. And there are several message packages that packages in ros_comm depend upon.

We do need the default message generators to be quite stable as new releases do cause effectively a complete rebuild. We can coordinate it with the low level releases, thus we'd want to expect to release it at most as frequently as ros_comm

@mikeferguson
Copy link

I see -- I misread the name of the file that was being edited in the other PR and thought this was being added as a dependency of ros_comm, to make it a default.

@syrnick
Copy link

syrnick commented Apr 24, 2016

@IanTheEngineer I don't know if there are major engines that one would use server-side aside from Node (with V8 and perhaps chakra nodejs/node#4765). There are engines from Mozilla(Rhino, SpiderMonkey) and more on https://en.wikipedia.org/wiki/List_of_ECMAScript_engines . I imagine most server-side engines would allow you to pull in some re-implementation of node's Buffer that you use to (de-)serialize messages.

There are other interesting creatures like espruino (http://www.espruino.com/) that are javascript engines for microcontrollers. They would be much less forgiving for an essentially a node dependency.

Let me rephrase my initial point in slightly more general terms. If genjs depends on nodejs (e.g. relies https://nodejs.org/api/buffer.html), it should be called gennodejs to avoid confusion. It's possible that it would still be usable in other javascript environments, but the the users would be appropriately warned. To be called genjs, it should depend as much as possible on ES5 only (maybe ES6?) and have a clear interface that's not tied to node. That could be as simple as pulling out serializer/deserializer from it and having the consumer supply those.

Also https://github.com/genjs/genjs doesn't help.

Lastly, this isn't the only way for node :) Javascript has a well-developed ecosystem of just-in-time compilation & packaging that could be adapted to generating javascript message wrappers on the fly, so the message files could be required as just message files.

const StringMsg = require('std_msgs/String.msg');

You could adapt babel-register (https://github.com/babel/babel/blob/4c371132ae7321f6d08567eab54a59049e07f246/packages/babel-register/src/node.js) to do that. I believe babel-register was itself an adaptation of coffeescript (http://coffeescript.org/documentation/docs/register.html).

Maybe it's possible to write a babel plugin that does that.

@chris-smith
Copy link

@syrnick genjs does currently use Buffer. I did this because I was writing it immediately for my use in node where all the apis use Buffer. As far as I'm aware, Buffer uses javascript's Uint8Array under the hood though. Moving away from relying on it has been something I've wanted to experiment with. Particularly for parsing large arrays of items, javascript's native ArrayBuffer and view implementation could improve things - though I'm not familiar enough with them to say definitively. We could also explicitly rely on the polyfill instead. Your suggestion for serializer/deserializer is also possible. This is work though, not something that fundamentally and eternally breaks the applicability of genjs to production environments outside of node. As noted, we do need to be careful about releases to the ROS build farm, but we welcome any and all help in getting there - this code is open source.

genjs also relies on a fair amount of ROS infrastructure to actually generate messages. People could write this themselves in any number of adaptations, but on the fly message generation is not possible without pulling all of the message files into your workspace. Currently, ROS users only need to include packages for their personal message types in their repos or for default message packages they want to make changes to as an overlay (I imagine this is rare). I don't believe the raw message files are even available on a ROS installed build. To get them, users would need to pull down the myriad of message packages provided by a ROS install. This would burden ROS users who want to code in javascript.

There are also, obviously, a thousand ways to generate this code. It will be nice if we can support people outside of a ROS ecosystem relatively painlessly. But this is also supposed to be for people who are working in a ROS environment. Users should be able to run catkin_make and have usable javascript code generated for them automatically without needing to create special hooks. Since message_generation is at the very bottom of a very large dependency tree, the code that does this should ideally not introduce a ton of new dependencies on the build system.

@chris-smith
Copy link

I would also say, though, that I'm not sure what environments outside of node anybody would want to use the auto-generated code in. The generated code, by itself, is largely useless. rosjs has a lot more dependencies on node - that could also change but I don't really see that happening. I'm pretty open to renaming rosjs to rosnodejs.

Is anybody actually going to write a client library for those other environments? That also seems unlikely to me. This argument would push me towards also renaming genjs to gennodejs. But the amount of work to make genjs-generated code compatible with other environments is relatively small and the code is open source. Even if I don't get to it, someone else might realize that would be useful to them and do that work. It would be a little weird that the package is still called gennodejs under those circumstances if someone is using it to code in Rhino. It is also absolutely possible that this never happens and genjs remains eternally dependent on node. I think the reason to keep it named genjs for now is that the invitation to make these types of changes is open. If we name it gennodejs then we are essentially publicly closing off that possibility.

@gavanderhoorn
Copy link
Contributor

I think the reason to keep it named genjs for now is that the invitation to make these types of changes is open. If we name it gennodejs then we are essentially publicly closing off that possibility.

This strikes me as odd: package naming (at least as far as I can tell) has never taken things that might hypothetically eventually be added in a package's future into account (or the other way around). You're essentially saying we shouldn't let the name of this package reflect what it really does (or uses, or is intended for) because it might deter potential future contributors from extending it. That doesn't really make sense to me.

@chris-smith
Copy link

That's true - message generation feels a bit different to me compared to other ROS packages though. I don't really know lisp, but my assumption is that you couldn't really resolve the differences between code generated by geneus and genlisp despite both belonging to the same family. Maybe you can - maybe people who want to generate js code for Rhino should just write genrhinojs. Resolving differences could be done here though and it's something I have wanted to explore but haven't had time to look at.

I don't know that I have a high-enough view of the ROS eco-system to say that we should lean one way or the other in this scenario - leaving it genjs was the way I leaned though.

@tprk77
Copy link

tprk77 commented Apr 25, 2016

I think this debate over the name is a little silly. I think a name like gennodejs is a bit unexpected and seems to break convention. (I mean, you might need to also rename genlisp to gen-sbcl-lisp, since that's the only Lisp it works with. 😉) Anyway, if the only Node.js "dependency" is Buffer, then that's not really a dependency at all. Just use a bundler like Browserify.

@syrnick
Copy link

syrnick commented Apr 25, 2016

@chris-smith Message definitions are included in all distributions or else rosmsg show would be pretty tricky.

Here's the proof-of-concept for require hook. https://gist.github.com/syrnick/0cd20d838ddef407b564ff03cd88f9fe. The user would just do

require('genjs/require_hook');
const actionlibTutorialAveragingAction = require('actionlib_tutorials/msg/AveragingAction');

I cut some corners (removed MD5 and raw message) to make it work:
https://github.com/syrnick/genjs/compare/kinetic-devel...syrnick:hackedNodeRequire?expand=1. It obviously would need a bit more work. I think caching JS code by message name or message MD5 should be plenty efficient for node use.

Using a require hook and/or transpiling messages in js-land would make any changes to genjs much easier. E.g. if someone doesn't want to use ES6 classes, if they want to add flow or typescript annotations, they can just fork genjs and require that version. As people start using JS message wrappers, I'm sure they'll have feedback.

This setup closely matches the way JS language extensions are developed. Once there's a reasonable proposal for a feature, someone builds a babel plugin and it becomes available to users as an opt-in. The proposal may get altered and the transpiler gets updated (e.g. ES7 async/await is fabulous, but it'll be a long while before it's supported by the runtime).

As a side benefit, it makes it straight-forward to package all the necessary transpiled files into the JS package itself.

@IanTheEngineer
Copy link
Contributor Author

@syrnick thats interesting - my understanding was that the generated code itself had a hook to return the text rather than that the file itself existed. This discussion should be continued in either genjs or rosjs rather than here in the REP. There's already an issue open in rosjs about creating message classes on the fly, which seems relevant. As it stands, dynamic message generation is not a required feature for native ROS Javascript to be useful to people, but I do very much welcome (and look forward to) your contribution.

I will concede that @tprk77 and @gavanderhoorn are right: the naming of this package is not all that important at the end of the day, and the name will not deter future contributions. I'll make the necessary changes to turn this package into gennodejs. I'd like to underscore to everyone that, going forward, we intend to keep releases of gennodejs as infrequent (and stable) as possible to prevent rebuilds of the ROS buildfarm.

@IanTheEngineer IanTheEngineer changed the title updating 142 to add genjs as kinetic ros_core pkg updating 142 to add gennodejs as kinetic ros_core pkg Apr 28, 2016
@dirk-thomas
Copy link
Member

There are pros and cons for both suggested names. Renaming the new package to gennodejs sounds like a reasonable compromise to address the concern of several of you. It clearly states what the generated message can be used for (similar to geneuslisp) and differentiates it clearly from the ros js bridge.

Thank you to everyone for your feedback on this topic.

@dirk-thomas dirk-thomas merged commit ce470d9 into ros-infrastructure:master Apr 28, 2016
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.

10 participants