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

[WIP] Extend DynamicSubscription to support serialized messages #2195

Draft
wants to merge 4 commits into
base: dynamic_subscription
Choose a base branch
from

Conversation

emersonknapp
Copy link
Collaborator

No description provided.

@@ -56,10 +63,7 @@ class DynamicSubscription : public rclcpp::SubscriptionBase
rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::SharedPtr type_support,
const std::string & topic_name,
const rclcpp::QoS & qos,
std::function<void(
rclcpp::dynamic_typesupport::DynamicMessage::SharedPtr,
const rosidl_runtime_c__type_description__TypeDescription &
Copy link
Member

Choose a reason for hiding this comment

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

Will be interesting to see if not having the description in the callback always will be annoying, but I like it not being in the argument list myself. Ideally it would be accessible through the DynamicMessage, but that might not make sense, not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately the descriptions aren't baked into DynamicMessage, since they're technically distinct concepts (and DynamicMessageTypes can be created at runtime without descriptions.)

The only major tradeoff to me of not having the description in the callback is feature discoverability (which was the reason I decided to put it into all callback signatures in the first place.) But if that's a non-issue I am very much for including getters in the DynamicSubscription class.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, well then maybe you should be able to get the DynamicMessageType from the DynamicMessage. The description from the DynamicMessageType might be an optional thing. The question is "do users often need the description in the callback"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking, if you need the description in the callback, you can provide it yourself via closure, or member access in the case of class methods:

auto dynamic_typesupport = DynamicMessageTypeSupport::make_shared(type_description);
auto sub = create_dynamic_subscription(
  node, 
  dynamic_typesupport, 
  topic_name, 
  qos, 
  [type_description](shared_ptr<SerializedMessage> message) { 
    // has access to type_description 
  });

Providing the description to the callback as a parameter does allow for reuse of the same callback for several dynamic subscriptions to different types, I guess - but I don't see the real life use case for that.

Copy link
Collaborator Author

@emersonknapp emersonknapp May 17, 2023

Choose a reason for hiding this comment

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

This seems the same to me as e.g. topic_name and topic_type_name - which you also don't have access to in subscription callbacks, right?

Copy link
Member

Choose a reason for hiding this comment

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

Right, it's the same, but only if the DynamicMessage can be useful without the description. Currently you cannot list the fields with just it, so you need the description to reasonably use it. Whereas with a message and topic name, you can most often make use of the message without the topic name, so it's fine to ask the user to figure that out with captures or something.

Anyway @methylDragon has a bunch of notes on this.

@methylDragon
Copy link
Contributor

methylDragon commented May 17, 2023

Thanks Emerson for helping with this! 🙇

I think other than the points William listed above, we just need to add getters to the DynamicSubscription to get the DynamicMessageType (probably as const or copy) and the type description.

Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp emersonknapp force-pushed the emersonknapp/dynamic-serialized-sub branch from b9d7913 to 9da20f8 Compare May 17, 2023 21:08
Signed-off-by: Emerson Knapp <[email protected]>
@methylDragon
Copy link
Contributor

There are some further discussions about the C++ classes on the parent PR:

@methylDragon methylDragon force-pushed the dynamic_subscription branch 2 times, most recently from be61087 to dd5c608 Compare December 31, 2024 04:25
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.

3 participants