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.

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

Dynamic Subscription (REP-2011 Subset): Stubs for rclcpp #2165

Merged
merged 6 commits into from Apr 13, 2023

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Apr 11, 2023

This is a stubbed out version of: #2160

  • New classes are defined (stubbed out)
  • New virtual methods are set for SubscriptionBase
  • SubscriptionBase is using an enum to govern its behavior and execute_subscription behavior now uses that enum

Note: I had to introduce (and stub out) the new dynamic typesupport classes in rclcpp since the virtual methods on SubscriptionBase use them

I added the classes, then stubbed them out in a separate commit so it's clear what's being removed.

Stuff removed are:

  • Non-virtual constructors
  • Templated class methods
  • Normal class methods
  • The function implementations
  • Executor behavior surrounding dynamic subscriptions (inside a code branch in execute_subscription())

Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon marked this pull request as draft April 11, 2023 22:52
@methylDragon methylDragon marked this pull request as ready for review April 11, 2023 23:01
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

To see what was actually stubbed out, see ee1964f

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Debug Build Status
  • RHEL Build Status

@methylDragon
Copy link
Contributor Author

CI is unstable because of unused parameters (and another unrelated issue on rqt_reconfigure that this PR does not affect), I'll issue a fix soon

Copy link
Contributor

@allenh1 allenh1 left a comment

Choose a reason for hiding this comment

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

@methylDragon just some minor comments / questions. Looks good overall.

rclcpp/src/rclcpp/generic_subscription.cpp Show resolved Hide resolved
rclcpp/include/rclcpp/subscription_base.hpp Show resolved Hide resolved
rclcpp/include/rclcpp/subscription.hpp Show resolved Hide resolved
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 12, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Build Status
  • RHEL Build Status

Copy link
Contributor

@allenh1 allenh1 left a comment

Choose a reason for hiding this comment

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

@methylDragon LGTM 👍

rclcpp/include/rclcpp/generic_subscription.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/subscription_base.hpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/subscription_base.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/subscription_base.cpp Outdated Show resolved Hide resolved
@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 12, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Build Status
  • RHEL Build Status

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.

This looks generally OK to me, assuming CI comes back clean for a run on all packages.

@methylDragon
Copy link
Contributor Author

This looks generally OK to me, assuming CI comes back clean for a run on all packages.

Thanks for the review! 🙇

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 13, 2023

🤦 lint errors

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Debug Build Status
  • RHEL Build Status

@clalancette
Copy link
Contributor

@ros-pull-request-builder retest this please

@clalancette clalancette merged commit b8173e2 into rolling Apr 13, 2023
2 of 3 checks passed
@clalancette clalancette deleted the dynamic_subscription_stubs branch April 13, 2023 14:32
Barry-Xu-2018 pushed a commit to Barry-Xu-2018/rclcpp that referenced this pull request Jan 12, 2024
* Implement subscription base changes

* Add stubbed out classes

Signed-off-by: methylDragon <methylDragon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants