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

change generators to IDL-based pipeline #15

Merged
merged 14 commits into from
Mar 12, 2019
Merged

change generators to IDL-based pipeline #15

merged 14 commits into from
Mar 12, 2019

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Nov 21, 2018

Connected to ros2/rosidl#334.

@dirk-thomas dirk-thomas added the in progress Actively being worked on (Kanban column) label Nov 21, 2018
@dirk-thomas dirk-thomas self-assigned this Nov 21, 2018
@dirk-thomas dirk-thomas force-pushed the idl-stage-7 branch 4 times, most recently from ae4e5a5 to efb8f9b Compare November 24, 2018 21:03
@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 26, 2018
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Is this PR lacking the checks to support actions that both FastRTPS and OpenSplice will have? Why?

@dirk-thomas
Copy link
Member Author

Is this PR lacking the checks to support actions that both FastRTPS and OpenSplice will have? Why?

The generator will be refactored to use the IDL-based pipeline so no need to add this temporarily.

@jwillemsen
Copy link
Contributor

Wouldn't a C++ template approach reduce the need for vendor specific code, have one generation from msg to idl that is generic and provide a set of traits to the template instantiation to work with a specific vendor? The amount of duplicated code between DDS vendors looks huge to me

@dirk-thomas
Copy link
Member Author

Wouldn't a C++ template approach reduce the need for vendor specific code, have one generation from msg to idl that is generic and provide a set of traits to the template instantiation to work with a specific vendor?

@jwillemsen While certainly possible it is out-of-scope for the IDL work in this PR. The amount of changes to switch the pipeline to use IDL is already extremely large. But after that has landed we would happily take pull requests to consolidate these parts of the code base.

dirk-thomas and others added 7 commits February 5, 2019 10:51
* Switch C++ type support generation pipeline.

* Adds outer C++ IDL templates.

* Adapts C++ msg & srv header templates.

* Adapts C++ msg & srv source templates.

* Switch C type support generation pipeline.

* Adds outer C IDL templates.

* Adapts C msg & srv header templates.

* Adapts C msg source template.

* Adapts C msg & srv source templates.

* Adds missing Python imports in templates.

* Converts IDL files to RTI Connext IDL files.

* Fixing typesupport generation issues.

* Uses rosidl_dds again for IDL2IDL conversion.

* Applies many fixes to C & C++ typesupport.

* Fixes for C++ srv templates.

* Many fixes to C & C++ type support.

* Yet more fixes to C & C++ type support.

* Fixes copyright dates.

* Adds back attribute to generated code constants.

* Adds missing type support for action messages.
@dirk-thomas dirk-thomas mentioned this pull request Feb 12, 2019
8 tasks
@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Feb 15, 2019
@dirk-thomas dirk-thomas changed the title update includes and type mapping change generators to IDL-based pipeline Feb 19, 2019
@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 21, 2019
@dirk-thomas dirk-thomas requested a review from hidmic February 21, 2019 23:23
Copy link
Contributor

@hidmic hidmic left a 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 minor comments and pending green CI

@[ elif field.type.is_primitive_type()]@
dds_message.@(field.name)_[static_cast<DDS_Long>(i)] =
ros_message.@(field.name)[i];
@[ if isinstance(member.type.basetype, BaseString)]@
Copy link
Contributor

Choose a reason for hiding this comment

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

@dirk-thomas just to double check, DDS_String_* functions operate with wchar_t-based std::base_string instances, correct? Otherwise, consider checking for String and asserting on WString.

dds_message.@(member.name)_[static_cast<DDS_Long>(i)]@(' == DDS_BOOLEAN_TRUE' if member.type.basetype.type == 'boolean' else '');
@[ elif isinstance(member.type.basetype, BaseString)]@
ros_message.@(member.name)[i] =
dds_message.@(member.name)_[static_cast<DDS_Long>(i)];
Copy link
Contributor

Choose a reason for hiding this comment

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

@dirk-thomas same comment about wchar_t strings present or not.

@dirk-thomas dirk-thomas merged commit 409f82e into master Mar 12, 2019
@dirk-thomas dirk-thomas deleted the idl-stage-7 branch March 12, 2019 04:11
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 12, 2019
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.

4 participants