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

Add EventsExecutor API for dynamic_fastrtps #600

Merged
merged 2 commits into from
May 4, 2022

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented May 3, 2022

Fixes #599

Two commits:

  • Copy the implementation done in Add EventsExecutor #468 for dynamic version
  • Add some check to parameters passed to fastrtps_cpp to keep both implementations in sync

Testing:

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

I think that cpplint is not related to this PR. I can open a new PR to fix it.

@j-rivero j-rivero requested a review from clalancette May 3, 2022 18:46
@clalancette
Copy link
Contributor

I think that cpplint is not related to this PR. I can open a new PR to fix it.

Yeah, agreed. I'd say let's get that fix in (I'm happy to review it), then rebase this one and get green CI. The Rpr job is going to be broken for right now until Jacob finishes doing releases for Rolling.

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 reasonable to me, but let's rebase it onto the latest and run a full CI on it to make sure nothing breaks.

Mostly a copy of the implementation done for rmw_fastrtps_cpp.
Fixes #599.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
To keep the both rmw_fastrtps_ implementations in sync with respect to
argument checking.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero j-rivero force-pushed the jrivero/executor_for_dynamic branch from f2cfc82 to ef9f429 Compare May 4, 2022 15:23
@j-rivero
Copy link
Contributor Author

j-rivero commented May 4, 2022

CI:

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

@j-rivero
Copy link
Contributor Author

j-rivero commented May 4, 2022

Seems fine, merging.

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.

rmw_fastrtps_dynamic_cpp is currently incomplete
2 participants