-
Notifications
You must be signed in to change notification settings - Fork 116
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: rmw_fastrtps #655
Conversation
403a9f9
to
fd7a44d
Compare
fd7a44d
to
9f0b14a
Compare
479ae06
to
f390143
Compare
50ad2fb
to
6b7a024
Compare
f9d10df
to
3134b7e
Compare
106ac92
to
2e1cd56
Compare
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>
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>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
2e1cd56
to
1097480
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
// void | ||
// on_type_discovery( | ||
// DomainParticipant *, | ||
// const eprosima::fastrtps::rtps::SampleIdentity &, | ||
// const eprosima::fastrtps::string_255 & topic_name, | ||
// const eprosima::fastrtps::types::TypeIdentifier *, | ||
// const eprosima::fastrtps::types::TypeObject *, | ||
// eprosima::fastrtps::types::DynamicType_ptr dyn_type)) final | ||
// { | ||
// NOTE(methylDragon): The dynamic type deferred case is !! NOT SUPPORTED !! | ||
// This is because currently subscriptions are required to have the type at | ||
// construction to create the listener. Deferring it means that the listener | ||
// construction will have to be deferred, and that would require logic changes | ||
// elsewhere (e.g. to check for listener initialization status), which is | ||
// } | ||
|
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.
Should this just be removed then? I think (as I've stated elsewhere) we should just not use NOTE
. I think this should either be a TODO or not left in.
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.
lgtm, I had a few comments, but they're more questions and clean up of comments than anything. I'm less worried about the polish of this pr since it's not part of the user facing API and also its functionality is covered by examples and tests above.
// NOTE(methylDragon): I'm not sure if this is essential or not... | ||
// It doesn't appear in the dynamic type example for FastDDS though | ||
// if (!rmw_fastrtps_shared_cpp::register_type_object(type_support, type_name)) { | ||
// RMW_SET_ERROR_MSG_WITH_FORMAT_STRING( | ||
// "failed to register type object with incompatible type %s", | ||
// type_name.c_str()); | ||
// return nullptr; | ||
// } |
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 think at this point we should remove snippets like this.
// NOTE(methylDragon): The dynamic type deferred case is !! NOT SUPPORTED !! | ||
// This is because it's difficult as-is to create a subscription without | ||
// already having the type. Too much restructuring is needed elsewhere to | ||
// support deferral... |
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.
Should we remove this or is this suppose to be accompanied with a check of some sort?
Signed-off-by: methylDragon <methylDragon@gmail.com>
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 think my comments should still be addressed, but since they're all about code comments, I think we should merge this and fix them up in a follow up. @methylDragon can you make an issue for that if you don't have time to make a pr?
This PR is part of the runtime interface reflection subscription feature of REP-2011: ros2/ros2#1374
Description
This PR includes implementation of the new rmw APIs for supporting runtime interface reflection:
It also includes:
TODO: