(refactor) add rcl_lifecycle package by Karsten1987 · Pull Request #91 · ros2/rcl

Karsten1987 · 2016-11-29T04:26:10Z

lifecycle state machine
notification publisher for state changes
service for get state, change state

wjwwood

Other than some nitpicks and questions, the code looks good to me. The only thing I think must change is the error handling and how the progress is rolled back in the init function comments I had.

wjwwood · 2016-12-07T10:49:24Z

+find_package(lifecycle_msgs REQUIRED)
+
+include_directories(
+  include)


nitpick: this doesn't need to be wrapped

wjwwood · 2016-12-07T10:52:21Z

+# :type var_prefix: string
+#
+# Copy/pasted from rclcpp/cmake/get_rclcpp_information.cmake.
+# Code duplication is evil. Don't do this at home, kids.


This probably deserves a TODO so that we don't forget to deduplicate this code at some point.

wjwwood · 2016-12-07T10:54:28Z

+ * @param state: integer giving the state
+ * @param label: label for easy indexing
+ */
+typedef struct _rcl_lifecycle_state_t


nitpick: I don't think it is necessary to use a leading _ here, or at least the rest of rcl doesn't do it this way.

wjwwood · 2016-12-07T10:56:58Z

+#endif
+
+/**
+ * @brief simple definition of a state


Elsewhere we use this doxygen pattern:

/// Brief description on a single line. /* Detailed description, with multiple * lines if necessary. * * \param foo we use the \ rather than the @ style escapes too. */

We can discuss which to use, but I believe I decided to use the above as it is the recommend "C++ style" rather than the "Java style" which you're using here. Either way, it would be good to have one style throughout the packages.

wjwwood · 2016-12-07T11:00:32Z

+/**
+ * @brief transition definition
+ * @param start: rcl_lifecycle_state_t as a start state
+ * @param goal: rcl_lifecycle_state_t as a goal state


I'm not sure you should use the @param directive to describe members of the struct. There is a member documentation syntax for doxygen, see http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html (search for Putting documentation after members as there doesn't seem to be an achor).

wjwwood · 2016-12-07T11:28:49Z

+    if (ret != RCL_RET_OK) {
+      fcn_ret = RCL_RET_ERROR;
+    }
+    lifecycle_msgs__msg__TransitionEvent__fini(&msg);


If you were going for reverse order destruction, then this message should be finalized before the publisher.

wjwwood · 2016-12-07T11:33:10Z

+  const rcl_lifecycle_state_t * start, const rcl_lifecycle_state_t * goal)
+{
+  msg.start_state.id = start->id;
+  rosidl_generator_c__String__assign(&msg.start_state.label, start->label);


This function returns a bool which should be checked.

wjwwood · 2016-12-07T11:35:30Z

+// limitations under the License.
+
+#ifndef COM_INTERFACE_HXX_
+#define COM_INTERFACE_HXX_


Why is this an .hxx file?

that addresses a linter complaint when including private headers. The linter requires an include folder with it. The linter only checks .h and .hpp though. Is there a recommended way of doing so?

Can you give an example of this? I'm not familiar with the linter error you're talking about.

4:[...] Include the directory when naming .h files [build/include] [4]

test_communication does lint its files (https://github.com/ros2/system_tests/blob/8074a4128b785c6c2376ec432e94d8bc5e85ff04/test_communication/CMakeLists.txt#L16) and uses a local header too (https://github.com/ros2/system_tests/blob/8074a4128b785c6c2376ec432e94d8bc5e85ff04/test_communication/test/message_fixtures.hpp). And somehow that passes 😉

wjwwood · 2016-12-07T11:36:58Z

+
+  // add transitions to map
+  // TODO(karsten1987): Add global transition for unknown/resetting
+  // TODO(karsten1987): Pointer comparison fails here because of copy!


Does this need to be addressed before merging?

wjwwood · 2016-12-07T11:40:57Z

+    return RCL_RET_ERROR;
+  }
+
+  // TODO(Karsten1987): pointer comparison fails here


Again, should this be addressed before a merge?

Karsten1987 · 2016-12-07T15:43:07Z

Thanks for the review. I'll address the comments asap and reopen this PR for review.

dirk-thomas

Throughout the code we use the extension .hpp, not .hxx.

dirk-thomas · 2016-12-07T17:42:25Z

+     // and limited in length of 255
+    const char * topic_prefix = "__transition_notify";
+    char * topic_name;
+    if (concatenate(&node_name, &topic_prefix, &topic_name) != true) {


The comparison with != true can be written simpler by just prefixing the call with !.

There are similar variations throughout the patch like == true, == NULL, etc.

dirk-thomas · 2016-12-07T17:44:13Z

+
+  {  // initialize publisher
+     // Build topic, topic suffix hardcoded for now
+     // and limited in length of 255


The three space indentation looks weird. Maybe don't put the first comment in line 85 but into a new line. Than it can have consistent indentation with the block.

Same below.

This comment still applies.

dirk-thomas · 2016-12-07T17:44:54Z

+
+RCL_LIFECYCLE_PUBLIC
+rcl_ret_t
+rcl_lifecycle_state_machine_init(rcl_lifecycle_state_machine_t * state_machine,


... as well as other files.

dirk-thomas · 2016-12-07T17:48:01Z

+  // start label and classify them.
+  const rcl_lifecycle_state_t * start_state = rcl_lifecycle_get_primary_state(m, start_id);
+  if (start_state == NULL) {
+    // return false here?


Should this be addressed and the return type of the function be changed?

Karsten1987 · 2016-12-08T00:48:47Z

I updated most of the comments mentioned.
The concat function and custom allocator is not changed yet. Mainly also in favor of getting this PR done.

@gbiggs Thanks for your comment. I changed the state machine transitions in a way that is hopefully easier to grabs. All transitions are now equally implemented. This leads that a transition is composed of two individual steps by now:

example:

1.) transition from unconfigured to configuring

current state is now configuring
will never fail

2.) transition from configuring to inactive

this transition needs feedback from the on_configuring callback
if callback feedback was success -> current state is errorprocessing
if callback feedback was no success -> current state is active

3.) in case of error, two more transitions are available

cleanup indicates all error are resolved -> current_state is unconfigured
shutdown indicates that errors could not be resolved -> current_state is finalized

Karsten1987 · 2016-12-08T17:35:17Z

CI jobs:

windows:
osx:
linux:

gbiggs · 2016-12-08T17:43:32Z

@Karsten1987 Thanks for working to make the state machine implementation clearer.

I still think that it is too complex, though. I think that implementing it as a plain state machine is sufficient. Equipping every transition with two destinations is not how state machines are typically designed or implemented. Rather, every state should just be a state, and every transition should just be from one state to another state, with no deviations. The current implementation will be confusing for anyone who knows how state machines work because their assumptions will be incorrect. This and the additional complexity mean it is less maintainable and less reusable. It will also struggle if at any point we have states with more than two possible transitions out.

The movement from the unconfigured state to the inactive state, for example, could be implemented something like this:

unconfigured is a state where the state machine sits until some external input triggers the transition to the configuring state. The implementation of this state would probably do nothing but wait for external input to trigger one of the valid transitions out of this state.
The state machine shifts to the configuring state.
The configuring state calls the user-provided callback on_configure.
The on_configure callback returns success or failure.
The configuring state checks the result. If it is success, it triggers the transition to the inactive state. If the result is failure, it triggers the transition to the errorprocessing state.
The state machine shifts to the inactive or errorprocessing state. Things go on from there in the same fashion.

This can be achieved by making the state machine machinery execute a registered function for each state. Each function would be responsible for controlling which next transition is executed and when. Some of those functions would also call user-provided callbacks for the node behaviour.

Karsten1987 · 2016-12-09T01:02:07Z

@gbiggs I am sorry for not being clear at this point. The current implementation is pretty much exactly the way you described it.
Every state is exactly one a single state (regardless if later on considered transition or primary state). And every transition is only a transition between two states. I simply attached the error state at each transition. This may be a premature at this point, but it'd allow having custom error handling states for each transition.
There are further two main functions for constructing the state machine:

register state https://github.com/ros2/rcl/blob/lifecycle_impl/rcl_lifecycle/include/rcl_lifecycle/transition_map.h#L28
register transition https://github.com/ros2/rcl/blob/lifecycle_impl/rcl_lifecycle/include/rcl_lifecycle/transition_map.h#L34

Then again, this is shared C code, which serves as a base for high level languages. The current rclcpp api https://github.com/ros2/rclcpp/blob/lifecycle_impl/rclcpp_lifecycle/test/test_lifecycle_node.cpp provides then an user friendly interface, which internally checks whether a callback returned successfully or not.

Karsten1987 · 2016-12-10T23:20:54Z

Linux:
OSX:
Windows:

Windows seems to be a unrelated flaky test

gbiggs · 2016-12-12T02:17:19Z

@Karsten1987 I did look at the implementation, and it is not as I attempted to describe it. The effect is the same, but the implementation does not have certain states checking for callback results and choosing the next state. It has the transition checking the result and choosing the next state. It is implemented as transitions with "on transition" actions (which are often used in state machines) but two possible destinations based on the result.

A standard FSM may execute something when a transition occurs but the result of that action does not have any impact on the next state. Only the actions executed in the state can have an impact on the next state. This is why I think the current implementation is not correct. Instead of transitions with callbacks, with two states inactive and active, you should have callback-less transitions and three states, inactive, activating and active. The action executed in each of these states will determine what the next state is. For inactive and active, if the callback returns failure then the next state is clearly error_processing, but if it returns success then the next state is just inactive or active again. (Some external stimulus being required to move to the next nominal state; also the designed state machine does not have a do/ action for inactive but I wanted to make the explanation clearer.) For activating the next state is either error_processing or active, because that state does not have a transition back to itself.

The current implementation does work for the current life cycle state machine, because it just happens that all the transitions between important life cycle states go via states with callbacks that may fail. But I think it is likely that someone will want to implement a custom life cycle for their nodes at some point. It would be nice if the implementation would be generic so that any desired life cycle state machine can be implemented. If this is not a goal, then I think we should just have a hard-coded state machine, which means getting rid of things like register_callback. That would be easier to test and maintain.

Also, configurable error states are fine, but they should be done by altering the state machine, not by having transitions with multiple possible destinations.

I re-made the state machine diagram that Tully (I think it was) made making it clearer where functions are called and when transitions happen. Remember that if a particular transition is not triggered, then the state machine does not move.

https://gist.github.com/gbiggs/3c887c6a56c16190e685e63b6557f6fc#file-managed_node_default_lifecycle-png

wjwwood · 2016-12-12T09:06:50Z

As far as I can see, each transition (made by calling rcl_lifecycle_start_transition) has exactly two end states that are possible, the goal of the transition and the error state. I think this does violate the idea of a transition for a FSM as you said:

Also, configurable error states are fine, but they should be done by altering the state machine, not by having transitions with multiple possible destinations.

We could refactor that function to not take the success bool, which switches the new current state:

https://github.com/ros2/rcl/blob/lifecycle_impl/rcl_lifecycle/src/rcl_lifecycle.c#L165-L171

and have explicit transitions between each appropriate state and the error processing state. However, I do not believe that the implemented system has any other ambiguity of what the end state can be, i.e. the transitions do not determine what the next non-error state will be, there is always exactly one.

Separately from that, I think that your diagram you linked is incorrect because it says that the states like configuring and activating go to the error processing state if their callback (e.g. onConfigure and onActivate) fails. From my perspective, the state "Activating" calls a callback "onActivate" which the user can override and if the user returns true, then it transitions to the active state, and if they throw an exception (or otherwise indicate an unhandled error) then it will trigger a transition to the error processing state, but (and this is where your diagram departs from the diagram in the design doc I think) if the user returns false for "onActivate" then the Activating state should trigger a transition back to the inactive state. @tfoote can confirm that this is the correct behavior, but I think it is because otherwise it would be impossible to fail an activation and not end up in the unconfigured or finalized state. It's also pretty clear to me that this is what is described in the design doc's diagram.

Now, @Karsten1987's implementation doesn't do that as-is but that's another issue 😛. Currently this pr implements it such that returning false by the user in either onConfigure or onActivate will result in a transition to the error processing state and from there to either the unconfigured or finalized state. This might be ok for onConfigure because it would have ended up in the unconfigured state anyways, but I do not believe this is the desired end behavior for onActivate.

As for having a generic FSM implementation, I agree that this is not that. If we wanted that then we would need to refactor the "change_state" method in the C++:

https://github.com/ros2/rclcpp/blob/ac6789dd7d0379c6ae405e4584836ea68475674f/rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp#L274-L317

Currently this function has a few assumed preconditions which hold true for our state machine, but do not hold true for all FSM:

the start state for the transition is one of the "primary states" as described in the design doc
the callback for the immediate next state returns a bool which can be used to indicate whether or not to go to yet another state or to an error state

So this is some simple, but implicit logic that is baked into our state machine which means that all valid start states transition to "autonomous" states which do not require external stimulus to then transition to another valid "start" state. This function takes these steps:

verifies the requested transition is valid
makes the transition
executes the new current states callback
uses the false return code or raised exception to transition to the error state or otherwise transition to the next state (success state)

While this is not a generic FSM, I do not come to the same conclusion as you @gbiggs (that the transitions are responsible for deciding the next state or that the transitions can fail) because it is actually the callback of the state (configuring or activating, etc.) which determines the next state and not the transition itself.

To make it more generic we would want to refactor it such that it:

verifies the requested transition is valid
makes the transition
calls the callback for the new current state

Then in the implementation of the callback for the current states we would cause additional transitions if appropriate. For example the callback of the Activating state might:

call the user's onActivate callback
on return of true from the user schedule transition to the active state
on return of false from the user schedule transition back to the inactive state
on an unhandled exception (or similar for the language) schedule transition to error processing

A similar set of logic would then be implemented for each state, like Configuring, Deactivating, Unconfiguring, etc. The callback for other states like unconfigured, inactive, and active may contain some logic, but by design would not schedule a new transition themselves, but rather would wait for external stimulus to do so.

With all of that being said, I do not think we need to refactor the state machine to behave like this right now. I think this because I see this as largely an implementation detail. In either case I think the user will implement callbacks for some states and not for others and they will manually trigger transitions out of some states but not for others, and that this will not change materially if we change the state machine as described above.

The caveat to that is that we may need to do this refactor now in order for @Karsten1987 to change the behavior to match the document, i.e. so that a return of false from onActivate returns you to the inactive state not the error processing state, but that isn't clear to me.

gbiggs · 2016-12-12T23:55:21Z

Separately from that, I think that your diagram you linked is incorrect because it says that the states like configuring and activating go to the error processing state if their callback (e.g. onConfigure and onActivate) fails. ........... This might be ok for onConfigure because it would have ended up in the unconfigured state anyways, but I do not believe this is the desired end behavior for onActivate.

I based my diagram on the text specification at http://design.ros2.org/articles/node_lifecycle.html rather than the diagram. As an example, for the activating state this spec says that valid transitions are:

If the onActivate callback succeeds the node will transition to Active.
If the onActivate callback raises or results in any other return code the node will transition to ErrorProcessing.

I have no problems with changing the behaviour to allow a false return from the callback to transition back to inactive. I agree that even if this is not commonly used someone might find it useful. If there is an agreement on this then I will update the text and diagram accordingly.

I do not think that the current implementation needs to be refactored as I have asked to enable this. The function controlling the transition can do it already with a minor change.

gbiggs · 2016-12-12T23:59:31Z

While this is not a generic FSM, I do not come to the same conclusion as you @gbiggs (that the transitions are responsible for deciding the next state or that the transitions can fail) because ...........
A similar set of logic would then be implemented for each state, like Configuring, Deactivating, Unconfiguring, etc. The callback for other states like unconfigured, inactive, and active may contain some logic, but by design would not schedule a new transition themselves, but rather would wait for external stimulus to do so.

I think I can see that my waffling comments are not comprehensible enough. 😄 I was trying to abstract from the implementation to the effective result.

What you propose is pretty much what I am asking for.

With all of that being said, I do not think we need to refactor the state machine to behave like this right now. I think this because I see this as largely an implementation detail. In either case I think the user will implement callbacks for some states and not for others and they will manually trigger transitions out of some states but not for others, and that this will not change materially if we change the state machine as described above.

Yes, I agree with this point. The implementation of this specific state machine is not user-facing, so long as the user is happy with the existing state machine.

wjwwood · 2016-12-13T00:54:29Z

I based my diagram on the text specification at http://design.ros2.org/articles/node_lifecycle.html rather than the diagram.

Ahhhh, you're right the diagram and text are in disagreement. Talking with @tfoote I think the text suffers from a copy-paste error. I'll coordinate with @Karsten1987 to address this issue, either by leaving it as-is for now and plan to address it after the beta or work with him to update the text in the document and his implementation here to get consistent behavior.

Also we're gonna try to merge your pr against that design doc.

wjwwood

Other than the existing comments (about the indention and the .hxx file, etc.) and the apparently reverting commits, both of which are just logistical things, this lgtm.

wjwwood · 2016-12-13T09:27:08Z

 }

 rcl_ret_t
-rcl_trigger_guard_condition(const rcl_guard_condition_t * guard_condition)


These changes seem to undo recent commits to master. I think you need to make sure your rebase worked correctly...

Actually, they don't undo master, but they do the same as master. I think they'll just disappear on merge, but if you have an opportunity to rebase before the final CI run, you can get rid of this commit that way.

* misra rule 10.1 non-boolean comparison * misra c rule 8.2 (partial) * misra c rule 14.4 * fix shadowing issue on ret variable * match argument name in header and c file

Use -Wpedantic

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

Karsten1987 self-assigned this Nov 29, 2016

Karsten1987 mentioned this pull request Nov 29, 2016

Lifecyle node and C state_machine ros2/rclcpp#265

Merged

Karsten1987 force-pushed the lifecycle_impl branch 2 times, most recently from 2b316e2 to 7a71150 Compare December 2, 2016 21:14

Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 2, 2016

Karsten1987 force-pushed the lifecycle_impl branch 3 times, most recently from 91ef95e to 16ea839 Compare December 3, 2016 00:03

Karsten1987 added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Dec 6, 2016

Karsten1987 force-pushed the lifecycle_impl branch from 734a92f to de669fe Compare December 6, 2016 22:54

wjwwood requested changes Dec 7, 2016

View reviewed changes

dirk-thomas reviewed Dec 7, 2016

View reviewed changes

Karsten1987 force-pushed the lifecycle_impl branch from a9ee85c to 1cc68ff Compare December 9, 2016 00:34

Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 11, 2016

wjwwood approved these changes Dec 13, 2016

View reviewed changes

Karsten1987 added 24 commits December 14, 2016 09:14

(clean) style and lifecycle prefix

854b161

(dev) cmake macro get_rcl_lifecycle_information

572242b

(cleanup) remove unused files'

211389b

(cleanup) remove callback pointer

96f030b

(debug) add print state machine function

b931197

(review) address review comments

ddeaa96

(bugfix) correct export in information.cmake

749a7f2

(fix) correct visibility control

4d4ec06

(vcs) fix convertion from size_t to unsigned int

f953d78

(typo) fix internal struct name

db17c76

const correctness

ad4c039

get_available_states service

87b1442

new service msgs

1d462bd

tes for multiple instances

2948a97

(dev) return codes

91a9233

initial refactor

7cf8c57

test default sequence

cf18b75

refactor state machine

ae29b51

apply upstream changes

a5c61eb

c++14

cfd4f4c

disable state machine print

54aa407

address review comments

fa0f908

uncrustify

cc553ce

fix comparison with unsigned warning

afa9a81

Karsten1987 force-pushed the lifecycle_impl branch from a5ec0e7 to afa9a81 Compare December 14, 2016 17:15

Karsten1987 merged commit a18ef97 into master Dec 14, 2016

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

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

ivanpauno pushed a commit that referenced this pull request Jan 2, 2020

Merge pull request #91 from ros2/pedantic

4c87763

Use -Wpedantic

Conversation

Karsten1987 commented Nov 29, 2016 • edited by wjwwood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Dec 7, 2016

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Dec 8, 2016

Uh oh!

Karsten1987 commented Dec 8, 2016

Uh oh!

gbiggs commented Dec 8, 2016

Uh oh!

Karsten1987 commented Dec 9, 2016

Uh oh!

Karsten1987 commented Dec 10, 2016

Uh oh!

gbiggs commented Dec 12, 2016

Uh oh!

wjwwood commented Dec 12, 2016

Uh oh!

gbiggs commented Dec 12, 2016

Uh oh!

gbiggs commented Dec 12, 2016

Uh oh!

wjwwood commented Dec 13, 2016

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Karsten1987 commented Nov 29, 2016 •

edited by wjwwood

Loading

dirk-thomas Dec 8, 2016 •

edited

Loading