Skip to content
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.

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

Add action_msgs package #41

Merged
merged 3 commits into from
Oct 25, 2018
Merged

Add action_msgs package #41

merged 3 commits into from
Oct 25, 2018

Conversation

jacobperron
Copy link
Member

Contains common message and service definitions for all ROS actions.

Messages:

  • GoalStatus.msg - Contains a goal ID, timestamp, and status.
  • GoalStatusArray.msg - An array of GoalStatus.msg.
  • UUID.msg - Type of goal IDs; this is temporary until we port the existing uuid_msgs (see TODOs).

Service:

  • CancelGoal.srv - API for canceling goals describing the cancel policy.

Resolves #40

Contains common message and service definitions for all ROS actions.

Messages:
- GoalStatus.msg - Contains a goal ID, timestamp, and status.
- GoalStatusArray.msg - An array of GoalStatus.msg.
- UUID.msg - Type of goal IDs; this is temporary until we port the existing uuid_msgs (see TODOs).

Service:
- CancelGoal.srv - API for canceling goals describing the cancel policy.
@jacobperron jacobperron added the in progress Actively being worked on (Kanban column) label Oct 19, 2018

<build_depend>builtin_interfaces</build_depend>

<exec_depend>builtin_interfaces</exec_depend>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think <depend>builtin_interfaces</depend> to get all three build_depend, exec_depend, and build_export_depend would make sense because a C generator would export a header with an include to builtin_interfaces/msg/Time.h

Copy link
Member

Choose a reason for hiding this comment

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

In general there are only two types of dependencies: the ones for build time and the ones for run time. Internally exec_depend and build_export_depend do the same thing - they are only named separately since users found it "easier" / more intuitive than run_depend.

Anyway if you use both then using depend makes sense 👍

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 19, 2018
@jacobperron
Copy link
Member Author

Ready for review.

I've added GoalInfo.msg for encapsulating the goal ID and timestamp and updated the package.xml to use a <depend> tag.

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron
Copy link
Member Author

Blocked by ros2/rosidl#307

@jacobperron
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM. Just one opinion that UUID could live in builtin_interfaces, but that is a conversation that can be resolved later.

@@ -0,0 +1,6 @@
# Goal ID
# TODO(jacobperron): Use a ported version of https://github.com/ros-geographic-info/unique_identifier/blob/master/uuid_msgs
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that uuid_msgs has just a single message, I think the UUID.msg could live in builtin_interfaces rather than porting a package named uuid_msgs. Porting the tools in unique_id wouldn't need to block that.

@jacobperron
Copy link
Member Author

Just one opinion that UUID could live in builtin_interfaces, but that is a conversation that can be resolved later.

Sounds good.

@dirk-thomas
Copy link
Member

builtin_interfaces only contains types defined in DDS. I don't think we should add any other ROS specific types to it.

@jacobperron jacobperron merged commit 47d3ad3 into master Oct 25, 2018
@jacobperron jacobperron deleted the action_msgs branch October 25, 2018 20:41
@jacobperron
Copy link
Member Author

@sloretz @dirk-thomas
Created a ticket regarding UUID message definitions: ros2/ros2#593

@dirk-thomas
Copy link
Member

See ros2/common_interfaces#64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants