Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Select between introspection_c and introspection_cpp #133

Merged
merged 1 commit into from
Feb 29, 2016

Conversation

jacquelinekay
Copy link
Contributor

These changes are necessary to use rcl with Connext Dynamic.

@jacquelinekay jacquelinekay added the in progress Actively being worked on (Kanban column) label Feb 12, 2016
find_package(rosidl_typesupport_introspection_cpp)
if(NOT rosidl_typesupport_introspection_cpp_FOUND)
if(NOT rosidl_typesupport_introspection_cpp_FOUND AND NOT rosidl_typesupport_introspection_c)
Copy link
Member

Choose a reason for hiding this comment

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

Lacks _FOUND suffix?

@wjwwood
Copy link
Member

wjwwood commented Feb 12, 2016

screen shot 2016-02-12 at 2 41 40 pm

lol

@jacquelinekay
Copy link
Contributor Author

That's how we roll.

@jacquelinekay
Copy link
Contributor Author

I'm starting CI for this to see that it doesn't break anything. Locally testing with test_rclcpp worked.

http://ci.ros2.org/job/ci_linux/978
http://ci.ros2.org/job/ci_osx/813
http://ci.ros2.org/job/ci_windows/1066

I haven't tested this with C yet though. I'll make a differently named branch to enable building rcl for Connext Dynamic.

@jacquelinekay
Copy link
Contributor Author

Alright, I fixed some stuff with strings and started a branch in rcl that enables the dynamic test again.

http://ci.ros2.org/job/ci_linux/982

@jacquelinekay
Copy link
Contributor Author

I made some style fixes, relaunched with only Connext Dynamic enabled (hope that works).

http://ci.ros2.org/job/ci_linux/983/

@jacquelinekay
Copy link
Contributor Author

@jacquelinekay
Copy link
Contributor Author

Fixed a warning on OSX and Windows.

http://ci.ros2.org/job/ci_osx/827/

I think the failed tests on Windows also appear on the nightly build. I think we should fix those failures before merging this, but we should start review on this now.

I separated the new macros into a macros.hpp file so that the diff isn't impossible to view in the browser.

@jacquelinekay jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 16, 2016
if (using_introspection_c_typesupport(typesupport)) {
CREATE_TYPENAME_PREFIX(INTROSPECTION_C_TYPE)
} else if (using_introspection_cpp_typesupport(typesupport)) {
CREATE_TYPENAME_PREFIX(INTROSPECTION_CPP_TYPE)
Copy link
Member

Choose a reason for hiding this comment

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

Since both macros have the code to access the fields of members in common I would suggest to let the macro only define the members variable and continue to handle accessing the fields package_name_ and message_name_ outside.

Copy link
Member

Choose a reason for hiding this comment

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

Then the macro could also be renamed to something like GET_TYPED_MEMBERS.

@tfoote tfoote added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Feb 16, 2016
@jacquelinekay
Copy link
Contributor Author

After some thought I created a branch with a different approach that is more concise than this one (although I'm still not sure if it's the best):

https://github.com/ros2/rmw_connext/compare/introspection_template

Unfortunately I'm running into a compile error that I can't quite figure out, due to 7e8232e. Looks like feature isn't going to make it into this release, if the pull request in the current state is stylistically unacceptable.

@wjwwood
Copy link
Member

wjwwood commented Feb 17, 2016

@jacquelinekay thanks for trying to get it in. If you need someone to review it, then I can lend a hand after the alpha. We can try to get it merged asap after the release.

@jacquelinekay jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 18, 2016
@jacquelinekay
Copy link
Contributor Author

alright, I took a pretty different approach that is more concise and I reset this branch to reflect it. (the old approach is still around, in https://github.com/ros2/rmw_connext/compare/introspection_cpp_old)

http://ci.ros2.org/job/ci_linux/1020/
http://ci.ros2.org/job/ci_osx/846/
http://ci.ros2.org/job/ci_windows/1087/


bool using_introspection_c_typesupport(const char * typesupport_identifier)
{
return strcmp(typesupport_identifier, rosidl_typesupport_introspection_c__identifier) == 0;
Copy link
Member

Choose a reason for hiding this comment

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

The should not use string comparison since it is a pretty expensive operation. See the previously used macro: https://github.com/ros2/rmw/blob/55254b66ddb0f7c5ff8f05fb8c0c60378fb3b2da/rmw/include/rmw/impl/cpp/macros.hpp#L70

Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

everything here is a const char *, so using the equality operator should be okay.

return _create_type_name<rosidl_typesupport_introspection_cpp::MessageMembers>(untyped_members,
sep);
}
RMW_SET_ERROR_MSG("service members handle is null");
Copy link
Member

Choose a reason for hiding this comment

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

The error message doesn't match the error case.

Same below.

@dirk-thomas
Copy link
Member

To simplify the code further I think there should be only one list of field types (in C) and C++ should use the same constant.

The C++ part could still define "nicer" constants in a namespace (based on the C constants) but the rmw impl. would only need to deal with the C constants.

@dirk-thomas
Copy link
Member

Overall this looks much nicer then the previous branch.

@jacquelinekay
Copy link
Contributor Author

I agree that the C/C++ introspection APIs should share some types; I see no reason why they need separate definitions--that was the most frustrating thing about making this change...

I suppose the current scheme is better if we wanted to avoid a dependency of rosidl_typesupport_introspection_cpp on rosidl_typesupport_introspection_c, but I think most dynamic RMW implementations will want to support both C and C++ introspection and will therefore depend on both packages. So introducing a dependency between the two introspection packages is not a big deal.

@jacquelinekay
Copy link
Contributor Author

Oh, I understand now, changes to the introspection packages aren't necessarily needed, I believe the field type enums from either package can be used in the switch statements that I currently have macros for.

@jacquelinekay
Copy link
Contributor Author

I realized why sharing the enum definitions would be nice to have. ros2/rosidl#94

@jacquelinekay
Copy link
Contributor Author

CI for the new approach, with only connext dynamic enabled.

http://ci.ros2.org/job/ci_linux/1029
http://ci.ros2.org/job/ci_osx/848/
http://ci.ros2.org/job/ci_windows/1088/

@tfoote tfoote mentioned this pull request Feb 24, 2016
25 tasks
@esteve
Copy link
Member

esteve commented Feb 26, 2016

+1

@jacquelinekay
Copy link
Contributor Author

Since @esteve gave his +1 I'll merge these PRs today unless there are any objections.

@jacquelinekay jacquelinekay force-pushed the introspection_cpp branch 2 times, most recently from 5672492 to 23c6170 Compare February 29, 2016 17:37
jacquelinekay added a commit that referenced this pull request Feb 29, 2016
Select between introspection_c and introspection_cpp
@jacquelinekay jacquelinekay merged commit dd9a720 into master Feb 29, 2016
@jacquelinekay jacquelinekay deleted the introspection_cpp branch February 29, 2016 18:58
@jacquelinekay jacquelinekay removed the in review Waiting for review (Kanban column) label Feb 29, 2016
@esteve
Copy link
Member

esteve commented Feb 29, 2016

🎆 🎉 🎈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants