Runtime Interface Reflection: rmw_fastrtps#655
Merged
Conversation
403a9f9 to
fd7a44d
Compare
31 tasks
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>
wjwwood
reviewed
Apr 7, 2023
Comment on lines
+150
to
+165
| // 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 | ||
| // } | ||
|
|
Member
There was a problem hiding this comment.
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.
wjwwood
approved these changes
Apr 7, 2023
Member
wjwwood
left a comment
There was a problem hiding this comment.
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.
Comment on lines
+305
to
+312
| // 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; | ||
| // } |
Member
There was a problem hiding this comment.
I think at this point we should remove snippets like this.
Comment on lines
+160
to
+163
| // 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... |
Member
There was a problem hiding this comment.
Should we remove this or is this suppose to be accompanied with a check of some sort?
Signed-off-by: methylDragon <methylDragon@gmail.com>
wjwwood
approved these changes
Apr 8, 2023
Member
wjwwood
left a comment
There was a problem hiding this comment.
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: