Skip to content

Conversation

@Karsten1987
Copy link

No description provided.

@Karsten1987 Karsten1987 added the in progress Actively being worked on (Kanban column) label Nov 29, 2016
@Karsten1987 Karsten1987 self-assigned this Nov 29, 2016
@Karsten1987 Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 29, 2016
"msg/Transition.msg"
)
set(srv_files
"srv/GetState.srv"
Copy link
Member

Choose a reason for hiding this comment

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

nicktip: I think we aim for alphabetical order

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

@mikaelarguedas mikaelarguedas Nov 29, 2016

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)

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

except nicktips looks good to me, maybe we will wait to merge it along ros2/rclcpp#265 given that it'll be the first piece of code using these messages?

@tfoote
Copy link
Contributor

tfoote commented Nov 29, 2016

We commonly put the state enumerations into the msg to make sure that they're available cross platform. I know that this is a little bit lower level and the goal is for all of this to be in rcl only but even if we choose not to enumerate here, it would be great to add a comment where to look up the valid states. It might make sense to simply have a State message with the integer and the enums. It's extra overhead but improves insepction. And can be reused for publishing the state and rendering the state in the future since the data will be semantically typed.

@@ -0,0 +1,2 @@
uint8 start_state
uint8 goal_state
Copy link
Member

@gbiggs gbiggs Nov 29, 2016

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

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.

@dirk-thomas
Copy link
Member

Should these messages maybe be closer to the other messages using in rcl (from rcl_interfaces)? I would prefer rcl to not depend on the common_interfaces repo.

@Karsten1987
Copy link
Author

I'd left out the states names on purpose for now or two reasons:

  • optimization. This also goes along with the fact that in-code I can compare my state to the actual enum value. Therefore I can still avoid using the actual state number.
  • customization: Might be a minor point here, but in case users wanted to create their own states, they don't have to modify the lifecycle_msgs

@Karsten1987
Copy link
Author

@dirk-thomas What exactly is the difference between rcl_interfaces and common_interfaces? This package contains only lifecycle_msgs, that'd be the only dependency.

@dirk-thomas
Copy link
Member

Repositories will need to be released in a specific order. If rcl(*) uses this new package the whole repo needs to be released before. That is the reason why rcl_interfaces is in a separate repo.

@@ -0,0 +1,3 @@
string node_name
---
uint8 current_state
Copy link
Member

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

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?

@Karsten1987 Karsten1987 closed this Dec 6, 2016
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Dec 6, 2016
@Karsten1987
Copy link
Author

closed in favor of ros2/rcl_interfaces#11

@Karsten1987 Karsten1987 deleted the lifecycle_impl branch December 8, 2016 02:33
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.

7 participants