search

ros2 / rosidl_typesupport

Packages which provide the typesupport for ROS messages and services
Apache License 2.0
13 stars 34 forks source link

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

Closed StefanFabian closed 1 year ago

StefanFabian commented 2 years ago

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.

StefanFabian commented 2 years ago

@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).

ros-discourse commented 2 years ago

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 commented 2 years ago

bump

StefanFabian commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 :wink:

jhurliman commented 1 year ago

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

ros-discourse commented 1 year ago

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

StefanFabian commented 1 year ago

Push @sloretz @clalancette @jacobperron Sorry for being annoying and I understand you guys have a lot on your plate but I kinda have to. This bug is significantly reducing the usefulness of my ROS2 libraries and they were a lot of work compared to this small fix.

clalancette commented 1 year ago

Sorry for being annoying and I understand you guys have a lot on your plate but I kinda have to. This bug is significantly reducing the usefulness of my ROS2 libraries and they were a lot of work compared to this small fix.

In short; this generally looks good to me, though it needs a rebase onto the latest rolling branch so that we can run CI on it. Once that has been done, I'll run CI.

sloretz commented 1 year ago

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?

This is a good catch. Sorry for not responding earlier. I think we should introduce a package-specific visibility file. I see you're already doing that in ros2/rosidl#703 :tada:

I think the package-specific visibility files is needed because, we need the *_PUBLIC variable to be one thing when a library is being built, and another thing when the same library is being used on Windows. Using ROSIDL_TYPESUPPORT_CPP_PUBLIC here would be a problem because the package specific library we generate depends on the the main rosidl_typesupport_cpp library. That means when we build the package specific typesupport library, the visibility declarations on functions functions defined in the main rosidl_typesupport_cpp library are incorrect.

sloretz commented 1 year ago

Ah, looks like the galactic branch is deleted on git@github.com: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 commented 1 year ago

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

clalancette commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Friendly reminder @sloretz @clalancette

StefanFabian commented 1 year ago

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) image

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 commented 1 year ago

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

I have https://github.com/StefanFabian/qml_ros2_plugin/pull/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.

clalancette commented 1 year ago

@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 https://github.com/ros2/rosidl/pull/703 together.

StefanFabian commented 1 year ago

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

StefanFabian commented 1 year ago

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

clalancette commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Should I create a humble backport PR?

achim-k commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.