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

Features needed to implement - umbrella issue #498

Open
17 of 18 tasks
minggangw opened this issue Jun 26, 2019 · 44 comments
Open
17 of 18 tasks

Features needed to implement - umbrella issue #498

minggangw opened this issue Jun 26, 2019 · 44 comments

Comments

@minggangw
Copy link
Member

minggangw commented Jun 26, 2019

As ROS 2 iterates rapidly, actually, there are two stable releases per year and a series of patch releases. I want to emphasize the target of this Node.js client is that it has the comparative feature set as the python client - rclpy.

But there is still a gap with rclpy, and we want to eliminate it finally. Due to limited resource, I'd like to invite any volunteer to contribute to this project, so the developers can have an additional choice besides C++ and Python languages. I believe that the requirement of web technology in ROS 2 ecosystem is strong, and JavaScript/Node.js enable more developers to work on ROS project quickly because of the low threshold.

I have tried my best to maintain rclnodejs module can work on the latest ROS 2 stable release, e.g. Dashing Diademata, as soon as possible. I hope there are more contributors to implement the absent features and I will offer any help to move this project forward. Your contributions are welcome!

I list the features needed to implement below, so we can record the status. If anyone wants to handle it, please just assign it to yourself to indicate the feature is being implemented. Thanks!

P.S.
I add an indicator (Low/Medial/High) to represent the combination of workload and difficulty of each issue. You can reference it to decide which feature will be suitable for you to handle.

Some suggestions in my mind:

  • Please follow the Google C++ coding style to write your code, you can run npm run lint to verify it locally.
  • For JavaScript code, ES6 is preferred. Also, checked by eslint.
  • The current code on develop branch is based on the nightly build of ROS 2, and we will create tags for each stable release of ROS 2 when it's published.
  • The features above have counterparts in rclpy, so you can reference the C code there to wrap your JS.
  • Please add proper comments to illustrate your code if necessary, you can reference the existing ones (Currently the API doc is generated from them by jsdoc).
  • Please offer unit tests if features are added/changed, as we want to be TDD.
  • All PRs will be verified on CIs first, and the review work will start after all CIs pass.
  • Please read README to setup your own ROS2 env on your PC.
  • If you are the FIRST time to submit your code, please add yourself as a contributor.
  • If you meet any problems, please ping me and I will response ASAP. Also, I'd like to help part of the code (I cannot fully focus on it now, sorry).
  • Once your PR gets LGTM, I will merge it and you can get your credit :)

If I miss some feature or features you want to add, please append them to this bullet.

@suab321321
Copy link

I would like to contribute.
Could you please elaborate a little

@minggangw
Copy link
Member Author

minggangw commented Jul 2, 2019

Thanks for your response @suab321321 which angle do you want to know more, e.g. workload, difficulty level or coding rule? I can write a summary or BKM to facilitate other people who are also interested in this project. Thus, we can move forward smoothly!

@minggangw
Copy link
Member Author

I posted a topic on ROS discourse to let more people know it.

@suab321321
Copy link

I would like to know workload..

@suab321321
Copy link

I would like to contribute to #492 first..
Thank you

@minggangw
Copy link
Member Author

@suab321321 I've marked it for each feature, it's just kind of rough evaluation, not exact. Also, I added some suggestions, please check it before starting your work as it may save your time.

Again, you are welcome. Now please go ahead!

@suab321321
Copy link

suab321321 commented Jul 4, 2019

okay..
I understand the indicator tag now.
Now I want to understand how will I contribute to #421 because no description is provided on what to do

Thank you

@minggangw minggangw added this to In progress in 0.11.0 Jul 4, 2019
@minggangw
Copy link
Member Author

Actually, there is no specific requirement or design doc to describe the feature, all implementations should base on the rcl.

As for the rclnodejs, you can reference the rclpy client (It's a good way to get started). You may notice that the parameter feature contains several issues, I created these by ros2/rclpy#202, so I think you can find the implementation then.

@suab321321
Copy link

Okay..I will start now

@suab321321
Copy link

suab321321 commented Jul 4, 2019

I m unable to install rclnodejs either from npm or from sractch...its showing some file import error

OS-ubuntu 18.04
Screenshot from 2019-07-05 01-23-23

@minggangw
Copy link
Member Author

Have you executed source <path/to/ros2>/install/local_setup.bash , if not please follow the steps here, thanks!

@suab321321
Copy link

Yes I have done that...

@suab321321
Copy link

Is this issue common or is it just me?

@minggangw
Copy link
Member Author

It's not common, please double check with echo $COLCON_PREFIX_PATH. From the error log, it should be the problem of the wrong include path. I suggest that you clone this project and checkout the develop branch to start instead of adding it as a dependency.

@suab321321
Copy link

Nope..Its not happening

@mikelyndersOKCC
Copy link

Hello, could you elaborate on the steps that need to be done to wrap rcl with Node.js? What are some resources for users who would like to help contribute to one of these features. I would like to help with Actions support #469 but I am not sure where to start. I found this article on Nodejs addons.

Maybe you could give an example of a rcl feature (such as publisher/subscriber) and show how the rcl feature is wrapped by the C++ addon (here?) and then where the C++ addon is implemented in the Node.js library (here?) Such that one could extrapolate the process to the requested features listed above.

I am a primarily a python developer and I have enough JS experience to build GUI’s (React) for my ROS projects (using ROS2-web-bridge). However, my experience with Node.js by itself is limited (never heard of C++ addons until now). Will I be getting myself in over my head with this?

@wayneparrott
Copy link
Collaborator

hi @mikelyndersOKCC, good question. Last week I started work on implementing parameter support for rclnodejs. My C coding skills are pretty rusty and basic at best - but why let that stop me :) It took me a couple of hours of exploring code and docs to develop a mental model and impl a hello-world example. From there, I systematically evolved my node-c++ rcl binding.

  • one of the project's objectives is to maximize compatibility across ros2 releases. The rcl is a C implementation for which other rcl language bindings including rclnodejs are based. So get familiar with the project and it's api as you'll want to bind to it if at all possible for your feature.
  • take a look at current rcl bindings in rcl_bindings.cpp and bindings.gyp

General resources (I took bits and pieces from these and others)

best wishes.

@minggangw
Copy link
Member Author

minggangw commented Jan 23, 2020

Hi @mikelyndersOKCC thanks for your question, I think @wayneparrott has given some general steps to touch on the development of Node.js addon. I'd like to share some of my ideas

  • The development of addon for Node.js

    • We are using the "old" API (called nan) to bind the JS interface to C++ instead of using N-API which can decouple your code from runtime e.g. V8
    • As @wayneparrott has mentioned the nan library, here is the example repo. I think referencing the existing code in rclnodejs for binding is enough to write your own code 😄
    • The toolchain used for addon development for Node.js is node-gyp . Ideally, you don't need to change it.
  • The development of rclnodejs

    • The general design of it, please see the 7th slide
    • rcl is the C interfaces we want to wrap and expose to JavaScript through the nan library.
    • There are usually two parts to implement a new feature for rclnodejs
      1. Define and write the JavaScript interfaces to be exposed to users
      2. Implement the C++ binding methods called on the JavaScript side (you need to add binding functions into rcl_bindings.cpp in most cases)
    • I strongly you could refence the implementation of the feature you are going to do for rclnodejs in rclpy, because they are very similar C++ <=> Python & C++ <=> JavaScript, it will save your time to do so.
  • For the test
    To ensure the quality and stability of rclnodejs, we are runing a series of unit tests on Linux, Windows10 and macOS platforms, you can see the three badges in README. Some static check will also run e.g. eslint & cpplint. So, if you implement a new feature, you'd better write the test cases for it. The framework for testing is mocha which is used widely for Node.js projects.

If you have any problems, please feel free to ask on Github. Thanks!

P.S. the original issue RobotWebTools/ros2-web-bridge#130

@mikelyndersOKCC
Copy link

@minggangw @wayneparrott Thank you both very much for your insight! I really appreciate the guidance. I will study the materials that have been linked here and hopefully I can do something with it!

@suab321321
Copy link

@minggangw @mikelyndersOKCC I would like to contribute to #421 .Is anyone working on this issue?

@wayneparrott
Copy link
Collaborator

wayneparrott commented Jan 28, 2020

@suab321321 I'm about 50% complete on a PR for #416. Hoping to submit it by EOW or some time next weekend.

@suab321321
Copy link

@wayneparrott is #414 free?Is this free?

@wayneparrott
Copy link
Collaborator

@suab321321 re: #414, it is part of #416. I've started working on #416 based on the rcl library. I've temporarily halted development in order to finish a PR for ros2cli to support pluggable build-types. My plan is to finish the ros2cli PR tomorrow and resume #416. But if you are interested in #416 I will be glad to yield it to you or anyone else that is ready to push it forward. While you asked about #414 my assessment is that it's design needs to account for #421 which will initialize the node's parameters from the commandline.

@suab321321
Copy link

@wayneparrott can you tell me which is most beginners friendly issue?

@minggangw
Copy link
Member Author

Hi @suab321321 I suggest @wayneparrott is going to handle all param related features, so there will be no blockers or dependencies throughout the implementation (I have updated the assignees for these issues).

Meanwhile, I check the existing issues and probably #492 is suitable for beginners (I have tagged it with good-first-issue). Again, I'd like to mention some necessary knowledge one may need if he/she wants to contribute, please see below:

Generally, the whole framework of rclnodejs is not complicated, as it's only a thin wrapper of rcl library and some platform related implementations. So you'd better have experiences

  • Language skills: JavaScript (ES6) and C++ (C+14/17)
  • Node.js C++ addon development
  • Have some basic concepts of ROS/ROS2, e.g. pub/sub, client/service

Steps and useful methods for ramping up, please see the comments (#498 (comment) and #498 (comment)) in this thread, thanks!

@suab321321
Copy link

suab321321 commented Jan 30, 2020

@minggangw okay I will begin the work soon.

@suab321321
Copy link

suab321321 commented Jan 31, 2020

@minggangw @wayneparrott I m finding it extremly difficult to understand rcl_binding.cpp
Is is rcl_binding.cpp same to rclnodejs as rmw for rclpy and rclcpp?
I found that inside rclpy _rclpy.c the implementations are similar to what is there rcl_bindings.cpp isnt it?

@minggangw
Copy link
Member Author

Hi @suab321321 I recommend you learn from some examples of Node.js addon and try to understand the mechanism of binding C++ to JavaScript. #554 is a good demonstration.

Is is rcl_binding.cpp same to rclnodejs as rmw for rclpy and rclcpp?

rmw is the middleware layer of ROS2 and it's transparent to us. What we should concern is rcl (based on rmw) library. _rclpy.c is the counterpart in Python bindings compared with Node.js (rcl_bindings.cpp), so the behaviors between them are similar.

@suab321321
Copy link

@minggangw I have some doubt in rcl_binding.cpp

NAN_METHOD(CreateTimer) {
  int64_t period_ms = info[0]->IntegerValue();
  RclHandle* context_handle = RclHandle::Unwrap<RclHandle>(info[1]->ToObject());
  rcl_context_t* context =
      reinterpret_cast<rcl_context_t*>(context_handle->ptr());
  rcl_timer_t* timer =
      reinterpret_cast<rcl_timer_t*>(malloc(sizeof(rcl_timer_t)));
  *timer = rcl_get_zero_initialized_timer();
  rcl_clock_t* clock =
      reinterpret_cast<rcl_clock_t*>(malloc(sizeof(rcl_clock_t)));
  rcl_allocator_t allocator = rcl_get_default_allocator();
  THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK,
                           rcl_clock_init(RCL_STEADY_TIME, clock, &allocator),
                           rcl_get_error_string().str);
  THROW_ERROR_IF_NOT_EQUAL(
      RCL_RET_OK,
      rcl_timer_init(timer, clock, context, RCL_MS_TO_NS(period_ms), nullptr,
                     rcl_get_default_allocator()),
      rcl_get_error_string().str);

  auto js_obj = RclHandle::NewInstance(timer, nullptr, [timer, clock] {
    rcl_ret_t ret_clock = rcl_clock_fini(clock);
    free(clock);
    rcl_ret_t ret_timer = rcl_timer_fini(timer);
    return ret_clock || ret_timer;
  });
  info.GetReturnValue().Set(js_obj);
}

NAN_METHOD(IsTimerReady) {
  RclHandle* timer_handle = RclHandle::Unwrap<RclHandle>(info[0]->ToObject());
  rcl_timer_t* timer = reinterpret_cast<rcl_timer_t*>(timer_handle->ptr());
  bool is_ready = false;

  THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, rcl_timer_is_ready(timer, &is_ready),
                           rcl_get_error_string().str);

  info.GetReturnValue().Set(Nan::New(is_ready));
}

@suab321321
Copy link

suab321321 commented Feb 3, 2020

@minggangw in above code in function CreateTimer() rcl_timer_t* variable is by one method but in function IsTimerReady() rcl_timer_t* variable is created by another method.
1.Why is this?
2.RclHandle* timer_handle = RclHandle::Unwrap<RclHandle>(info[0]->ToObject()); can you give a brief explanation what is happening here? (I only know that info is used to access the paramter passed from javascript functions). :)

@minggangw
Copy link
Member Author

1.Why is this?

In CreateTimer, we need to allocate the memory for the handle, while IsTimerReady, we get the handle passed from JavaScript, which was created by the first method. So they are different usages.

2.RclHandle* timer_handle = RclHandle::Unwrap(info[0]->ToObject());

Here we unwrap the C++ object from the parameter as you said, some detailed explanations please see https://nodejs.org/api/addons.html#addons_passing_wrapped_objects_around and https://github.com/nodejs/nan/blob/master/doc/object_wrappers.md

@suab321321
Copy link

suab321321 commented Feb 20, 2020

@minggangw sir I think I m on verge solving of this issue :)
I have a questions.

  1. are callbacks functions called using event driven architecture of nodejs(Event emitter)?

@suab321321
Copy link

I can see that there is execute() function in node.js which is checking for callbacks for subscription,timer,etc.but shouldnt this run infinetly to check for if a certain event has occured?

@suab321321
Copy link

One more question..
When creating a new timer we are needed to create a timerHandle but not in case of Publisher or Subscriber.why is this?

@minggangw
Copy link
Member Author

I can see that there is execute() function in node.js

executed() function is triggered by the sub-thread, so it doesn't need to run infinitely (as a loop).

When creating a new timer we are needed to create a timerHandle but not in case of Publisher or Subscriber.why is this?

the handle represents a "handle" of C resource, see

constructor(handle, typeClass, options) {

@suab321321
Copy link

suab321321 commented Feb 22, 2020

@minggangw I need to add some parameters like publisher class constructor so shoud I pass it as an inside the options objects because see this

/**
   * Create a Publisher.
   * @param {function|string|object} typeClass - The ROS message class,
        OR a string representing the message class, e.g. 'std_msgs/msg/String',
        OR an object representing the message class, e.g. {package: 'std_msgs', type: 'msg', name: 'String'}
   * @param {string} topic - The name of the topic.
   * @param {object} options - The options argument used to parameterize the publisher.
   * @param {boolean} options.enableTypedArray - The topic will use TypedArray if necessary, default: true.
   * @param {QoS} options.qos - ROS Middleware "quality of service" settings for the publisher, default: QoS.profileDefault.
   * @return {Publisher} - An instance of Publisher.

or should I add one more parameter in method.

@minggangw
Copy link
Member Author

@suab321321 do you have a specific requirement or an issue (please create a new one if there is none), I suggest we'd better discuss there as this is a general issue for features to be implemented, thanks!

@suab321321
Copy link

suab321321 commented Feb 23, 2020

@minggangw no no,see when we add the feature of liveliness and deadline callback, then this should be declared while creating Publisher/Subscriber, for this thing I need to add one more paramter in the methods which is used for creating Publisher/Subsciber, so I m asking if need to embed this new parameter inside options object or add just add a new paramter field.
What guidelines is followed I dont know?

and one more thing :)
what is the use of timerCallback(),I have seen that at the end this function calls rcl_timer.c but cant understand what is its role

this._timers.forEach((timer) => {
      if (timer.isReady()) {
        rclnodejs.callTimer(timer.handle);
        timer.callback();
      }
    });

*** Should we to continue this to #492 thread.***

@wayneparrott
Copy link
Collaborator

wayneparrott commented Feb 28, 2020

A useful feature to consider is the message_filter. The current ros2 implementation is found here https://github.com/ros2/message_filters.

@mattrichard
Copy link
Collaborator

@wayneparrott It seems like message filters would be better implemented as a separate library, if possible, since it's not built into rclcpp. But that starts bringing up the question of how to distribute rclnodejs packages. Distributing on npm with rclnodejs as a peer-dep would probably be fine for now.

@flynneva
Copy link

flynneva commented Mar 24, 2020

I'd love to help out with this! it looks like most of the topics are already done (sorry for being late to the party).

I'll poke around and see where I can help

@minggangw
Copy link
Member Author

Hi @flynneva welcome, the checklist in c#1 shows some main features which are not implemented yet, but I think there should be some others not found. If you have experience with rclpy, it's easy to discover the missing functions or you could enhance some existing ones. Some insights:

Also, you can add/fix any features, thanks!

@FlorisE
Copy link

FlorisE commented Aug 28, 2020

roslibjs has some support for getting information about a running ROS system via http://robotwebtools.org/jsdoc/roslibjs/current/Ros.html, like getting the list of active nodes, mesage information, etc. Should rclnodejs offer such functionality, and if yes, is it already implemented somewhere? If no, is there another JS library that offers such functionality for ROS 2?

@minggangw
Copy link
Member Author

rclnodejs is not proper to implement this sort of APIs, instead, rosbridge acting as back-end of roslibjs should offer it, some reference

If no, is there another JS library that offers such functionality for ROS 2?

I haven't heard some JS library offers that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
0.11.0
  
In progress
Development

No branches or pull requests

7 participants