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

msg2idl.py generates incorrect idl #706

Open
bijoua29 opened this issue Sep 24, 2022 · 9 comments
Open

msg2idl.py generates incorrect idl #706

bijoua29 opened this issue Sep 24, 2022 · 9 comments
Labels
backlog enhancement New feature or request

Comments

@bijoua29
Copy link

bijoua29 commented Sep 24, 2022

Bug report

Required Info:

  • Operating System: Ubuntu 22.04
  • Installation type: binaries
  • Version or commit hash: N/A
  • DDS implementation: default Fast-RTPS
  • Client library (if applicable): N/A

Steps to reproduce issue

Run msg2idl on following msg file: ros2 run rosidl_adapter msg2idl.py State.msg

State.msg:
bool enabled

Expected behavior

Expected output is:

module my_interfaces {
module msg {
module dds_ {
struct State_ {
boolean enabled;
};
};
};
};

Actual behavior

Actual output is:

module my_interfaces {
module msg {
struct State {
boolean enabled;
};
};
};

Additional information

The actual output looks like any older format and doesn't have the additional nesting and underscores.

@clalancette
Copy link
Contributor

The IDL that it generates is what is used by the ROS 2 tooling. So it is performing as expected currently.

Why do you think it should be generating something different? Can you explain more about your use case?

@bijoua29
Copy link
Author

bijoua29 commented Sep 26, 2022

The IDL that it generates is what is used by the ROS 2 tooling. So it is performing as expected currently.

Why do you think it should be generating something different? Can you explain more about your use case?

OK, my use case is that I am trying to connect my ros2 application to a native DDS application. I need the IDL that is used on the wire so I can compile that IDL in my native DDS application. At least, on the wire, the data looks like what I indicated in the expected behavior IDL above i.e. the additional nesting of dds_ and the type has an underscore after it:

module my_interfaces {
module msg {
module dds_ {
struct State_ {
boolean enabled;
};
};
};
};

This problem has been seen earlier by others both in CycloneDDS and ConnextDDS where people have to "massage" the IDL to get it to work.

@clalancette
Copy link
Contributor

OK, my use case is that I am trying to connect my ros2 application to a native DDS application.

OK, yeah, that's kind of what I expected. This is indeed currently a problem, which will require changing both what rosidl generates and what the rest of the ROS 2 tooling consumes.

Note that this is not the only problem with making ROS 2 interoperate with native DDS; see my joint talk at ROSCon 2021 about some of the other issues.

If you'd like to take a stab at fixing it, we'd be happy to review some patches. But we aren't likely to be spending much time on it in the near future.

@clalancette clalancette added backlog enhancement New feature or request labels Sep 26, 2022
@bijoua29
Copy link
Author

@clalancette Thanks for the response. Yes, I had listened to your joint talk at ROSCon 2021 earlier and understand the limitations in interfacing to native DDS including keyed fields as well as the conventions that ROS 2 follows.

Since we have control over our native DDS application, we can modify it to follow the ROS 2 conventions, at least the parts that interface to ROS 2. The problem I see is that we need the correct IDL to start with in our native DDS application. BTW, I have been able to communicate natively to a ConnextDDS application as long as I manually "fix" the generated IDL and use compatible QoS settings. I just don't want to manually fix the IDL every time.

Even in your talk, you mention that ROS 2 types are formatted as ::msg::dds_::. But this is not the IDL that msg2idl.py generates. Is there a different IDL used for tooling versus what's generated for the middleware? I understand the goal was to support non-DDS middleware as well, so does the DDS middleware add this additional dds nesting?

I wouldn't mind taking a stab at fixing this, but I wouldn't know where to start and what parts are affected.

@clalancette
Copy link
Contributor

Even in your talk, you mention that ROS 2 types are formatted as ::msg::dds_::_. But this is not the IDL that msg2idl.py generates. _

Ah, now I see what you mean. It looks like this changed between when I wrote my talk a year ago and in Humble (and now Rolling). You are right that it no longer has the dds_ component in it.

Is there a different IDL used for tooling versus what's generated for the middleware?

No, it's the same. Just for whatever reason, it has changed since my talk. Probably the first part here would be to figure out when that changed, and what the reason for changing it was. I would probably poke around in the history of this repository to start with to see if I could find that.

@bijoua29
Copy link
Author

When did the idl change to remove the dds_ component? I am on the latest rolling from the last sync and it still has the dds_ component. Unless it changed within the last month which I don't see in the change history.

When I try to interface to ConnextDDS, the Connext tools show that the dds_ component is still there. And this shows someone experiencing the same when trying to interface to native CycloneDDS just in the past few days. So I'm still confused if/when the IDL has changed.

I did check and my version of rosidl-adapter is 3.2.1 dated 7/11/22. It is not that recent but it's what was released in the latest sync.

@bijoua29
Copy link
Author

Just updated to latest rolling sync from 2022-09-29 and the rosidl-adapter version is now updated to latest 3.3.0 but still the same behavior.

@bijoua29
Copy link
Author

@clalancette Wondering if you can point me to where the disconnect is between what is on the wire(with the additional dds_ component) and what msg2idl.py generates which is ::msg:: instead of::msg::dds_? I would have assumed msg2idl is what is used by the rmw and the tooling but that doesn't seem to be the case. I'll be glad to look at the code if I knew where to look e.g. where is the idl generated that is used by the rmw.

@TrevorGibson-SR
Copy link

TrevorGibson-SR commented Nov 15, 2022

I'm working with @bijoua29 on this problem and have a related question. Please let me know if this isn't the right location to ask this.

Update on attempt to solve the original problem:
I have forked the rosidl repo into our project and made a few modifications to accommodate the IDL format for our external application. This works as expected when generating IDL files using ros2 run rosidl_adapter msg2idl.py <msg_file_name>.

I'm running into difficulty trying to use these modifications as part of our colcon build process, to automatically generate IDL files when building packages that contain .msg files. Currently, we're using the rosidl_generate_interfaces CMake function from the rosidl_cmake package. We have a dependency for rosidl_default_generators in the package with .msg files, which seems to appropriately reference our forked version of rosidl.

Related problem:
However, the python call inside the rosidl_adapt_interfaces function always references the default rosidl_adapter package included with our ROS installation instead of the locally modified version. The code in question is on line 47 of rosidl_adapt_interfaces.cmake, specifically the python call "${python_interpreter}" -m rosidl_adapter.

Does the build environment have a different python search path than the shell used to initiate the build? It seems that the -m option is not finding our local version of rosidl_adapter and instead using the ROS default version.

Update (problem fixed)
Just wanted to close the loop for anyone who comes across my issue in the future. My issue was solved by changing the dependencies for the package containing the .msg files. The original implementation had rosidl_default_generators as the only IDL dependency, but that apparently referenced the ROS default version of that package since I didn't have a locally modified copy in my workspace. Removing that dependency and adding rosidl_adapter and rosidl_cmake (both of which I have local copies of in my workspace) allowed the python call in rosidl_adapt_interfaces() to reference the correct package(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants