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

Runtime Interface Reflection: rclcpp #2077

Closed
wants to merge 27 commits into from

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Jan 3, 2023

This PR is part of the runtime interface reflection subscription feature of REP-2011: ros2/ros2#1374

Description

This PR includes support for runtime type subscriptions, including:

  • Edits to SubscriptionBase and executor code to allow for use of runtime type
    • This includes new pure virtual methods for SubscriptionBase that have also been implemented in all child classes
  • A new RuntimeTypeSubscription subscription class that handles a subscription callback using ser_dynamic_data

@methylDragon methylDragon marked this pull request as draft January 3, 2023 23:52
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 6 times, most recently from 42802ae to af692ec Compare January 25, 2023 08:40
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 3 times, most recently from 41a2df1 to 0ee1cee Compare January 30, 2023 09:24
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch from 0ee1cee to d2b1743 Compare February 28, 2023 01:19
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 2 times, most recently from a298ef5 to dd1ab8c Compare March 16, 2023 18:10
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 9 times, most recently from 86522b4 to 28ce1bf Compare March 24, 2023 09:39
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch from f7f548f to f71bea7 Compare March 28, 2023 19:57
@methylDragon methylDragon marked this pull request as ready for review March 28, 2023 22:36
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch from 8933e83 to 67a958e Compare March 30, 2023 20:35
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 3 times, most recently from 2217287 to b22731b Compare April 4, 2023 00:10
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 4 times, most recently from ac43cb6 to 0a21288 Compare April 5, 2023 23:28
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch from 0a21288 to 29577da Compare April 6, 2023 06:12
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch from b961350 to 32859cd Compare April 8, 2023 01:16
@mergify
Copy link
Contributor

mergify bot commented Apr 9, 2023

⚠️ The sha of the head commit of this PR conflicts with #2160. Mergify cannot evaluate rules on this PR. ⚠️

if (ret != RCL_RET_OK) {
throw std::runtime_error("error initializing rosidl message type support");
}
if (!ts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can all these bad cases be true if ret == RCL_RET_OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 probably not.

I might still leave in the typesupport identifier check though

@mergify
Copy link
Contributor

mergify bot commented Apr 13, 2023

⚠️ The sha of the head commit of this PR conflicts with #2160. Mergify cannot evaluate rules on this PR. ⚠️


RCLCPP_PUBLIC
void
manage_description_(rosidl_runtime_c__type_description__TypeDescription * description);
Copy link
Collaborator

Choose a reason for hiding this comment

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

manage is an overloaded term - this is really a make_shared function to put ownership of the object into a shared pointer, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's true.. I wanted it to be like a: "We are taking ownership/managing the lifetime of the pointer now)

It's a private method though, so I don't have any issues with changing the name

ROS_MESSAGE = 1, // take message as ROS message and handle as ROS message
SERIALIZED_MESSAGE = 2, // take message as serialized and handle as serialized
DYNAMIC_MESSAGE_DIRECT = 3, // take message as DynamicMessage and handle as DynamicMessage
DYNAMIC_MESSAGE_FROM_SERIALIZED = 4 // take message as serialized and handle as DynamicMessage
Copy link
Collaborator

Choose a reason for hiding this comment

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

The distinction here isn't clear to me between dynamic message direct and serialized.

What does this wording pattern "take as X and handle as Y" mean? Does "handle" refer to the argument type for the callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, "handle" is for the callback.

The way it works with ROS is that a message has to be taken, then passed to a handle function that passes it to a callback. So take -> handle.

Copy link
Collaborator

@emersonknapp emersonknapp Apr 13, 2023

Choose a reason for hiding this comment

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

What about a dynamic subscription that passes serialized data to the handler function? GenericDynamicSubscription, perhaps. It's what we would want in rosbag2 recording. Or is that the case for DYNAMIC_MESSAGE_FROM_SERIALIZED, I couldn't quite tell.

Copy link
Contributor Author

@methylDragon methylDragon Apr 13, 2023

Choose a reason for hiding this comment

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

In that case you would want to use a serialized subscription (I.e. one that takes and handles serialized data), then use the DynamicMessage class to deserialize the serialized message into the DynamicMessage inside the callback

You probably would need to subclass that subscription so you can instantiate a DynamicMessage and have it accessible inside the callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Basically you just need a way to implement what the executor is doing here, in the callback)

Copy link
Contributor Author

@methylDragon methylDragon Apr 13, 2023

Choose a reason for hiding this comment

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

    // Take serialized and then convert to dynamic message
    case rclcpp::SubscriptionType::DYNAMIC_MESSAGE_FROM_SERIALIZED:
      {
        std::shared_ptr<SerializedMessage> serialized_msg =
          subscription->create_serialized_message();

        // NOTE(methylDragon): Is this clone necessary? If I'm following the pattern, it seems so.
        DynamicMessage::SharedPtr dynamic_message = subscription->create_dynamic_message();
        take_and_do_error_handling(
          "taking a serialized message from topic",
          subscription->get_topic_name(),
          [&]() {return subscription->take_serialized(*serialized_msg.get(), message_info);},
          [&]()
          {

            // === <<< THIS IS THE MOST IMPORTANT LINE!!! >>> ===
            bool ret = dynamic_message->deserialize(&serialized_msg->get_rcl_serialized_message());
            // ^^^ <<< THIS IS THE MOST IMPORTANT LINE!!! >>> ^^^

            if (!ret) {
              throw_from_rcl_error(ret, "Couldn't convert serialized message to dynamic data!");
            }
            subscription->handle_dynamic_message(dynamic_message, message_info);
          }
        );
        subscription->return_serialized_message(serialized_msg);
        subscription->return_dynamic_message(dynamic_message);
        break;
      }
  }

@methylDragon
Copy link
Contributor Author

Closing this in favor of #2176

@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2023

⚠️ The sha of the head commit of this PR conflicts with #2160. Mergify cannot evaluate rules on this PR. ⚠️

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