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 (Rebased) #2176

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

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Apr 28, 2023

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

It's a rebased (and slightly modified version of #2077)

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 DynamicTypeSubscription subscription class that handles a subscription callback using dynamic_data

@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 28, 2023

  • Linux Build Status Rerun: Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@methylDragon
Copy link
Contributor Author

methylDragon commented May 17, 2023

I spoke to William, and got some preliminary reviews on this PR. These notes do not yet constitute a full review, so there might be even more issues to uncover...
There's a bunch of stuff that need to be changed, and some signatures to smooth out, all of which is going to take some time.

I'll just go about the changes in detail over here, for posterity's sake (no timeline for how/when they'll get done just yet though.)

Non-ABI Breaking

These changes can and should be made in this PR (or a very quick follow-up) since they won't break ABI in Iron

Minor Renames / Comments

Getters

  • There should be getters for the DynamicMessage (and related classes), as well as the description

SharedPtr Getters

  • There are a lot of convenience getters that return shared ptrs. They should either be removed (since move constructors already exist), or if not, should defer to the non-shared versions of those getters, to remove code duplication

Free Functions

  • There should be a free function for creating the DynamicSubscription

ABI Breaking

Class Refactors

  • We should add pimpl members for all C++ classes to the rolling branch for posterity
  • We should store the allocators passed to each object
  • Reintroduce copy constructors, internally calling clone (passing in the stored allocator)

SharedPtr Refactors

  • We're currently storing SharedPtrs to force a lifetime extension of the objects that are stored, we should either store WeakPtrs or references (for DynamicMessage, DynamicMessageType, DynamicMessageTypeBuilder
    • It's more idiomatically C++ to store references and let users control the lifetime, especially for data structures like the classes we're creating here
    • SharedPtrs can cause circular references
    • We maybe might be able to leave the DynamicMessageTypeSupport as a SharedPtr in subscriptions

Self-Describing Messages Description Binding

See next post!!!

Extra Work

@methylDragon
Copy link
Contributor Author

methylDragon commented May 17, 2023

Self-Describing Messages Description Binding

Currently, there is no way to use a DynamicMessage, DynamicMessageType or DynamicMessageTypeBuilder to iterate through the fields of the message. That is, these objects are not self-describing. It's weird because you can access the fields of DynamicMessage if you knew the structure, but there's no way to list that structure otherwise (there's an expectation that you have access to a separate type description alongside the dynamic message.)

In order to achieve self-description and fix this, we can store a reference/pointer to the description.

That is the idea is to:

  • In DynamicMessageTypeBuilder, construct the TypeDescription alongside the DynamicType (as you are adding fields)
  • In the DynamicMessageType (when built from the builder class), copy and store a const version of the TypeDescription
  • In the DynamicMessage, store a reference/pointer to the DynamicMessageType, so you can eventually use the type to access the description

So the descriptions (or at least references to the descriptions) will be baked into the DynamicMessage. So receiving a DynamicMessage will necessarily get you a means to iterate through the fields (via the description)

Additional note:

  • There's two ways to create DynamicMessageType, from a passed in description, or at runtime by calling add_xxx_field() on the DynamicMessageTypeBuilder
    • Because of that we can't always guarantee that a description will be available to be bound to the DynamicMessageType unless we construct it alongside the type in DynamicMessageTypeBuilder (hence the suggestion to up there)

Open question to @wjwwood : Should we associate the type description with the C versions of the structs instead? These C++ classes wrap the C versions.

EDIT:
The following is needed to manipulate TypeDescription messages

Then the intention is to keep the TypeDescription message (or a reference to it) available.

## Helper Classes for Iron
(Cancelling for simplicity)
Okay cool. The problem is all of this ^, while necessary to avoid having to capture the description in a lambda for the callback will make things really nice and smooth, we can't exactly roll the changes out since they'll not be backportable to Iron.

Instead what we can do is (and @wjwwood might have additional notes on this), is to:

  • In DynamicSubscription, only allow constructors that accept descriptions
  • Create SelfDescribingDynamicMessage wrapper class for DynamicMessage that composes the description that the DynamicSubscription can pass the description to, and a new callback signature to AnyDynamicSubscriptionCallback that takes SelfDescribingDynamicMessage.
    • We'll deprecate these classes in rolling

Then in DynamicSubscription's handle_dynamic_message() callback, it can decide when it needs to do this passing.

@wjwwood
Copy link
Member

wjwwood commented May 18, 2023

Specifically for the helper for Iron (to get around ABI issues with the changes we want), I was imagining something like this (pseudocode):

namespace rclcpp::future {
#if IRON
class DynamicSubscription : public rclcpp::DynamicSubscription
{
public:
  using DynamicMessageT = rclcpp::SelfDescribingDynamic;

  DynamicSubscription(
    rclcpp::node_interfaces::NodeBaseInterface * node_base,
    rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::SharedPtr type_support,
    const std::string & topic_name,
    const rclcpp::QoS & qos,
    rclcpp::DynamicSubscription::DynamicCallback callback,
    const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> & options)
  :
    rclcpp::DynamicSubscription(
      node_base,
      type_support,
      topic_name,
      qos,
      std::bind(&DynamicSubscription::wrapper_callback, this, ...),
      options)
  {}

  void
  wrapper_callback(const rclcpp::DynamicMessage& msg)
  {
    if constexpr (std::holds_alternative_v<callback_, SelfDescribingDynamicCallback>()) {
       rclcpp::SelfDescribingDynamic sddm(msg, this->get_description());
       callback_(sddm);
    } else {
      callback_(msg);
    }
  }
};
#else
// deprecate
using DynamicSubscription = rclcpp::DynamicSubscription;
#endif
}
#if IRON
using rclcpp::future::DynamicSubscription;
#else
using rclcpp::DynamicSubscription;
#endif

auto callback = [](const DynamicSubscription::DynamicMessageT & msg) {
  // ... msg is either rclcpp::DynamicMessage or rclcpp::SelfDescribingDynamicMessage, depending on the version
};
auto sub = rclcpp::create_subscription<DynamicSubscription>(node, ..., callback, ...);

Idea being to minimize the user's version of the code between Iron and Rolling.

@methylDragon
Copy link
Contributor Author

I just got some time again, so I'm gonna be spinning this up again and breaking up the PR into smaller PRs to progressively get this in.

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