Skip to content

add lifecycle_msgs #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

add lifecycle_msgs #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcad052
add lifecycle_msgs
Karsten1987 Nov 28, 2016
b2cf212
(dev) add default states for transition message
Karsten1987 Nov 29, 2016
b9bc65b
(fix) style
Karsten1987 Nov 29, 2016
66a7a0d
add default transition values for change_state service
Karsten1987 Nov 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lifecycle_msgs/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
cmake_minimum_required(VERSION 3.5)

project(lifecycle_msgs)

if(NOT WIN32)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wextra")
endif()

find_package(ament_cmake REQUIRED)
find_package(rosidl_default_generators REQUIRED)
find_package(std_msgs REQUIRED)

set(msg_files
"msg/Transition.msg"
)
set(srv_files
"srv/ChangeState.srv"
"srv/GetState.srv"
)

rosidl_generate_interfaces(${PROJECT_NAME}
${msg_files}
${srv_files}
DEPENDENCIES std_msgs
Copy link
Member

@mikaelarguedas mikaelarguedas Nov 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that you need to depend on std_msgs if you are using only primitive types (dependency can be removed from package.xml too)

ADD_LINTER_TESTS
)

ament_export_dependencies(rosidl_default_runtime)

ament_package()
12 changes: 12 additions & 0 deletions lifecycle_msgs/msg/Transition.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Default values for primary states
# as described on
# http://design.ros2.org/articles/node_lifecycle.html

uint8 STATE_UNKNOWN = 0
uint8 STATE_UNCONFIGURED = 1
uint8 STATE_INACTIVE = 2
uint8 STATE_ACTIVE = 3
uint8 STATE_FINALIZED = 4

uint8 start_state
uint8 goal_state
Copy link
Member

@gbiggs gbiggs Nov 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if ease of introspection would mean that the state name should be used instead of enumeration values? I can see the efficiency value in using one-byte numbers, but I can also see the value in being able to see state names from the command line. What concerns me is that unlike the enumeration used in the implementation, these messages form an external, visible interface.

I guess this probably needs more thinking about in terms of how we'll interact with a node's life cycle state machine. If there is a command line tool to manage them, then it can provide readability through its interface, but if we're going to end up using rostopic and rosservice to do it, then the messages themselves would need to be human-readable. I think I prefer the former.

Either way, the state enumerations should probably be in the message, as @tfoote suggested.

Copy link
Member

@wjwwood wjwwood Nov 29, 2016

Choose a reason for hiding this comment

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

Related to this, there was a discussion at some point about making rostopic echo in ROS 1 print out the "constant name" rather than the value. I can't find the discussion, it may have been in person, but the idea was that you could "hint" in the message description as to which fields would use the constants as values.

For example:

# Foo.msg

# hint: start constant group "states"
int STATE_ONE = 1
int STATE_TWO = 2
int STATE_THREE = 3
# hint: end constant group

int state  # hint: uses "states" constant group
int something_else

If you echo'ed that you'd get something like this:

---
state: STATE_ONE (1)
something_else: 1
---
state: STATE_TWO (2)
something_else: 2
---
state: STATE_THREE (3)
something_else: 3
---

This is done by rostopic echo introspecting the message comments to find the relationship between the values and comments. This doesn't help you when using this message programmatically, and doesn't address corner cases like groups having duplicate values, but at the time we were looking for a convention that didn't require a change to the message format. Instead we could change the message format to add the idea of typed enumerations, something like this:

# Bar.msg

enum States {
  STATE_ONE = 1,
  STATE_TWO = 2,
  STATE_THREE = 3,
}

States state
int something_else

In the end, it seems like a shame to avoid the optimization on a message type that is likely to be sent so many times in virtually all systems. My vote would be to keep the optimization and try to mitigate the introspection issues with tooling, but I can see the point of why strings for states would be compelling.

23 changes: 23 additions & 0 deletions lifecycle_msgs/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0"?>
<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="2">
<name>lifecycle_msgs</name>
<version>0.0.0</version>
<description>A package containing some lifecycle related message and service definitions.</description>
<maintainer email="karsten@osrfoundation.org">Karsten Knese</maintainer>
<license>Apache License 2.0</license>

<buildtool_depend>ament_cmake</buildtool_depend>
<buildtool_depend>rosidl_default_generators</buildtool_depend>

<build_depend>std_msgs</build_depend>

<exec_depend>rosidl_default_runtime</exec_depend>
<exec_depend>std_msgs</exec_depend>

<test_depend>ament_lint_common</test_depend>

<export>
<build_type>ament_cmake</build_type>
</export>
</package>
11 changes: 11 additions & 0 deletions lifecycle_msgs/srv/ChangeState.srv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
uint8 TRANSITION_CONFIGURING = 10
uint8 TRANSITION_CLEANINGUP = 11
uint8 TRANSITION_SHUTTINGDOWN = 12
uint8 TRANSITION_ACTIVATING = 13
uint8 TRANSITION_DEACTIVATING = 14
uint8 TRANSITION_ERRORPROCESSING = 15

string node_name
uint8 transition
---
bool success
3 changes: 3 additions & 0 deletions lifecycle_msgs/srv/GetState.srv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
string node_name
---
uint8 current_state
Copy link
Member

@mikaelarguedas mikaelarguedas Nov 30, 2016

Choose a reason for hiding this comment

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

should constants be defined for the current_state here as they've been defined in the Transition.msg file ? or this would be unnecessarily redundant?

Copy link
Member

@gbiggs gbiggs Dec 4, 2016

Choose a reason for hiding this comment

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

What about putting the constants for the default life cycle state in a separate file that gets included by both message definitions?