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

remodel publication manager #749

Merged
merged 2 commits into from
Apr 22, 2021
Merged

remodel publication manager #749

merged 2 commits into from
Apr 22, 2021

Conversation

Karsten1987
Copy link
Collaborator

A few things here:

  • I renamed the publisher_manager to publication_manager.
  • The publication manager sets up a node in its constructor
    • It can't thus be default initialized as a class member var as rclcpp::init wasn't triggered just yet.
    • A separate call to setup publishers before running it, eases up discovery of the node and the publishers before actually calling publish
    • We only have one node for all publishers within the test. Before that, each publisher had its own node, which certainly didn't help discovery.
  • General cleanup of the record_end_to_end_tests

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987 Karsten1987 requested a review from a team as a code owner April 22, 2021 16:07
@Karsten1987 Karsten1987 requested review from emersonknapp and zmichaels11 and removed request for a team April 22, 2021 16:07
@Karsten1987
Copy link
Collaborator Author

CI without xfail:

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

I don't necessarily expect a clean CI here, but the record end to end tests should pass.

@Karsten1987
Copy link
Collaborator Author

master is currently broken due to ros2/rmw_cyclonedds#306

@Karsten1987
Copy link
Collaborator Author

building without CycloneDDS:

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

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM - i like that the code got smaller

rclcpp::get_logger("publication_manager"),
"publish on topic %s", publisher->get_topic_name());
}
std::this_thread::sleep_for(interval);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would there be a slightly simpler way to build this with timers instead? Not sure, just thinking out loud

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am actually in favor of this solution. Timers involve node spinning and executors. Here I can simply call publish and sleep.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Collaborator Author

with cyclone

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

@Karsten1987 Karsten1987 merged commit 5e83ce3 into master Apr 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the kk/cleanup_tests branch April 22, 2021 20:22
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

2 participants