Skip to content

Conversation

wayneparrott
Copy link
Collaborator

This PR implements the ROS2 Lifecycle Node (managed node) as described in the ROS2 Managed Node design and implemented by the rclcpp lifecycle implementation.

Key changes:

index.js (updated)

  • added lifecyle import
  • added rclnodejs.lifecycle property - simulate namespace
  • added createLifecycleNode()
  • refactored common logic for node creation for use in both createNode() and createLifecycleNode()

index.ts (updated)

  • added TypeScript declarations for createLifecyleNode()

lifecycle.js (new)

  • includes LifecycleNode, a subclass of ShadowNode, and lifecycle support classes and interfaces

index.d.ts (new)

  • includes TypeScript declarations for LifecycleNode and lifecycle support classes and interfaces.

rcl_lifecycle_bindings.hpp (new)
rcl_lifecycle_bindings.cpp (new)

  • contains native addon C functions for interfacing with rcl_lifecycle_state_machine data-structures and api.

lifecycle_publisher.js (new)

  • implements LifecyclePublisher - based on the rclcpp LifecyclePublisher implementation

lifecycle_publisher.d.ts (new)

  • TypeScript declarations for LifecyclePublisher.

publisher.js (updated)

  • refactored common publisher creation functionality for reuse by LifecyclePublisher.

test-lifecycle.js (new)

  • unit tests for LifecycleNode and related classes

test-lifecycle-publisher.js (new)

  • unit tests for LifecyclePublisher and related classes

main.ts (updated)

  • added dtslint LifecycleNode cases

lifecycle-node-example.js (new)

  • demonstrates how to use LifecycleNode, register transition callbacks and change states.

Fix #589

@coveralls
Copy link

coveralls commented Nov 13, 2020

Coverage Status

Coverage decreased (-0.3%) to 91.289% when pulling 44ccb79 on wayneparrott:lifecycle into 4ff5957 on RobotWebTools:develop.

Copy link
Member

@minggangw minggangw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing this HUGE pull request and I look through it quickly, generally looks good! As I don't understand lifecycle comprehensively, I'd like to invite @koonpeng to review it, thanks!

@minggangw minggangw requested a review from koonpeng November 16, 2020 08:12
@koonpeng
Copy link
Collaborator

this is quite a large PR, I may need some time to review this 😅. I don't have experience with lifecycle nodes as well so I will just try to review based on my knowledge of rcl.

@wayneparrott
Copy link
Collaborator Author

wayneparrott commented Nov 17, 2020

@koonpeng @minggangw et al, please ping me if I can be of assistance during the review.

At work I usually start reviews by asking the dev how comfortable he/she is with a specific unit of code, are there any hot spots or overly complex areas that he has concerns. With that I'll share 2 areas that need discussion:

  1. I've already highlighted that the implementation makes 4 Services created by the rcl lifecycle api visible to rclnodejs. A work-around of the RclHandler Reset behavior was implemented to avoid double free() error of the Service ptr.

edited @wayneparrott - Please hold off on further review. While describing design issue #2 below I identified a simplification which I will submit an update later today.

2. The lifecycle.js module requires an initialize() function to be called before it is runnable. I'm not fully comfortable with this. This requirement arises because of a dependency cycle that arises because I was trying to use lifecycle state machine constants defined in the lifecycle State and Transition messages rather than hard-code those same constants in JS code. An alternate approach is to hard-code the lifecycle state machine constants defined in the lifecyle State and Transition messages. I'm not opposed to the latter approach. But wanted to get the insight of others before doing so.

tldr;
Here's the details behind this and an alternate approach:

The State and Transition message definitions include the state id constants used by the lifecycle state machine. The implementation loads instances of these message types at runtime in order to reference their state machine constants. This dependency leads to the Lifecycle module not being runnable until message files have been generated which happens at rclnodejs installation and at rlcnodejs.init() time. Here's where it gets tricky, index.js loads the lifecycle module and exposes it through a new public lifecycle property, i.e., rclnodejs.lifecycle. To work around this chicken-egg dependency the lifecycle module has an initialize() function that must be called after calling rclnodejs.init(). At this time, lifecycle.initialize() is called implicitly as a part of rclnodejs.createLifecycleNode().

Summary of the dependency: rclnodejs -> lifecycle module -> generated messages -> rclnodejs.init()

The lifecycle module dependency upon the State and Transition messages could be eliminated by hard-coding their state machine constants in JS code rather than pulling them from these 2 messages. I was hoping to avoid hardcoding the constants. But as a consequence, the lifecycle module has to wait for rclnodejs.init() to complete before it can safely load the 2 messages.

So before going too deep into the review, let's decide if the current design with dependencies on the message constants is acceptable or if we should work to remove the dependency on the State and Transition messages, i.e., hard code the constants. At this time I'm very open to hard coding the constants. Thoughts?
tldr;

@wayneparrott
Copy link
Collaborator Author

wayneparrott commented Nov 17, 2020

The latest commit should be the one. It includes @minggangw earlier suggestions; especially see rclhandler.cpp free_ptr_. In lifecycle.js I switch to using lazy initialization of a couple of message interfaces which I feel is a good simplification from my initial commit.

Copy link
Collaborator

@koonpeng koonpeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing back to SetAccessors, I wonder if it's possible to bind all the rcl structs to js objects.

Copy link
Member

@minggangw minggangw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no further comments, lgtm, and thanks for implementing this important feature, great progress!

@minggangw
Copy link
Member

I think we could move forward, @koonpeng any comments? @wayneparrott please merge it when it's good to go (soft reminder: please select "Squash and merge" option)

Copy link
Collaborator

@koonpeng koonpeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Key Changes based on @minggangw comments and rethinking clunky
initialalization of StateInterface and TransitionInterface.
rcl_handle.hpp/cpp
* added `free_ptr_` bool property to RclHandle

lifecycle.js
* removed initialize() function and refactored to use lazy
initialization of StateInterface and TransitionInterface
* update shutdown() to use rcl_lifecycle_shutdown_label
@wayneparrott wayneparrott merged commit 25b6076 into RobotWebTools:develop Nov 23, 2020
minggangw pushed a commit that referenced this pull request Nov 26, 2020
lifecycle api, types, tests & example.
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.

LifecycleNode support in rclnodejs
4 participants