Runtime Interface Reflection: rmw_fastrtps #655
Merged
Conversation
This file contains 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
methylDragon
force-pushed
the
runtime_interface_reflection
branch
4 times, most recently
from
403a9f9
to
fd7a44d
Compare
31 tasks
methylDragon
force-pushed
the
runtime_interface_reflection
branch
from
fd7a44d
to
9f0b14a
Compare
methylDragon
force-pushed
the
runtime_interface_reflection
branch
2 times, most recently
from
479ae06
to
f390143
Compare
methylDragon
force-pushed
the
runtime_interface_reflection
branch
4 times, most recently
from
50ad2fb
to
6b7a024
Compare
methylDragon
force-pushed
the
runtime_interface_reflection
branch
3 times, most recently
from
f9d10df
to
3134b7e
Compare
methylDragon
force-pushed
the
runtime_interface_reflection
branch
10 times, most recently
from
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>
methylDragon
force-pushed
the
runtime_interface_reflection
branch
from
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 | ||
| // } | ||
|
|
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
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; | ||
| // } |
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... |
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
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?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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: