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

export for the event type support function #747

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

iuhilnehc-ynos
Copy link
Contributor

There is no reason to ignore event_message.

Please see

package_name=package_name, message=service.event_message,

@iuhilnehc-ynos
Copy link
Contributor Author

iuhilnehc-ynos commented May 26, 2023

rosidl_runtime_cpp only provides the template function to get the message type support (also including the service and action)

template<typename T>
const rosidl_message_type_support_t * get_message_type_support_handle();

which is different from rosidl_runtime_c using a macro to call the function
#define ROSIDL_GET_MSG_TYPE_SUPPORT(PkgName, MsgSubfolder, MsgName) \
ROSIDL_TYPESUPPORT_INTERFACE__MESSAGE_SYMBOL_NAME( \
rosidl_typesupport_c, PkgName, MsgSubfolder, MsgName)()

, I wonder if it's necessary to add the declaration for each message/service in the header file by rosidl_generator_cpp.

  • e.g., build/example_interfaces/rosidl_generator_cpp/example_interfaces/srv/detail/add_two_ints__type_support.hpp
#include "rosidl_typesupport_cpp/message_type_support.hpp"

#ifdef __cplusplus
extern "C"
{
#endif
// Forward declare the get type support functions for this type.
ROSIDL_GENERATOR_CPP_PUBLIC_example_interfaces
const rosidl_message_type_support_t *
  ROSIDL_TYPESUPPORT_INTERFACE__MESSAGE_SYMBOL_NAME(
  rosidl_typesupport_cpp,
  example_interfaces,
  srv,
  AddTwoInts_Request
)();
#ifdef __cplusplus
}
#endif

// already included above
// #include "rosidl_typesupport_cpp/message_type_support.hpp"

#ifdef __cplusplus
extern "C"
{
#endif
// Forward declare the get type support functions for this type.
ROSIDL_GENERATOR_CPP_PUBLIC_example_interfaces
const rosidl_message_type_support_t *
  ROSIDL_TYPESUPPORT_INTERFACE__MESSAGE_SYMBOL_NAME(
  rosidl_typesupport_cpp,
  example_interfaces,
  srv,
  AddTwoInts_Response
)();
#ifdef __cplusplus
}
#endif

As we know, the definition of messages
https://github.com/ros2/rosidl_typesupport/blob/9062d91c09a79c3b9902a8f06bca4e994121cc52/rosidl_typesupport_cpp/resource/srv__type_support.cpp.em#L3-L24
can be used for loading dynamically without the header file,

https://github.com/ros2/rclcpp/blob/95adde2a19426c23d6ff6e13fab21d62e5e2ae69/rclcpp/src/rclcpp/typesupport_helpers.cpp#L124-L129

    std::string symbol_name = typesupport_identifier + "__get_message_type_support_handle__" +
      package_name + "__" + (middle_module.empty() ? "msg" : middle_module) + "__" + type_name;

    const rosidl_message_type_support_t * (* get_ts)() = nullptr;
    // This will throw runtime_error if the symbol was not found.
    get_ts = reinterpret_cast<decltype(get_ts)>(library.get_symbol(symbol_name));

My question is,
do we need to add the declaration in the header file by rosidl_generator_cpp, or remove the declaration because it seems unnecessary and unuseful?

NOTE: examples of getting message type support

@iuhilnehc-ynos
Copy link
Contributor Author

iuhilnehc-ynos commented May 26, 2023

I think it's not good to add rosidl_typesupport_interface
e.g.,

#define ROSIDL_GET_MSG_TYPE_SUPPORT_CPP(PkgName, MsgSubfolder, MsgName) \
  ROSIDL_TYPESUPPORT_INTERFACE__MESSAGE_SYMBOL_NAME( \
    rosidl_typesupport_cpp, PkgName, MsgSubfolder, MsgName)()

in rosidl_runtime_cpp, but it seems it's the follow-up PR of #703.

Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be added along with response and request message types.

@fujitatomoya
Copy link

can be used for loading dynamically without the header file,
do we need to add the declaration in the header file by rosidl_generator_cpp, or remove #703 because it seems unnecessary and unuseful?

it is doable, but the difference is between compile time and runtime? i might be mistaken.

@iuhilnehc-ynos
Copy link
Contributor Author

it is doable, but the difference is between compile time and runtime?

Yes, but no.

Let me take a generated service typesupport in cpp as an example,

  • build/example_interfaces/rosidl_typesupport_cpp/example_interfaces/srv/add_two_ints__type_support.cpp
namespace rosidl_typesupport_cpp
{

template<>
ROSIDL_TYPESUPPORT_CPP_PUBLIC
const rosidl_service_type_support_t *
get_service_type_support_handle<example_interfaces::srv::AddTwoInts>()
{
  return &::example_interfaces::srv::rosidl_typesupport_cpp::AddTwoInts_service_type_support_handle;
}

}  // namespace rosidl_typesupport_cpp

#ifdef __cplusplus
extern "C"
{
#endif

ROSIDL_TYPESUPPORT_CPP_PUBLIC
const rosidl_service_type_support_t *
ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_cpp, example_interfaces, srv, AddTwoInts)() {
  return ::rosidl_typesupport_cpp::get_service_type_support_handle<example_interfaces::srv::AddTwoInts>();
}

#ifdef __cplusplus
}
#endif

Is there anybody want to use ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_cpp,...) instead of get_service_type_support_handle<>?

Please notice:

  1. C++ template is also built in the compile time
  2. the ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_cpp,...) is calling the template inside.
  3. use in runtime (load dynamically) doesn't need the declaration in the header file

This is why I was confused that adding the declaration ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_cpp,...) in the header file?

@fujitatomoya
Copy link

@iuhilnehc-ynos thanks for the explanation.

Is there anybody want to use ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_cpp,...) instead of get_service_type_support_handle<>?

in short, i think this is unnecessary.

according to your explanation, it seems that we do not need to have these macros in the header file, although i am not familiar with history, there could be intention to add. lets hear opinion from maintainers.

CC: @adityapande-1995 @methylDragon @quarkytale

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

Successfully merging this pull request may close these issues.

2 participants