-
Notifications
You must be signed in to change notification settings - Fork 797
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
Binding a Fast RTPS publisher with a ROS 2 Dashing subscriber node fails #829
Comments
@richiprosima friendly ping :) |
I think I might have a clue of what is happening. Using Cannot echo topic '/SensorCombined_PubSubTopic', as it contains more than one type: [px4_msgs::msg::SensorCombined, px4_msgs/msg/SensorCombined] Where $ ros2 node info /sensor_combined_listener
/sensor_combined_listener
Subscribers:
/SensorCombined_PubSubTopic: px4_msgs/msg/SensorCombined
/parameter_events: rcl_interfaces/msg/ParameterEvent
Publishers:
/parameter_events: rcl_interfaces/msg/ParameterEvent
/rosout: rcl_interfaces/msg/Log
Services:
/sensor_combined_listener/describe_parameters: rcl_interfaces/srv/DescribeParameters
/sensor_combined_listener/get_parameter_types: rcl_interfaces/srv/GetParameterTypes
/sensor_combined_listener/get_parameters: rcl_interfaces/srv/GetParameters
/sensor_combined_listener/list_parameters: rcl_interfaces/srv/ListParameters
/sensor_combined_listener/set_parameters: rcl_interfaces/srv/SetParameters
/sensor_combined_listener/set_parameters_atomically: rcl_interfaces/srv/SetParametersAtomically This leads me to think that the way that the agent and the This is an issue, but I suspect that I might have to adjust the QoS params as well, unless Thanks in advance for your help! |
@dirk-thomas do you have any tips on the above? |
Dashing has its own IDL generator for FastRTPS. But px4_ros_com is using fastrtpsgen to generate type support. It seems these generators generate different typename. The best solution will be px4_ros_com uses current Dashing IDL generator to generate typesupport instead of fastrtpsgen. |
@richiware I am using the IDL's generated from the ROS IDL generator on fastrtpsgen to generate the typesupport. How do you suggest to use the ROS typesupport generator? I mean, px4_msgs already generates the typesupport for FastRTPS, but how can one use it on the microRTPS instead of the ones generated by fastrtpsgen? Does one have to replace https://github.com/PX4/px4_ros_com/blob/master/cmake/GenerateMicroRTPSAgent.cmake#L175-L178 with the typesupport headers generated by |
Update: Looking at One main difference between what was being done on Crystal and now on Dashing for |
Unfortunately I am not able to use the IDL generator given by [ 13%] Built target px4_msgs__rosidl_generator_c
[ 13%] Generating C introspection for ROS interfaces
No terminal defined for 'u' at line 1 col 1
uint64 timestamp # time since system sta
^
Expecting: {'CONST', 'MODULE', 'ENUM', 'STRUCT', 'TYPEDEF', '__ANON_9', 'AT'}
/home/nuno/PX4/px4_ros_com_ros2/src/px4_msgs/msg/ActuatorArmed.msg
Error processing idl file: /home/nuno/PX4/px4_ros_com_ros2/src/px4_msgs/msg/ActuatorArmed.msg
Traceback (most recent call last):
File "/opt/ros/dashing/lib/rosidl_generator_dds_idl/rosidl_generator_dds_idl", line 41, in <module>
sys.exit(main())
File "/opt/ros/dashing/lib/rosidl_generator_dds_idl/rosidl_generator_dds_idl", line 36, in main
args.additional_service_templates,
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_generator_dds_idl/__init__.py", line 51, in generate_dds_idl
generate_files(generator_arguments_file, mapping, additional_context, keep_case=True)
File "/home/nuno/PX4/rosidl_ws/install/rosidl_cmake/lib/python3.6/site-packages/rosidl_cmake/__init__.py", line 94, in generate_files
raise(e)
File "/home/nuno/PX4/rosidl_ws/install/rosidl_cmake/lib/python3.6/site-packages/rosidl_cmake/__init__.py", line 73, in generate_files
idl_file = parse_idl_file(locator)
File "/home/nuno/PX4/rosidl_ws/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 64, in parse_idl_file
content = parse_idl_string(string, png_file=png_file)
File "/home/nuno/PX4/rosidl_ws/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 72, in parse_idl_string
tree = get_ast_from_idl_string(idl_string)
File "/home/nuno/PX4/rosidl_ws/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 89, in get_ast_from_idl_string
return _parser.parse(idl_string)
File "/usr/lib/python3/dist-packages/lark/lark.py", line 306, in parse
return self.parser.parse(text, start=start)
File "/usr/lib/python3/dist-packages/lark/parser_frontends.py", line 182, in parse
return self._parse(text, start)
File "/usr/lib/python3/dist-packages/lark/parser_frontends.py", line 54, in _parse
return self.parser.parse(input, start, *args)
File "/usr/lib/python3/dist-packages/lark/parsers/earley.py", line 293, in parse
self._parse(stream, columns, to_scan, start_symbol)
File "/usr/lib/python3/dist-packages/lark/parsers/xearley.py", line 137, in _parse
to_scan = scan(i, to_scan)
File "/usr/lib/python3/dist-packages/lark/parsers/xearley.py", line 114, in scan
raise UnexpectedCharacters(stream, i, text_line, text_column, {item.expect.name for item in to_scan}, set(to_scan))
lark.exceptions.UnexpectedCharacters: No terminal defined for 'u' at line 1 col 1
uint64 timestamp # time since system sta
^
Expecting: {'CONST', 'MODULE', 'ENUM', 'STRUCT', 'TYPEDEF', '__ANON_9', 'AT'} |
I don't have any input in this beyond the content that you need to use compatible types. Atm the ROS typesupport for FastRTPS doesn't use I also don't know enough about micro and why it would not just use the same types already existing in ROS. |
@richiware can you please comment on this? I mean, at this stage, I don't understand as well why would this result on incompatible types. I was with the idea that any Fast-RTPS participant would be capable of communicating between each other, and while ROS is using This only means that currently, any Fast-RTPS application using fastrtpsgen to generate its typesupport for its messages is basically incompatible with with any ROS2 node. Which ends up being quite awkward, given that Fast-RTPS started as the default middleware for ROS2. For the implementation of the microRTPS to work, at this stage, with Dashing, it requires that the full code to be ported as a ROS 2 node, which at this stage is not on the plans and it was not something that one should have to be concerned about, as by default we would expect that two participants on a DDS domain, even more from the same vendor, would be communicating correctly between each other. @richiprosima @LuisGP (or any other eProsima team member) please advise when able. Thank you! Note: Keep in mind that I can correctly bind a Publisher generated with fastrtpsgen of Fast-RTPS version 1.7.2 and a ROS2 Crystal Subscriber node. The same I cannot say with Fast-RTPS-Gen v1.0.2 (now out of the Fast-RTPS repo) and the same node but running under ROS2 Dashing interfaces and typesupport. |
I was investigating a little more, fastrtpsgen should generate the type's name like the ROS2 IDL generator. fastrtpsgen also creates the type's name using |
I can give you more information. New module px4_msgs
{
module msg
{
struct SensorCombined
{
. . .
};
};
}; With this IDL But for Connext and Opensplice, ROS2 is still generating the old-way IDL files. For module px4_msgs
{
module msg
{
module dds_
{
struct SensorCombined_
{
. . .
};
};
};
}; @dirk-thomas Any suggestion? |
There are two conflicting parts in the IDL generation / type naming:
That is why the IDL files generated for each vendor use the extra module |
Thanks, @dirk-thomas. @TSC21 For Crystal you should be using the IDL files generated by rosidl_dds for Opensplice to generate code with fastrtpsgen, shouldn't you? In this case, for Dashing you should use also these IDL files instead of the ones generated by rosidl_adapter. |
@richiprosima no I am not. As you can see on the history of this issue, in Crystal and bellow I use
@dirk-thomas where exactly are these being stored? I don't see any |
The type support packages for FastRTPS don't need custom You should find the exact path of the |
I know. But then we get back to the exact same problem that is being reported. |
@dirk-thomas Changing the type name in code, adding I don't' know if it was a decision you made, but if you want to solve this interoperability problem I think the types defined in the IDL file generated by |
@dirk-thomas is there a way we can find a compatible solution for this? Otherwise, it's impossible to have a bind between ROS 2 DDS participants and other participants from Fast-RTPS applications. |
It is unlikely that I will have the bandwidth to drive that development. But I am more than happy to review any proposals and help out. Any proposed solution still needs to make sure that cross-vendor communication continues to work and that the generated types (from ROS and the different vendors) can coexist. |
Considering that the problem here is that we have incompatible types for the same vendor, I think that's reason enough to consider it already a problem. Besides, @dirk-thomas I understand that you may not have the time, but considering that this circumstance actually breaks in-vendor compatibility under the same DDS domain, I think this is of the most importance to actually fix it - otherwise the problem will continue to exist and will never be solved. At least some guidance on how to approach it would be fine (readding the previous behaviour for @richiprosima do you have anything else to add here as well? Thank you both! |
@dirk-thomas @tfoote @richiware as Crystal is becoming EOL, it's becoming essential that this gets solved as soon as possible. This reported problem is, IMO, a direct violation of the wire protocol interoperability that should be respected for a DDS compliant implementation, given that RTPS is not being made compatible between two implementations and typesupport generations for the same vendor. If you don't have time to fix this issue, I ask you to please provide insights on how you propose this to be solved. I suggested above that, at least, If you feel there's another way of quickly solving this, please let me know. In the mean time, I ask you to please to provide some insights on how we could handle this incompatibility in a way we don't have to completely change the microRTPS bridge implementation. It's also our priority to support and use what ROS 2 has to offer, but with Crystal becoming EOL, that will be at risk. Thanks in advance! @LorenzMeier, @thomasgubler, @jkflying, FYI. |
@TSC21/Auterion are happy to support implementation but we want to avoid implementing in the wrong direction that afterwards can not get merged. Could you provide guidance? Would you maybe be available for a call this Thu or Fr your morning (PST)? |
If I understand correctly the problem is because of the generated type name. I can think about several workarounds, but for the future, the way to go is using Micro-ROS as the Bridge. I am available also to attend the call. |
@JaimeMartin we are not planning at this point to move to microROS. The paradigm will completely change in terms of what we currently view as the standard bridging between the companion computer and PX4 (it wouldn't be a bridge between uORB and RTPS, but would rather tend to replace uORB) and we are not planning to replace uORB, at least from what I know and discussed with the dev team and the stakeholders. |
@JaimeMartin if you know about "workarounds", I ask you to please suggest one so we can work through it. Thank you! |
@TSC21 The Micro-ROS client does not replace the uORB, that is not the idea. We developed micro-RTPS bridge as the initial PoC for what is now Micro-ROS. In that sense, it is outdated, and the way to go is Micro-ROS. Also, for your issue, you have functions as https://github.com/eProsima/Fast-RTPS/blob/1.9.x/include/fastrtps/TopicDataType.h#L88 you could use to change the name. |
Where exactly is all of this documented? If we are to consider using micro-ROS and the Micro-XRCE capabilities, I need to first understand if it makes sense to have an MVP based on how well documented the API is so it can be used and integrated in short time. Otherwise, it doesn't make sense to replace the bridge. Also, it's not deprecated, it has actually been maintained and updated to our needs. What was a PoC is no longer a a PoC and it's actually performing well and fitting what we require.
Right, thanks for this. I am going to try this and let you know. Though, I am still convinced that the |
@TSC21 Here you have some links: |
@dirk-thomas I was able to change the Type name of the agent side. Though, weirdly enough, I get the following when trying to echo the topic: $ ros2 topic echo /SensorCombined_PubSubTopic
Cannot echo topic '/SensorCombined_PubSubTopic', as it contains more than one type: [px4_msgs/msg/SensorCombined, px4_msgs/msg/SensorCombined] ?? What's happening here? As far as I can tell the types look exactly the same... |
Did you found any solution of this problem? I found a trick to solve the problem from here. I used px4_ros_com repository. Then, you can echo the topic!
|
@ldg810 thank you! I should have found that source earlier. The solution (even though not ideal) was added in PX4/PX4-Autopilot#13907. Thanks for the support. Closing. |
For everyone still having this issue, I added a fix to the FastRTPSGen tool that through an argument passed to the Hope this helps! |
I am pushing ros2/rmw_fastrtps#251 here, as I am not sure at this point if this is a problem on the ROS 2 node or a problem on the agent code.
System specifications:
micrortps_bridge -t UDP
in a console andsensor_combined_listener
in another.Code snippets:
A WireShark record from
localhost
with both agent and ROS 2 node subscriber running: https://drive.google.com/open?id=1QTx6xjRQZ1Rh1F-sibek40AvvSsF3hw6.@richiware @LuisGP @MiguelCompany any tips will help, as this is currently a development blocker. Thank you!
The text was updated successfully, but these errors were encountered: