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())

@methylDragon methylDragon force-pushed the dynamic_subscription_stubs branch from a2ad9cf to 4de3fd1 Compare April 11, 2023 22:49
@methylDragon methylDragon marked this pull request as draft April 11, 2023 22:52
@methylDragon methylDragon force-pushed the dynamic_subscription_stubs branch from 4de3fd1 to b1b99ea Compare April 11, 2023 23:01
@methylDragon methylDragon marked this pull request as ready for review April 11, 2023 23:01
Signed-off-by: methylDragon <[email protected]>
@methylDragon methylDragon force-pushed the dynamic_subscription_stubs branch from b1b99ea to ee1964f Compare April 11, 2023 23:02
@methylDragon
Copy link
Contributor Author

To see what was actually stubbed out, see ee1964f

Signed-off-by: methylDragon <[email protected]>
@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.

@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 👍

@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 <[email protected]>
@methylDragon methylDragon force-pushed the dynamic_subscription_stubs branch from 331b790 to 1243d80 Compare April 13, 2023 01:05
@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
@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 <[email protected]>
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.

3 participants