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

Implement deliver message kind #2168

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Implement deliver message kind #2168

merged 2 commits into from
Apr 14, 2023

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Apr 13, 2023

Follow-up to: #2165

From further discussions, it was found that the SubscriptionType enum to change the behavior of the subscription was better thought of as a way to configure the "category" of message that is being delivered by a subscription (i.e. what is being passed in the subscription's handle function.)

Under that view, it is also possible to collapse the two dynamic message related enum options into one (and have the behavior change in the implementation of the take function.)

Signed-off-by: methylDragon <methylDragon@gmail.com>
Comment on lines -68 to +76
INVALID = 0, // The subscription type is most likely uninitialized
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
INVALID = 0,
ROS_MESSAGE = 1, // The subscription delivers a ROS message to its callback
SERIALIZED_MESSAGE = 2, // The subscription delivers a serialized message to its callback
DYNAMIC_MESSAGE = 3, // The subscription delivers a dynamic message to its callback
Copy link
Member

Choose a reason for hiding this comment

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

I asked @methylDragon to change this, since I believe the difference between a dynamic message delivered from the middleware or by wrapping a serialized message from the middleware should be encapsulated in the subscription, that is to say, neither the executor nor the user creating the subscription should care how the dynamic message is delivered. That also lead to the discussion of the name of this enum and how it was a bit ambiguous, e.g. normal vs generic subscription or lifecycle vs normal, "type" was just overloaded too much.

Also, we discussed a use case where the user wanted the dynamic message to be delivered by wrapping a serialized message always, even if the middleware supported directly returning one, but then we concluded the user could do this themselves by asking for a serialize message in their callback and creating a dynamic message from that themselves. And if it becomes a common pattern we want to put into our API, it should be a separate option form this enum.

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@methylDragon methylDragon merged commit fd7a0dc into rolling Apr 14, 2023
@delete-merged-branch delete-merged-branch bot deleted the delivered_message_kind branch April 14, 2023 03:08
Barry-Xu-2018 pushed a commit to Barry-Xu-2018/rclcpp that referenced this pull request Jan 12, 2024
* Implement deliver message kind

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add examples to docstring

Signed-off-by: methylDragon <methylDragon@gmail.com>

---------

Signed-off-by: methylDragon <methylDragon@gmail.com>
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.

None yet

2 participants