Lifecyle node and C state_machine #265

Karsten1987 · 2016-11-01T18:49:34Z

up for design review!
Not crustified yet. Not tested yet (except the demo). First prototype of a lifecycle node demo.

rcl_lifecycle contains a C based state machine with primary states and transitions as depicted in design.ros2.org.
rclcpp_lifecycle contains the higher level c++ implementation of a managed node. A managed node has a rcl_state_machine_t object as member for keeping track of the state and possible transitions. Further, it has the same API as a rclcpp::Node for creating publishers etc. However, each managed node has a handle to all pub/sub/srv/clients in order to activate/deactivate them.
lifecycle_talker.cpp is a first demo application. Missing here is any kind of container which opens services for calling the transition functions such as on_configure() or on_activate(), etc.

First commits are equal to #258

gbiggs

I think it would be architecturally nicer if the state machine itself could be used to trigger the appropriate callbacks in the node. Then the external API would just have to trigger a transition from the current state, and leave the rest up to the machinery of the (generic) state machine implementation. This would simplify the implementation of the managed lifecycle node class.

gbiggs · 2016-11-02T05:52:02Z

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

+ * Pure virtual functions as defined in
+ * http://design.ros2.org/articles/node_lifecycle.html
+ */
+class NodeInterface


Why not give the interface a decent name rather than putting it in a namespace? How about something like "ManagedLifecycleNodeInterface"?

gbiggs · 2016-11-02T05:53:39Z

rclcpp_lifecycle/include/rcl_lifecycle/default_state_machine.h

+/*static*/ const rcl_state_t LIFECYCLE_EXPORT rcl_state_inactive     = {/*.state = */1, /*.label = */"inactive"};
+/*static*/ const rcl_state_t LIFECYCLE_EXPORT rcl_state_active       = {/*.state = */2, /*.label = */"active"};
+/*static*/ const rcl_state_t LIFECYCLE_EXPORT rcl_state_finalized    = {/*.state = */3, /*.label = */"finalized"};
+/*static*/ const rcl_state_t LIFECYCLE_EXPORT rcl_state_error        = {/*.state = */4, /*.label = */"error"};


Where are the following states:
CleaningUp
Configuring
Activating
Deactivating
ShuttingDown

ErrorProcessing appears to have been renamed to error. Not a problem, but we should update the design document to match.

I only capture the 4 primary states. But yes, you're right. The remaining transition states will be added.

I think you could argue that the transition states don't need to be actual states. The implementation of the state machine just needs to make sure the correct functions are called as it transitions between the "main" states.

However, having them as actual states would be useful for debugging, and it might make it easier to understand the execution time usage of a managed node that has real-time characteristics.

gbiggs · 2016-11-02T05:54:57Z

rclcpp_lifecycle/include/rcl_lifecycle/transition_map.h

+typedef struct LIFECYCLE_EXPORT _index
+{
+  unsigned int index;
+  const char* label;


Label of what? The state? If we have the state index why do we need the label as well?

label here was meant to be a human readable string such as "active" or "inactive".

gbiggs · 2016-11-02T06:06:43Z

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

+ * @brief LifecycleNode as child class of rclcpp Node
+ * has lifecycle nodeinterface for configuring this node.
+ */
+class LifecycleNode : public rclcpp::node::Node, public lifecycle_interface::NodeInterface


This is starting to show the problem of not having a Node interface. "Favor 'object composition' over 'class inheritance'" is often thrown around and abused, but C++ best practice generally says to not inherit implementations except for minor tweaks to behaviour. It may work as is for this case, but for a major entity like Node, having an abstract interface would be more flexible, and would make things more testable.

I agree. The node implementation needs a refreshing. @wjwwood is currently working on a refactoring which addresses the things you mentioned such as composition vs. inheritance.

gbiggs · 2016-11-02T06:07:56Z

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

+  }
+
+  LIFECYCLE_EXPORT
+  ~LifecycleNode(){}


= default?

gbiggs · 2016-11-02T06:37:21Z

rclcpp_lifecycle/include/rclcpp_lifecycle/type_traits/is_manageable_node.hpp

+{};
+
+template<class T>
+struct is_manageable_node<T, typename std::enable_if< has_on_activate<T>::value


Big yes to doing things this way!

That will work on compile time. However, a base class is needed at some point for dynamic runtime loading of nodes. See ros2/demos#84 as an example.

The two ideas are not incompatible.

gbiggs · 2016-11-02T06:43:57Z

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

+
+  LIFECYCLE_EXPORT
+  virtual bool
+  on_configure()


This is not the right name for the external API for controlling the state machine.

This implementation mixes life cycle control (when to configure) and node-specific life cycle functionality (such as what to do in the Configuring transition state/onConfigure() callback) into a single function.

This function should be called configure and there should be a separate method, implemented by users who inherit from this class to define their own managed nodes, called onConfigure. In that function will go all their configuring stuff. In this function will go the life cycle management stuff (which is here already) and any configuration that should be done for every node.

This comment applies to the other transition functions as well.

What I had in mind here is that these on_* functions can be overridden by users in order to take in place their own implementation. What's generally missing here is an external entry point (such as a LifecycleManager or similar) which handles commands such as configure and then calls on_configure on the respective lifecycle node.

Yes, the external API is what is missing. Your prototype's example gives the impression that you are treating the external and internal APIs the same.

I think that there should be a node interface that defines the external API. When the node is registered with a LIfecycleManager or something like that, then it would work with that API. A user could also manage their node's lifecycle directly from their main function, if they wish. Simultaneously, there needs to be a base class with virtual void methods (on_activate etc) for the implementer of a specific node to override to provide their own functionality.

If you like I can write up a short example, but I think you probably know what I mean and are already working in that direction.

gbiggs · 2016-11-02T06:44:44Z

rclcpp_lifecycle/src/lifecycle_talker.cpp

+
+  lc_node->print_state_machine();
+
+  if (!lc_node->on_configure())


The external API of a managed lifecycle node for controlling transitions between these states is not the callbacks. There should be an API containing the 7 transitions mentioned in the design document (create, configure, etc.) When, for example, configure() is called on the node, it should execute the on_configure() callback and shift its state to the appropriate successor state based on the result.

See also my comment in the managed lifecycle node implementation.

There is no external API at this point. I put the callback calls in the talker example for demonstration. I see the talker code here as a lifecycle manager or similar which itself then has a public external API for triggering configure, activate, etc. (also via service calls)

In that case, perhaps "talker" is not a good name for the demonstratation class.

gbiggs · 2016-11-02T06:45:42Z

rclcpp_lifecycle/src/lifecycle_talker.cpp

+
+  lc_node->print_state_machine();
+
+  if (!lc_node->on_configure())


It would be good if we can minimise how much the user has to manually control the node's life cycle. Possibly that will come through another API layered on top, which this talker example should be modified to use when it's done.

gbiggs · 2016-11-02T06:47:52Z

rclcpp_lifecycle/include/rcl_lifecycle/lifecycle_state.h

+ * possible transitions registered with this
+ * state machine.
+ */
+typedef struct LIFECYCLE_EXPORT _rcl_state_machine_t


Do you intend to implement managed lifecycle nodes in rcl as well?

If not, what is the purpose of writing the state machine in C rather than C++, or rather than using something like Boost's state machine library?

Good point. I indeed wrote this in C for multilanguage purposes such that every rcl* has the same set of states and valid transitions.
I believe ROS2 generally tries not to rely on boost.

I have no opinion on using or avoiding Boost, and I haven't actually looked at the Boost state machine library in quite a while. I just thought I would mention it as an option.

Anyway, I agree with having it in C so other languages can re-use the managed lifecycle as-is.

gbiggs

I like the way this is going!

gbiggs

What is the value in the state machine implementation differentiating between states with on-going actions and those once-only states that occur between the on-going states? From a FSM point of view, there is no difference (one requires a trigger to leave, the other leaves as soon as it has done some particular processing). I think it would simplify the implementation if we did not differentiate between the two, except where we define the triggers that allow the machinery to move to the next state.

So "active" would wait for the triggers "error" or "deactivate" before leaving, but "deactivating" would run the on_deactivate function and then immediately transition to "error" or "inactive" based on the result.

dirk-thomas · 2016-11-08T18:47:20Z

rclcpp_lifecycle/CMakeLists.txt

+)
+
+macro(targets)
+  if(NOT target_suffix STREQUAL "")


This condition is not necessary and doesn't do what you would expect. The condition is basically always true since target_suffix is never an empty string but undefined in that case. Just always calling get_rclcpp_information is fine.

direct copy from https://github.com/ros2/examples/blob/master/rclcpp_examples/CMakeLists.txt#L30
we should generally define a CMakeLists.txt as an example.

dirk-thomas · 2016-11-08T18:51:16Z

rclcpp_lifecycle/CMakeLists.txt

+install(TARGETS rcl_lifecycle
+    ARCHIVE DESTINATION lib
+    LIBRARY DESTINATION lib
+)


Needs to install to bin on Windows / for RUNTIME DESTINATION.

dirk-thomas · 2016-11-08T18:54:24Z

Maybe we should define the ROS-level interface of the managed node / the lifecycle manager. Similar to what is defined in rcl_interfaces for the parameters.

Karsten1987 · 2016-11-08T19:25:19Z

@gbiggs I try to replicate basically what's depicted here: http://design.ros2.org/articles/node_lifecycle.html
I agree that these transition states are not completely defined. I just want to keep the vocabulary as is. But this may be open for change.

@dirk-thomas Do you propose to generate the c-style structs via message generation?

dirk-thomas · 2016-11-08T19:29:02Z

I was thinking about the services to control the state machine as well as the events / messages when the state changes. Kind of the external API used for introspection / orchestration. I am not sure if it makes sense for the structs.

dirk-thomas · 2016-11-09T00:27:07Z

rclcpp_lifecycle/include/rcl_lifecycle/lifecycle_state.h

+//#define bool int;
+// #ifdef __cplusplus
+// #error WRONG COMPILER
+// #endif


What about these comments?

dirk-thomas · 2016-11-09T00:28:10Z

rclcpp_lifecycle/src/rclcpp_lifecycle/lifecycle_talker.cpp

+    //loop_rate.sleep();
+    ++i;
+  }
+  */


What about these comments?

gbiggs · 2016-11-09T16:53:19Z

The managed nodes design document only has an ambiguous description of the management interface. I had done some work on a more detailed description but it got put on hold when Tully said he was going to do the node lifecycle implementation soon (because I wanted to see if anything changed). So I dug that work up and finished it off: ros2/design#99

Karsten1987 · 2016-11-10T05:32:40Z

@wjwwood I'd like to bring your attention to the last commit 58ae127. Does this go along with what we discussed? Therefore, I'd separate the node into a base_interface and a communication_interface.

Then please note that in the current talker.cpp if have preprocessor #if/#else derivatives which indicate a possible user api - namely the decision on how strictly we take the DRY principle.

wjwwood · 2016-11-10T21:14:24Z

e86473e looks like the right direction to me.

Karsten1987 · 2016-11-17T05:57:05Z

See lifecycle_talker.cpp as a demo on how a lifecycle orchestration would look like.

Status so far:

StateMachine in C
basic support for Publisher/Server in RCL data structures
LifecycleManager in CPP with two services get_state and change_state

TODO:

refactor Pub/Sub/Srv/Clients for extending their constructors in taking rcl_data structures directly.
write extensive tests
clarify on whether to integrate this PR into rcl/rclpp or in a self containing package called lifecycle.

Any thoughts on the latter?

Karsten1987 · 2016-11-24T01:04:46Z

This PR requires #279 to be merged.
And also a fix for ROSIDL message generation in C. @mikaelarguedas can you have a look at it?

diff --git a/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport.h b/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport.h
index 5fac344..f8ac1d6 100644
--- a/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport.h
+++ b/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport.h
@@ -76,6 +76,7 @@ struct StringHelper<rosidl_typesupport_introspection_c__MessageMembers>
     std::string str;
     deser >> str;
     rosidl_generator_c__String * c_str = static_cast<rosidl_generator_c__String *>(field);
+    rosidl_generator_c__String__init(c_str);
     rosidl_generator_c__String__assign(c_str, str.c_str());
   }
 };

mikaelarguedas · 2016-11-24T01:42:22Z

as discussed offline, we need to figure out why the rosidl_generator_c__String was not allocated properly before being passed to the fastrtps rmw_implementation. I proposed this quick fix just to get the thing unstuck but I think that reallocating memory in the vendor specific layers "in case it's not properly initialized" is not the proper fix. We should ensure at the rcl/rclcpp level that we are providing properly initialized data structures to all rmw_implementations and not patch this in the vendor specific code if not necessary.

Could you run CI on this to see if the same behavior shows up on Connext?

Karsten1987 · 2016-11-29T04:31:03Z

I refactored this initial PR and extract 3 more dependent PRs on it.

lifecycle_msgs: ros2/common_interfaces#24
rcl_lifecycle: ros2/rcl#91

further depends on two fixes:
rmw-fastrtps: ros2/rmw_fastrtps#69
service in rclcpp: #279

change exception message for windows ci bug

* [rcl_lifecycle] remove rosidl deps as this package doesnt generate any messages * depend on rosidl_generator_c

* Update build and test workflow * Update `setup-ros` to 0.0.13 * Update `action-ros-ci` to 0.0.13 Signed-off-by: Zachary Michaels <[email protected]>

wjwwood added the in progress Actively being worked on (Kanban column) label Nov 1, 2016

Karsten1987 self-assigned this Nov 1, 2016

gbiggs reviewed Nov 2, 2016

View reviewed changes

Karsten1987 force-pushed the lifecycle_impl branch 2 times, most recently from 276a1c6 to d092184 Compare November 4, 2016 23:32

gbiggs approved these changes Nov 7, 2016

View reviewed changes

gbiggs reviewed Nov 7, 2016

View reviewed changes

dirk-thomas reviewed Nov 8, 2016

View reviewed changes

dirk-thomas reviewed Nov 9, 2016

View reviewed changes

rclcpp_lifecycle/src/rclcpp_lifecycle/lifecycle_talker.cpp

//loop_rate.sleep();

++i;

}

*/

Copy link

Member

dirk-thomas Nov 9, 2016

Choose a reason for hiding this comment

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

What about these comments?

Karsten1987 force-pushed the lifecycle_impl branch from 58ae127 to e86473e Compare November 10, 2016 16:27

Karsten1987 force-pushed the lifecycle_impl branch 3 times, most recently from 23783e4 to 4f07abc Compare November 24, 2016 00:59

Karsten1987 force-pushed the lifecycle_impl branch 2 times, most recently from 2b42d73 to b371044 Compare November 29, 2016 01:49

Karsten1987 force-pushed the lifecycle_impl branch 3 times, most recently from 9f4cb84 to 456baa0 Compare December 9, 2016 01:43

Karsten1987 and others added 24 commits December 14, 2016 09:11

(refactor) comply for new state machine

fd8a32c

visibility control and test api

3a78ab9

(rebase) change to new typesupport

14402e7

uncrustify'

bd5db8d

fix visibility control

f0e2c1b

(fix) correct whitespace

99598c2

(fix) unused variable

89a7149

comparison signed and unsigned

6a41839

get_state returns complete state

c4c2ea1

get_available_states service

35539e3

new service msgs

5a7f67c

get available states and transitions api

1db9aac

(broken) state after rebase, does not compile demos

dd1dc88

fix the way lifecycle node impl is included

e28d800

fixed rebase compilation errors

1a615e1

remove copy&paste comment

f0416fa

remove empty line

c0ad638

(test) register custom callbacks

48211c2

(dev) return codes

1cdb89a

style

e2ff2c0

test for exception handling

b38e611

refacotr new state machine

0bda7b7

c++14

a08c736

change exception message for windows ci bug

49e9ed3

change exception message for windows ci bug

Karsten1987 force-pushed the lifecycle_impl branch from 436514a to 49e9ed3 Compare December 14, 2016 17:13

Karsten1987 merged commit 2c6d959 into master Dec 14, 2016

Karsten1987 deleted the lifecycle_impl branch December 14, 2016 17:29

Karsten1987 removed the in review Waiting for review (Kanban column) label Dec 14, 2016

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Fix rosidl dependencies (ros2#265)

9a9762f

* [rcl_lifecycle] remove rosidl deps as this package doesnt generate any messages * depend on rosidl_generator_c

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

Update build and test workflow (ros2#265)

1dfd42b

* Update build and test workflow * Update `setup-ros` to 0.0.13 * Update `action-ros-ci` to 0.0.13 Signed-off-by: Zachary Michaels <[email protected]>


		lc_node->print_state_machine();

		if (!lc_node->on_configure())

Lifecyle node and C state_machine #265

Lifecyle node and C state_machine #265

Conversation

Karsten1987 commented Nov 1, 2016

gbiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbiggs Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbiggs left a comment

Choose a reason for hiding this comment

gbiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 8, 2016

Karsten1987 commented Nov 8, 2016

dirk-thomas commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbiggs commented Nov 9, 2016

Karsten1987 commented Nov 10, 2016

wjwwood commented Nov 10, 2016

Karsten1987 commented Nov 17, 2016

Karsten1987 commented Nov 24, 2016

mikaelarguedas commented Nov 24, 2016

Karsten1987 commented Nov 29, 2016

gbiggs Nov 4, 2016 •

edited

Loading