-
Notifications
You must be signed in to change notification settings - Fork 436
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
Conversation
Signed-off-by: methylDragon <[email protected]>
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 |
There was a problem hiding this comment.
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 <[email protected]>
* Implement deliver message kind Signed-off-by: methylDragon <[email protected]> * Add examples to docstring Signed-off-by: methylDragon <[email protected]> --------- Signed-off-by: methylDragon <[email protected]>
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.)