-
Notifications
You must be signed in to change notification settings - Fork 133
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 rosidl_generator_cpp templates for actions #312
Conversation
db03640
to
819c976
Compare
819c976
to
c7352db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but for a few nits plus pending CI.
#include <action_msgs/msg/goal_status_array.hpp> | ||
#include <@(spec.pkg_name)/action/@(get_header_filename_from_msg_name(spec.action_name))__feedback.hpp> | ||
#include <@(spec.pkg_name)/action/@(get_header_filename_from_msg_name(spec.action_name))__goal.hpp> | ||
#include <@(spec.pkg_name)/action/@(get_header_filename_from_msg_name(spec.action_name))__result.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apojomovsky I think we should be consistent with rosidl_generator_c equivalent template: either we hard-code action
or we pass in @(subfolder)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, done!
|
||
using Goal = GoalRequestService::Request; | ||
using Result = GoalResultService::Response; | ||
using Feedback = @(spec.pkg_name)::action::@(spec.action_name)_Feedback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apojomovsky same thing about action
vs. @(subfolder)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guys, I found a few more things here and there while digging.
add_dependencies( | ||
${rosidl_generate_interfaces_TARGET} | ||
${rosidl_generate_interfaces_TARGET}${_target_suffix} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apojomovsky @sloretz I don't entirely follow why we check for ${rosidl_generate_action_interfaces_TARGET}
but use ${rosidl_generate_interfaces_TARGET}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy/paste error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2ed5832
add_dependencies( | ||
${rosidl_generate_interfaces_TARGET} | ||
${rosidl_generate_interfaces_TARGET}${_target_suffix} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apojomovsky @sloretz same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2ed5832
"${rosidl_generator_c_TEMPLATE_DIR}/rosidl_generator_c__action_visibility_control.h.in" | ||
"${_visibility_control_file}" | ||
@ONLY | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apojomovsky @sloretz seems we're not installing the generated _visibility_control_file
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installed visibility file in db68926
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As just mentioned here, the latest changes from @sloretz in ros2/rosidl_typesupport#36 in conjunction with the ones here enabled us to successfully build/run the |
Addresses parts of #303
Built on top of sloretz/speficication-to-str which is in turn based on sservulo/305-parse_action_file
This PR:
action__struct.hpp.em
andaction.hpp.em
in therosidl_generator_cpp
's resource directory.action__struct.h.em
andaction.h.em
in therosidl_generator_c
's resource directory.rosidl_generator_cpp
androsidl_generator_c
logic to process.action
files.ActionSpecification
class to therosidl_parser
.EDIT:
It wasn't possible to add a
test_action_initialization
test because of an implicit dependency tobuiltin_interfaces
package in the generated c++ code which ended up in a circular dependency with the package.Since this can't be tested in the generation pipeline yet, I've extended some of my notes in this gist with some instructions on how to run the generator by hand against an example package.
connects to ros2/rcl_interfaces#48