Fixed missing C interfaces to obtain service and action type support. #114

StefanFabian · 2021-07-26T08:49:50Z

Currently, only message type support contains a C interface to dynamically load the type support from a library.
This PR adds a C interface for services and actions.

Signed-off-by: Stefan Fabian <[email protected]>

StefanFabian · 2021-08-23T09:00:03Z

@sloretz Any idea when you could take a look at it?

This is a small change but essential for two large ROS2 libraries that I would like to release in the next week(s).

rosidl_typesupport_cpp/resource/action__type_support.cpp.em

rosidl_typesupport_cpp/resource/srv__type_support.cpp.em

ros-discourse · 2022-04-21T11:24:23Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros2-babel-fish-released/25201/1

StefanFabian · 2022-05-19T08:33:07Z

bump

sloretz

I think I understand the context, and I like the direction this is heading. @StefanFabian would you be willing to do the following? @clalancette thoughts?

Target this PR at the master branch. We'll backport it using mergify, and I'm sure these additions can be backported.
Open a PR to rosidl_generator_cpp to declare these functions for actions, services, and messages. None of the existing headers fit so I'd recommend putting them into new action|msg|srv__type_support.hpp headers to be included here

If I understand correctly, rosidl_typesupport_interface defines macros to be used to get handles to typesupports. These macros are defining a C API. If you know the name of the message then you can get the typesupport handle through it. The trouble is it's not consistently implemented.

C typesupport uses rosidl_typesupport_interface consistently - probably because it has no alternative

rosidl_generator_c
rosidl_typesupport_c
rosidl_typesupport_introspection_c
- Doesn't appear to support actions? Oops!
- Declares the symbol for services here
- Declares the symbol for messages here
- Defines the symbol for services here
- Defines tye symbol for messages here

C++ typesupport doesn't consistently implement rosidl_typesupport_interface

rosidl_generator_cpp
- Does not declare any functions to implement rosidl_typesupport_interface for actions, services, or messages
rosidl_typesupport_cpp
- Does not define functions to implement rosidl_typesupport_interface for actions and services
- Defines the function for messages here
rosidl_typesupport_introspection_cpp
- Doesn't appear to support actions? Oops again!
- Declares the function for services here
- Declares the function for messages here
- Defines the function for services here
- Defines the function for messages here

Instead what's happening in C++ is it's relying on a different interface that gets the typesupport from the C++ type.

I think it's fine for the C++ to have it's own interface, but I think it should implement the rosidl_typesupport_interface API too. That's what this PR does 🎉

Not to be done here (maybe in the future, I'll open a ticket), but now that I think I understand it I would implement C++'s templated methods differently. I think it'd be more consistent and simpler to have rosidl_generator_cpp generate get_*_type_support_handle<T>() in a header file that returned the result of calling the C API defined by rosidl_typesupport_interface.

StefanFabian · 2022-08-15T12:33:28Z

Thanks @sloretz for taking a look at this. Sorry for the late reply.
I will look into how it works and make a PR as you've suggested.

StefanFabian · 2022-08-15T14:16:52Z

Okay, not as simple as I've thought.
In rosidl_generator_c, a package-specific visibility control is used.
Here in this PR, I have used ROSIDL_TYPESUPPORT_CPP_PUBLIC. Do you want me to keep it that way or introduce a package-specific visibility control?
Currently, there are no visibility declarations in the cpp resources.

StefanFabian · 2022-09-16T16:01:08Z

@sloretz I've targeted the master here and created a PR for the declaration of the getter methods.
If you need anything else from me, just say the word 😉

jhurliman · 2023-01-26T21:07:08Z

Is any additional work needed on this PR to get it merge-ready?

**Public-Facing Changes** - Add ROS2 support for calling server-advertised services **Description** Based on #136 - Adds experimental ROS2 support for advertising/unadvertising services and allowing clients to call them - Implements the services spec that was added in foxglove/ws-protocol#344 Implementation details: The main problem is, that symbols from getting the service type support are missing from the `rosidl_typesupport_cpp` libraries that are generated for each msg/srv package (see ros2/rosidl_typesupport#122). There is an open pull request (ros2/rosidl_typesupport#114) to fix that, but so far it hasn't been merged. I found a working, yet little bit hacky, workaround for this problem which is described more in detail here: https://github.com/foxglove/ros-foxglove-bridge/blob/f978c182185e5deda93a6518132b6d3c339dcaeb/ros2_foxglove_bridge/src/generic_client.cpp#L59-L89 All in all, the implementation is working, but I would consider it as experimental for now. I am not sure if this approach would work on windows / mac

ros-discourse · 2023-02-17T15:21:37Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/humble-support-for-ros2-babel-fish-and-qml-ros2-plugin-for-great-looking-intuitive-robot-uis/29852/1

sloretz · 2023-03-18T00:14:33Z

rosidl_typesupport_cpp/resource/action__type_support.cpp.em

+{
+#endif
+
+ROSIDL_TYPESUPPORT_CPP_PUBLIC


ROSIDL_GENERATOR_CPP_PUBLIC_@(package_name) to match ros2/rosidl#703. Mind fixing the one in msg__type_support.cpp.em too?

The problem is that rosidl_typesupport_cpp does not depend on rosidl_generator so I would have to create a visibility header for rosidl_typesupport_cpp as well.
I can do that but I'm not really sure what for since these libraries are built for dynamic loading anyway and I don't see why anyone should want this method to be anything other than always visible.
However, I'm not 100% sure I understand all use cases of visibility control so if you think that makes sense I can add it.

In general, the PUBLIC macros are mostly there for Windows compatibility. That is, Windows requires that when compiling a library, these macros are set to dllexport, and that when using the library, these macros are set to dllimport.

In this particular case, I think I agree with @StefanFabian in that all of the other functions in this package use ROSIDL_TYPESUPPORT_CPP_PUBLIC, so we can use that here as well. But maybe @sloretz has something else in mind.

Ah, it looks like the generated library is using the visibility symbol ROSIDL_TYPESUPPORT_CPP_BUILDING_DLL, so ROSIDL_TYPESUPPORT_CPP_PUBLIC is the one to use.

This looks a little suspicious to me - as the generated library is package specific. It's also used for rosidl_typesupport_cpp's library. If the generated library includes a header from another generated library, or from the rosidl_typesupport_cpp main library, then I think dllimport/dllexport would be incorrect for symbols from the included header. I opened an issue since it's an existing problem #142

sloretz · 2023-03-18T00:17:59Z

Ah, looks like the galactic branch is deleted on [email protected]:StefanFabian/rosidl_typesupport.git - @StefanFabian mind resurrecting the galactic branch on your fork, if you still have it? Otherwise I can open a new PR with your changes.

StefanFabian · 2023-03-20T16:51:39Z

I have pushed my local copy of the branch back to my fork, does that work?

clalancette · 2023-03-20T16:56:08Z

I have pushed my local copy of the branch back to my fork, does that work?

It doesn't look like it, no. GitHub UI still says "This repository has been deleted".

StefanFabian · 2023-03-20T17:17:54Z

Ah yeah, I accidentally deleted the entire repository. Can't restore that anymore. I could only make a new PR from my "new" fork.

StefanFabian · 2023-04-03T08:35:52Z

So, how do we proceed? @sloretz
Is it okay to stick with ROSIDL_TYPESUPPORT_CPP_PUBLIC to keep it consistent with the other functions in this package?
Should I make a new PR from my new fork where we can change that or do you want to create one with the changes?

StefanFabian · 2023-05-04T13:41:38Z

Friendly reminder @sloretz @clalancette

StefanFabian · 2023-05-11T09:03:48Z

In the hopes that this will help judge the importance of accepting this.
Without this change, it's not possible to build the QML UI displayed below in ROS2.
(Well showing robot status is possible but the buttons triggering robot actions/behaviors would be non-functional)

Just this little change would allow extremely easily and quickly creating hardware-accelerated user interfaces (for operation or quick visual debugging) such as the one pictured with this already released currently only partially functional package qml_ros2_plugin.
@sloretz @clalancette

gavanderhoorn · 2023-05-11T09:23:19Z

I just wanted to add my 👍 to @StefanFabian's comment, but more than just a reaction emoji.

I have StefanFabian/qml_ros2_plugin#3 open, but without support for services and actions, utility of qml_ros2_plugin is significantly reduced.

It would be great if we could get the change proposed here merged.

sloretz · 2023-05-11T18:34:22Z

rosidl_typesupport_cpp/resource/action__type_support.cpp.em

+{
+#endif
+
+ROSIDL_TYPESUPPORT_CPP_PUBLIC


Ah, it looks like the generated library is using the visibility symbol ROSIDL_TYPESUPPORT_CPP_BUILDING_DLL, so ROSIDL_TYPESUPPORT_CPP_PUBLIC is the one to use.

This looks a little suspicious to me - as the generated library is package specific. It's also used for rosidl_typesupport_cpp's library. If the generated library includes a header from another generated library, or from the rosidl_typesupport_cpp main library, then I think dllimport/dllexport would be incorrect for symbols from the included header. I opened an issue since it's an existing problem #142

clalancette · 2023-05-11T20:23:54Z

@StefanFabian This needs one more rebase to catch up with all of the latest changes (I've tested it locally, and it is a trivial rebase). Once you've done that and pushed this, I'll go ahead and run CI on this and ros2/rosidl#703 together.

StefanFabian · 2023-05-12T08:25:39Z

I have rebased in #143 and am closing this in favor of this new PR that can be modified.

StefanFabian · 2023-05-16T10:38:30Z

Are backports to foxy and humble planned?
I'm not sure how the process for that works.

clalancette · 2023-05-16T12:03:17Z

Are backports to foxy and humble planned?

For foxy, definitely not. It is EOL in a week.

For humble, I guess we could consider it, but we'll have to make sure there are no ABI changes from merging these PRs. If you'd like to go ahead and verify that, we can then open the backport PRs.

StefanFabian · 2023-05-25T07:31:26Z

@clalancette PR ros2/rosidl#703 only declares new free functions (non-member functions) and this PR implements them.

According to GCC section Allowed Changes, adding an exported function is an ABI-compatible change.

StefanFabian · 2023-06-12T21:00:43Z

Should I create a humble backport PR?

achim-k · 2023-11-20T17:05:16Z

Should I create a humble backport PR?

@StefanFabian that would be highly appreciated. I think I could do it as well if you don't have time for it.

StefanFabian · 2023-11-20T23:28:40Z

I'm on vacation until next week Tuesday, so I can't do it before some time end of next week.
If you have the time that would be great 👍

aaron-jca · 2023-11-23T18:04:49Z

I'm on vacation until next week Tuesday, so I can't do it before some time end of next week. If you have the time that would be great 👍

Would also greatly appreciate this. We were just looking at how to handle this with our team and it would save us a lot of headache.

NicolasRouquette · 2023-11-28T22:05:51Z

For humble developers, we managed to work around this problem, which manifests as link errors like this for some [library], [package], [service]:

/usr/bin/ld: [library].so: undefined reference to `rosidl_service_type_support_t const* rosidl_typesupport_cpp::get_service_type_support_handle<[package]::srv::[service]>()'

There are similar errors for messages too.

The workaround involves a CMakeLists.txt like this for some [project], [dependency], [library]:

cmake_minimum_required(VERSION 3.5)
project([project])

find_package(ament_cmake REQUIRED)
find_package([dependency] REQUIRED)
find_package(rclcpp REQUIRED)
find_package(rclcpp_lifecycle REQUIRED)
find_package(rclcpp_components REQUIRED)
find_package(rclpy REQUIRED)
find_package(rosidl_default_generators REQUIRED)
find_package(rosgraph_msgs REQUIRED)
find_package(std_msgs REQUIRED)
find_package(std_srvs REQUIRED)

set(CMAKE_CXX_STANDARD 17)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
  #add_compile_options(-Wall -Wextra -Wpedantic)
endif()

rosidl_generate_interfaces(
    ${PROJECT_NAME}
        # Messages
        msg/...

        # Services
        srv/...
        
    DEPENDENCIES
        builtin_interfaces
        [dependency]
)

include_directories(include)

set(dependencies
  builtin_interfaces
  [dependency]
  rclcpp
  rclcpp_lifecycle
  rclcpp_components
  rosgraph_msgs
  std_msgs
  std_srvs
)

# Export the compile commands.
set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)

add_library([library]
  SHARED
  src/...)

ament_target_dependencies([library]
  ${dependencies})

rosidl_get_typesupport_target(cpp_typesupport_target "${PROJECT_NAME}" "rosidl_typesupport_cpp")
target_link_libraries([library] "${cpp_typesupport_target}")

ament_package()

Can someone explain how the above will change once the fix is back-ported to humble?

aaron-jca · 2023-12-04T18:53:45Z

For humble developers, we managed to work around this problem, which manifests as link errors like this for some [library], [package], [service]:

/usr/bin/ld: [library].so: undefined reference to `rosidl_service_type_support_t const* rosidl_typesupport_cpp::get_service_type_support_handle<[package]::srv::[service]>()'

There are similar errors for messages too.

The workaround involves a CMakeLists.txt like this for some [project], [dependency], [library]:

cmake_minimum_required(VERSION 3.5)
project([project])

find_package(ament_cmake REQUIRED)
find_package([dependency] REQUIRED)
find_package(rclcpp REQUIRED)
find_package(rclcpp_lifecycle REQUIRED)
find_package(rclcpp_components REQUIRED)
find_package(rclpy REQUIRED)
find_package(rosidl_default_generators REQUIRED)
find_package(rosgraph_msgs REQUIRED)
find_package(std_msgs REQUIRED)
find_package(std_srvs REQUIRED)

set(CMAKE_CXX_STANDARD 17)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
  #add_compile_options(-Wall -Wextra -Wpedantic)
endif()

rosidl_generate_interfaces(
    ${PROJECT_NAME}
        # Messages
        msg/...

        # Services
        srv/...
        
    DEPENDENCIES
        builtin_interfaces
        [dependency]
)

include_directories(include)

set(dependencies
  builtin_interfaces
  [dependency]
  rclcpp
  rclcpp_lifecycle
  rclcpp_components
  rosgraph_msgs
  std_msgs
  std_srvs
)

# Export the compile commands.
set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)

add_library([library]
  SHARED
  src/...)

ament_target_dependencies([library]
  ${dependencies})

rosidl_get_typesupport_target(cpp_typesupport_target "${PROJECT_NAME}" "rosidl_typesupport_cpp")
target_link_libraries([library] "${cpp_typesupport_target}")

ament_package()

Can someone explain how the above will change once the fix is back-ported to humble?

I have limited experience in this area, so correct me if I'm wrong, but I would imagine this workaround wouldn't be needed any longer. Instead the interfaces would just need to be rebuilt with the back-ported fix.

aaron-jca · 2023-12-05T18:30:51Z

Tried the backport myself, but I'm unable to make it work.
Getting errors of the form

CMake Error at /Workspace/install/rosidl_generator_cpp/share/rosidl_generator_cpp/cmake/rosidl_generator_cpp_generate_interfaces.cmake:118 (add_dependencies):
  The dependency target
  "/Workspace/build/example_interfaces/rosidl_generator_cpp/example_interfaces/msg/rosidl_generator_cpp__visibility_control.hpp"
  of target "example_interfaces" does not exist.
Call Stack (most recent call first):
  /opt/ros/humble/share/ament_cmake_core/cmake/core/ament_execute_extensions.cmake:48 (include)
  /Workspace/install/rosidl_cmake/share/rosidl_cmake/cmake/rosidl_generate_interfaces.cmake:286 (ament_execute_extensions)
  CMakeLists.txt:16 (rosidl_generate_interfaces)

All failing on rosidl_generator_cpp_generate_interfaces.cmake:118 (add_dependencies): but with different files not existing, even though the files are being generated and are at the path specified.

@StefanFabian your help would be much appreciated. I'm willing to help test and verify that this works, but I don't know how to proceed.

Added C interfaces to obtain service and action type support.

63ade0d

Signed-off-by: Stefan Fabian <[email protected]>

StefanFabian mentioned this pull request Jul 26, 2021

[foxy backport] Fixed missing C interfaces to obtain service and action type support. #115

Closed

StefanFabian changed the title ~~Added C interfaces to obtain service and action type support.~~ [galactic] Added C interfaces to obtain service and action type support. Jul 26, 2021

clalancette assigned sloretz Aug 12, 2021

clalancette requested changes Oct 13, 2021

View reviewed changes

rosidl_typesupport_cpp/resource/action__type_support.cpp.em Show resolved Hide resolved

rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Show resolved Hide resolved

StefanFabian requested a review from clalancette October 25, 2021 08:38

StefanFabian mentioned this pull request Feb 8, 2022

Service generator does not create symbol name for rosidl_typesupport_cpp #122

Open

StefanFabian mentioned this pull request Feb 18, 2022

ROS 2 Support StefanFabian/qml_ros_plugin#3

Closed

StefanFabian mentioned this pull request Apr 20, 2022

Something went wrong loading the typesupport library for [action/service] type... LOEWE-emergenCITY/ros_babel_fish#1

Open

StefanFabian changed the title ~~[galactic] Added C interfaces to obtain service and action type support.~~ [galactic] Fixed missing C interfaces to obtain service and action type support. May 19, 2022

sloretz requested changes Jun 11, 2022

View reviewed changes

sloretz mentioned this pull request Jun 11, 2022

Make C++ typesupport use rosidl_typesupport_interface ros2/rosidl#684

Open

clalancette assigned jacobperron Jun 30, 2022

StefanFabian changed the base branch from galactic to master August 15, 2022 12:33

StefanFabian closed this by deleting the head repository Sep 14, 2022

StefanFabian reopened this Sep 16, 2022

StefanFabian mentioned this pull request Sep 16, 2022

Generate typesupport declarations for actions, messages and services ros2/rosidl#703

Merged

StefanFabian requested review from sloretz and removed request for clalancette September 20, 2022 07:48

StefanFabian changed the title ~~[galactic] Fixed missing C interfaces to obtain service and action type support.~~ Fixed missing C interfaces to obtain service and action type support. Sep 20, 2022

achim-k mentioned this pull request Jan 26, 2023

Add ROS2 support for calling server-advertised services foxglove/ros-foxglove-bridge#142

Merged

sloretz reviewed Mar 18, 2023

View reviewed changes

jacobperron removed their assignment May 9, 2023

sloretz approved these changes May 11, 2023

View reviewed changes

jhurliman approved these changes May 11, 2023

View reviewed changes

clalancette approved these changes May 11, 2023

View reviewed changes

StefanFabian mentioned this pull request May 12, 2023

[RePR from #114] Fixed missing C interfaces to obtain service and action type support. #143

Merged

StefanFabian closed this May 12, 2023

StefanFabian mentioned this pull request May 24, 2023

[foxy backport] Added C interfaces to obtain service and action type support. #144

Merged

StefanFabian mentioned this pull request Dec 27, 2023

[humble backport] Added C interfaces to obtain service and action type support. #149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed missing C interfaces to obtain service and action type support. #114

Fixed missing C interfaces to obtain service and action type support. #114

StefanFabian commented Jul 26, 2021

StefanFabian commented Aug 23, 2021

ros-discourse commented Apr 21, 2022

StefanFabian commented May 19, 2022

sloretz left a comment

StefanFabian commented Aug 15, 2022

StefanFabian commented Aug 15, 2022

StefanFabian commented Sep 16, 2022 •

edited

Loading

jhurliman commented Jan 26, 2023

ros-discourse commented Feb 17, 2023

sloretz Mar 18, 2023

StefanFabian Mar 20, 2023

clalancette Mar 20, 2023

sloretz May 11, 2023

sloretz commented Mar 18, 2023

StefanFabian commented Mar 20, 2023

clalancette commented Mar 20, 2023

StefanFabian commented Mar 20, 2023

StefanFabian commented Apr 3, 2023

StefanFabian commented May 4, 2023

StefanFabian commented May 11, 2023

gavanderhoorn commented May 11, 2023

sloretz May 11, 2023

clalancette commented May 11, 2023

StefanFabian commented May 12, 2023

StefanFabian commented May 16, 2023 •

edited

Loading

clalancette commented May 16, 2023

StefanFabian commented May 25, 2023

StefanFabian commented Jun 12, 2023

achim-k commented Nov 20, 2023

StefanFabian commented Nov 20, 2023

aaron-jca commented Nov 23, 2023

NicolasRouquette commented Nov 28, 2023

aaron-jca commented Dec 4, 2023

aaron-jca commented Dec 5, 2023 •

edited

Loading

Fixed missing C interfaces to obtain service and action type support. #114

Fixed missing C interfaces to obtain service and action type support. #114

Conversation

StefanFabian commented Jul 26, 2021

StefanFabian commented Aug 23, 2021

ros-discourse commented Apr 21, 2022

StefanFabian commented May 19, 2022

sloretz left a comment

Choose a reason for hiding this comment

StefanFabian commented Aug 15, 2022

StefanFabian commented Aug 15, 2022

StefanFabian commented Sep 16, 2022 • edited Loading

jhurliman commented Jan 26, 2023

ros-discourse commented Feb 17, 2023

sloretz Mar 18, 2023

Choose a reason for hiding this comment

StefanFabian Mar 20, 2023

Choose a reason for hiding this comment

clalancette Mar 20, 2023

Choose a reason for hiding this comment

sloretz May 11, 2023

Choose a reason for hiding this comment

sloretz commented Mar 18, 2023

StefanFabian commented Mar 20, 2023

clalancette commented Mar 20, 2023

StefanFabian commented Mar 20, 2023

StefanFabian commented Apr 3, 2023

StefanFabian commented May 4, 2023

StefanFabian commented May 11, 2023

gavanderhoorn commented May 11, 2023

sloretz May 11, 2023

Choose a reason for hiding this comment

clalancette commented May 11, 2023

StefanFabian commented May 12, 2023

StefanFabian commented May 16, 2023 • edited Loading

clalancette commented May 16, 2023

StefanFabian commented May 25, 2023

StefanFabian commented Jun 12, 2023

achim-k commented Nov 20, 2023

StefanFabian commented Nov 20, 2023

aaron-jca commented Nov 23, 2023

NicolasRouquette commented Nov 28, 2023

aaron-jca commented Dec 4, 2023

aaron-jca commented Dec 5, 2023 • edited Loading

StefanFabian commented Sep 16, 2022 •

edited

Loading

StefanFabian commented May 16, 2023 •

edited

Loading

aaron-jca commented Dec 5, 2023 •

edited

Loading