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.

Sign up for GitHub

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

Jump to bottom

Dynamic Subscription (BONUS: Allocators): rclcpp #2160

Closed
wants to merge 31 commits into from
Closed

Dynamic Subscription (BONUS: Allocators): rclcpp #2160

wants to merge 31 commits into from

Conversation

methylDragon
Copy link
Contributor

@methylDragon
Implement dynamic typesupport wrappers
b8fd304 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Implement dynamic message type support wrapper
a3e5d0a 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Refine wrappers
554ad02 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Delete redundant wrapper class definitions
833efe0 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Take ownership of description, add print, and update ser support API
a2484be 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Implement first cut
0ce81c4 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Migrate to rosidl_dynamic_typesupport and update field IDs
5af57f8 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Refactor dynamic typesupport to couple serialization support to objects
927b9de 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Use dynamic message type support wrappers
bfcc29e 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Implement direct taking of dynamic messages
9ff2843 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Only use general type construction and implement default values
78a568d 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Use new deletes for memory management
9555859 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Alias dynamic_data with dynamic_message, pass desc, and build ts top-…
363861f 
…down

Signed-off-by: methylDragon <[email protected]>
@methylDragon
Add fixed string support
57900a9 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Implement SubscriptionType enum flag
ca1fe2b 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Unalias dynamic typesupport types
24e533e 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Remove dynamic data/type aliases
aac28e7 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Migrate methods to use return types
113dc01 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Lint
7e0d063 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Support type hashes
8571b28 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Add initializers for description and source getters
e7e9a29 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Support type hashes
ca17caf 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Use dynamic typesupport identifier getter
000f788 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Move dynamic type support struct to rosidl and fix mem issues
29577da 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Use create instead of init
d871af0 
Signed-off-by: methylDragon <[email protected]>
@methylDragon methylDragon force-pushed the runtime_interface_reflection_allocators branch from 1e2eb10 to 165fbd3 Compare April 7, 2023 10:18
@methylDragon methylDragon mentioned this pull request Apr 7, 2023
Open
28 tasks
@methylDragon
Remove print methods
30210a2 
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Remove RMW interfaces
32859cd 
Signed-off-by: methylDragon <[email protected]>
@methylDragon methylDragon force-pushed the runtime_interface_reflection_allocators branch from 165fbd3 to 32859cd Compare April 9, 2023 06:48
@mergify mergify bot mentioned this pull request Apr 9, 2023
Closed
@methylDragon methylDragon mentioned this pull request Apr 9, 2023
Merged
@methylDragon methylDragon force-pushed the runtime_interface_reflection_allocators branch 3 times, most recently from 0fd00b4 to d3077b2 Compare April 10, 2023 04:20
@methylDragon methylDragon force-pushed the runtime_interface_reflection_allocators branch from d3077b2 to 8447d36 Compare April 10, 2023 05:33
@methylDragon methylDragon force-pushed the runtime_interface_reflection_allocators branch from c52f250 to c8734b3 Compare April 10, 2023 16:19
@wjwwood
Copy link
Member

wjwwood commented Apr 10, 2023

I think we could just roll this one into #2077, since it hasn't been merged yet and needs review still.

@methylDragon
Copy link
Contributor Author

I agree :>, but I'll leave that one open for now until the refactor PRs are in (since this depends on those)

clalancette
clalancette requested changes Apr 10, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I took a brief look here, and pointed out a couple of potential problems.

rclcpp/include/rclcpp/subscription_base.hpp
Comment on lines -235 to +247
bool
is_serialized() const;
SubscriptionType
get_subscription_type() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as an FYI, we are going to have to tick-tock this public API.

rclcpp/src/rclcpp/executor.cpp
rclcpp::MessageInfo message_info;
message_info.get_rmw_message_info().from_intra_process = false;

if (subscription->is_serialized()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to coordinate with @mjcarroll and @alsora here, as this could run into conflict with what they are doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll wait for their responses!

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that anything here conflicts with the other executor changes. The execute_* family of functions deals with how to actually dispatch the work, while what I have been working on has been how that work is queued up.

@alsora what do you think?

@methylDragon methylDragon mentioned this pull request Apr 11, 2023
Merged
@methylDragon
Copy link
Contributor Author

methylDragon commented May 3, 2023 •
edited
Loading

Closing in favor of:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll left review comments

@clalancette clalancette clalancette requested changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@methylDragon @wjwwood @clalancette @mjcarroll