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 support for action topic and services (ROS2) #214

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

achim-k
Copy link
Collaborator

@achim-k achim-k commented Apr 13, 2023

Public-Facing Changes

  • Add support for action topic and services (ROS2)

Description

Adds support for parsing .action files to retrieve message definitions for the following action subtypes:

  • feedback topic (_FeedbackMessage) *
  • SendGoal
    • request (_SendGoal_Request) *
    • response (_SendGoal_Response)
  • GetResult
    • request (_GetResult_Request)
    • response (_GetResult_Response) *

Unfortunately, the action definition in the .action file does not include other fields such as the goal_id which are sent as part of the feedback message or SendGoal_Request and GetResult_Response. These types are therefore added to the message definition such that the entire schema is generated correctly.

Parsing IDL action definitions is currently not supported.

Example usage in Studio:
image

Adds support for parsing .action files to add message definitions for
the various action subtypes (feedback message and service request /
response message for  SendGoal and GetResult)
@achim-k achim-k requested a review from jtbandes April 13, 2023 18:26
namespace foxglove {

// Match datatype names (foo_msgs/Bar or foo_msgs/msg/Bar or foo_msgs/srv/Bar)
static const std::regex PACKAGE_TYPENAME_REGEX{
R"(^([a-zA-Z0-9_]+)/(?:msg/|srv/|action/)?([a-zA-Z0-9_]+)$)"};
R"(^([a-zA-Z0-9_]+)/((?:msg|srv|action)?)/?([a-zA-Z0-9_]+)$)"};
Copy link
Member

Choose a reason for hiding this comment

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

the / after the subtype name should not be optional

Comment on lines 210 to 217
// These type definitions may include additional fields such as the goal_id.
// See also https://design.ros2.org/articles/actions.html
const std::map<std::string, std::string> action_type_definitions = {
{"_FeedbackMessage", "unique_identifier_msgs/UUID goal_id\n" + feedbackDef},
{"_GetResult_Request", "unique_identifier_msgs/UUID goal_id\n"},
{"_GetResult_Response", "int8 status\n" + resultDef},
{"_SendGoal_Request", "unique_identifier_msgs/UUID goal_id\n" + goalDef},
{"_SendGoal_Response", "bool accepted\nbuiltin_interfaces/msg/Time stamp"}};
Copy link
Member

Choose a reason for hiding this comment

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

The linked design doc discusses these fields at a high level, but where is it documented that these are actually added to the definitions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also have not found proper documentation for that. Best documentation I could find is the source code that generates the action types:

https://github.com/ros2/rosidl/blob/a57baea5dee6108246db9991e39d28a10a7ee2b0/rosidl_parser/rosidl_parser/definition.py#L720-L727


// Create a MessageSpec instance for every action subtype and add it to the cache.
for (const auto& [action_suffix, definition] : action_type_definitions) {
DefinitionIdentifier definitionIdentifier;
Copy link
Member

Choose a reason for hiding this comment

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

I see mixed snake & camel case in this file, can we standardize on one?


// The action type name includes the subtype which we have to remove to get the action name.
// Type name: Fibonacci_FeedbackMessage -> Action name: Fibonacci
const std::string action_name = type_name.substr(0UL, type_name.find('_'));
Copy link
Member

Choose a reason for hiding this comment

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

do we need to handle type names that also contain underscores? do we need to verify that the suffix we stripped off is actually one of the suffixes we support?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we should. Will do that

Comment on lines 126 to 128
constexpr char SEP[] = "---";

const auto definitions = split_string(action_definition, SEP);
Copy link
Member

Choose a reason for hiding this comment

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

What if one of the segments has a string constant with --- in it? e.g. string FOO=--- ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

Seems alright. I do think we should put this behind an explicit cli flag because it's a niche/advanced/experimental use case for which we don't provide a great UX in studio.

@achim-k
Copy link
Collaborator Author

achim-k commented Apr 14, 2023

Seems alright. I do think we should put this behind an explicit cli flag because it's a niche/advanced/experimental use case for which we don't provide a great UX in studio.

I have submitted #216 for that purpose. Separate PR, since certain hidden action topics (e.g. .../_action/status) already got advertised before this PR

@achim-k achim-k merged commit f02099d into main Apr 14, 2023
@achim-k achim-k deleted the achim/action_type_support branch April 14, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants