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

ROSIDL - Support generating files for fastddsgen without multiple definition errors #767

Open
Ryanf55 opened this issue Sep 8, 2023 · 6 comments
Labels

Comments

@Ryanf55
Copy link

Ryanf55 commented Sep 8, 2023

Bug report

Required Info:

  • Operating System:
    -Ubuntu 22
  • Installation type:
    • Humble binaries
  • Version or commit hash:
    • 3.1.5-2jammy.20230718.201832 amd64
  • DDS implementation:
    • Fast-DDS
  • Client library (if applicable):
    • N/A

Steps to reproduce issue

mkdir -p IDL/std_msgs/msg/
# Same for others
cp /opt/ros/humble/share/std_msgs/msg/Header.idl IDL/std_msgs/msg/
cp /opt/ros/humble/share/builtin_interfaces/msg/Time.idl IDL/builtin_interfaces/msg/
cp /opt/ros/humble/share/geographic_msgs/msg/GeoPoint.idl IDL/geographic_msgs/msg/
cp /opt/ros/humble/share/vision_msgs/msg/BoundingBox2D.idl IDL/vision_msgs/msg/
cp /opt/ros/humble/share/vision_msgs/msg/Pose2D.idl IDL/vision_msgs/msg/
cp /opt/ros/humble/share/vision_msgs/msg/Point2D.idl IDL/vision_msgs/msg/

fastddsgen -cs -replace -typeros2 -d gen/builtin_interfaces/msg -I IDL IDL/builtin_interfaces/msg/Time.idl
# Repeat for all the other files

OR, use ros2 run rosidl_adapter msg2idl.py /opt/ros/humble/share/std_msgs/msg/Header.msg to generate the IDL file into a directory if you want to work off the ROS messages, you get the same idl files.

Expected behavior

The generated IDL files do not have naming conflicts.

Actual behavior

[build] /ws/IDL/builtin_interfaces/msg/Time.idl:8:16: error: Illegal identifier: builtin_interfaces::msg::Time is already defined (Definition: com.eprosima.idl.parser.tree.TypeDeclaration@5b239d7d)

Illegal identifier: std_msgs::msg::Header is already defined (Definition: com.eprosima.idl.parser.tree.TypeDeclaration@515aebb0)

Additional information

Causes this downstream issue because the IDL file has conflicts.

eProsima/Fast-DDS-Gen#52

@clalancette
Copy link
Contributor

Just so you know, we don't use fastddsgen when generating code from the IDL files; the ROS 2 core does this itself. So this isn't functionality that is usually used.

That said, if you find a way to make this work we'd be happy to review a fix.

@Ryanf55
Copy link
Author

Ryanf55 commented Sep 8, 2023

Just so you know, we don't use fastddsgen when generating code from the IDL files; the ROS 2 core does this itself. So this isn't functionality that is usually used.

That said, if you find a way to make this work we'd be happy to review a fix.

In a quick test, looks like a simple #pragma once at the top of every single ROS-generated IDL solved the problem on the EProsima side. I know from some research RTI supports it. Other vendors will need to be tested.

What's interesting is there are notes about this issue in the OMG IDL spec, but no proposed solution. #pragma once is not anywhere in the OMG IDL DDS spec, so I'm concerned it's not standard.

Thoughts?

@clalancette
Copy link
Contributor

In a quick test, looks like a simple #pragma once at the top of every single ROS-generated IDL solved the problem on the EProsima side. I know from some research RTI supports it. Other vendors will need to be tested.

What's interesting is there are notes about this issue in the OMG IDL spec, but no proposed solution. #pragma once is not anywhere in the OMG IDL DDS spec, so I'm concerned it's not standard.

Yes, that's a definite concern. So we'll have to see what happens with other vendors, particularly CycloneDDS and GurumDDS.

I'll note that without additional work in rosidl, adding this won't have any effect for ROS 2 itself. But if this helps others use fastddsgen and the like outside of ROS 2, then it is definitely something we can consider adding.

@emersonknapp
Copy link
Collaborator

To be even more specific, the OMG IDL spec says

IDL shall be preprocessed according to the specification of the preprocessor in ISO/IEC 14882:2003.

That ISO is the C++03 spec, which says that

[A #pragma directive] causes the implementation to behave in an implementation-defined manner. Any pragma that is not recognized by the implementation is ignored.

That all to say, a spec-compliant IDL implementation is not required to implement #pragma once - which is why I never get to use this handy little doodad in my C++ code, "just in case" :)

@Ryanf55
Copy link
Author

Ryanf55 commented Sep 13, 2023

To be even more specific, the OMG IDL spec says

IDL shall be preprocessed according to the specification of the preprocessor in ISO/IEC 14882:2003.

That ISO is the C++03 spec, which says that

[A #pragma directive] causes the implementation to behave in an implementation-defined manner. Any pragma that is not recognized by the implementation is ignored.

That all to say, a spec-compliant IDL implementation is not required to implement #pragma once - which is why I never get to use this handy little doodad in my C++ code, "just in case" :)

As far as I can tell, all the compilers for ROS tier 1 platforms support it.

@com-server-ap
Copy link

com-server-ap commented Aug 1, 2024

Up to now, the issue has not been resolved. just like Ryanf55 said: add #pragma once at the top of every idl, can ignore the issue.

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

No branches or pull requests

4 participants