RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
316 stars 70 forks source link

Propose making Node extendable (i.e, create Node subclasses) #753

Closed wayneparrott closed 3 years ago

wayneparrott commented 3 years ago

This is a proposal to slightly refactor rclnodejs such that it is possible to create Node subclasses without having to add a factory function to rcl (index.js). Included in this proposal is a requirement to maintain the current rcl api as is.

tldr; In rclnodejs the rcl module (see index.js) provides2 factory methods, one for creating basic Node instances and one for creating LifecycleNode instances. It is not possible to create instances of user-defined Node subclasses without adding additional factory methods to rcl. Over the past year of my use of rclnodejs there have been several times where I felt that creating a Node subclass would have simplified the organization of my app code, i.e., package custom code with in a node subclass. The need was not general enough to warrant adding a 1-off factory function to rcl. I encountered this limitation again recently when I implemented LifecycleNode. And I have been a little jealous at seeing that others were able to subclass Node in the rclcpp for similar purposes. With that, I'm proposing an enhancement to the rclnodejs design such that Node subclasses can be created directly. The enhancement will not change any current public api.

I have implemented a PR that I would like to submit if this idea is of broader interest.

I've implemented a PR that does this by:

  1. at a high-level, the developer directly creates a Node using it's constructor. He can then directly spin(), stop() & destroy() a node via it's api.
  2. expose Node constructor(), includes moving handler logic from rcl.createNode() to the constructor
  3. make Context responsible for managing it's Nodes, i.e., Context contains refs to it's nodes. Moved rcl context->nodes[]mapping to Contetx as a static property. Calling context.shutdown() now destroys the context's nodes.
  4. created ShadowNode <- Node <- LifecycleNode hierarchy
  5. expose spin(), spinOnce(), stop() as public Node methods
  6. all changes are backward compatible with rcl api, e.g., createNode(), spin(), shutdown(), isShutdown(). But I did mark these functions as deprecated (preliminary).

Here's an example of using this new design:

import * as rclnodejs from ('rclnodejs');

class ExampleNode extends rclnodejs.Node {
  constructor() {
    // use remapping to rename at runtime if needed
    super('ExampleNode');
    this._publisher = this.createPublisher('Example_Publisher', 'example');
  }

  publish(msg) {
    // preprocess msg here
    this._publisher.publish(msg);
  }

 // setup custom node state here, an example might be broadcasting robot state, and then spin() 

  destroy() {
   //do some custom cleanup
   super.destroy();
}

async function main() {
  await rclnodejs.init(); // use default Context
  let node = new ExampleNode();
  node.spin();
  rclnodejs.shutdown();
}

Thoughts?