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
319 stars 70 forks source link

lifecycle api, types, tests & example #722

Closed wayneparrott closed 3 years ago

wayneparrott commented 3 years ago

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)

index.ts (updated)

lifecycle.js (new)

index.d.ts (new)

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

lifecycle_publisher.js (new)

lifecycle_publisher.d.ts (new)

publisher.js (updated)

test-lifecycle.js (new)

test-lifecycle-publisher.js (new)

main.ts (updated)

lifecycle-node-example.js (new)

Fix #589

coveralls commented 3 years ago

Coverage Status

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

koonpeng commented 3 years ago

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

wayneparrott commented 3 years ago

@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 commented 3 years ago

The latest commit should be the one. It includes @minggangw earlier suggestions; especially see rclhandler.cpp freeptr. 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.

minggangw commented 3 years ago

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)