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

Have the EventsExecutor use more common code #2570

Merged
merged 11 commits into from
Jul 22, 2024
Merged

Have the EventsExecutor use more common code #2570

merged 11 commits into from
Jul 22, 2024

Conversation

alsora
Copy link
Collaborator

@alsora alsora commented Jun 29, 2024

The EventsExecutor was creating its own entities collector, collection ,etc.
Now that these are part of the executor base class, the events executor should just use them.

This PR should not do any functional change, but only reorganizing the code and reducing duplications.

alsora
alsora commented Jun 29, 2024
View reviewed changes
rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp
this->add_notify_waitable_to_collection(current_entities_collection_->waitables);

notify_waitable_->add_guard_condition(interrupt_guard_condition_);
notify_waitable_->add_guard_condition(shutdown_guard_condition_);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these 3 lines above are done in the base class

alsora
alsora commented Jun 29, 2024
View reviewed changes
rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp
notify_waitable_->add_guard_condition(shutdown_guard_condition_);

notify_waitable_->set_on_ready_callback(
this->create_waitable_callback(notify_waitable_.get()));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was likely the result of a bad merge, as the same thing is then done again (and correctly) in the line below

alsora added 10 commits June 29, 2024 16:28
@alsora
move notify waitable setup to its own function
55d4e77 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
move mutex lock to retrieve_entity utility
a625d0d 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
use entities_need_rebuild_ atomic bool in events-executors
e4ff7f7 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
remove duplicated set_on_ready_callback for notify_waitable
1cb74be 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
use mutex from base class rather than a new recursive mutex
52a147c 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
use current_collection_ member in events-executor
26b7892 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
delay adding notify waitable to collection
3ff6f20 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
postpone clearing the current collection
f73001f 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
commonize notify waitable and collection
69c1399 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora
commonize add/remove node/cbg methods
97c24e8 
Signed-off-by: Alberto Soragna <[email protected]>
@alsora alsora force-pushed the asoragna/commonize-executor branch from 423f712 to 97c24e8 Compare June 29, 2024 23:29
@alsora
fix linter errors
23f305e 
Signed-off-by: Alberto Soragna <[email protected]>
@mjcarroll mjcarroll self-assigned this Jul 12, 2024
@mjcarroll mjcarroll self-requested a review July 12, 2024 17:58
mjcarroll
mjcarroll approved these changes Jul 12, 2024
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Always good to see code disappear!

@ahcorde
Copy link
Contributor

ahcorde commented Jul 15, 2024

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

@alsora
Copy link
Collaborator Author

alsora commented Jul 15, 2024

Mmm I see this in the logs

08:35:04 [ssh-agent] Looking for ssh-agent implementation...
08:35:04 [ssh-agent]   Exec ssh-agent (binary ssh-agent on a remote machine)
08:35:04 $ ssh-agent
08:35:04 [ssh-agent]   Java/JNR ssh-agent
08:35:05 ERROR: [ssh-agent] Could not register BouncyCastle on the remote agent.

trying again:

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

@alsora
Copy link
Collaborator Author

alsora commented Jul 16, 2024

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

@alsora
Copy link
Collaborator Author

alsora commented Jul 18, 2024

  • Linux Build Status

@mjcarroll
Copy link
Member

mjcarroll commented Jul 22, 2024 •
edited
Loading

CI looks like there were some warnings in message filters, unrelated to the changes here.

Edit: they were addressed by ros-perception/point_cloud_transport#94

@mjcarroll mjcarroll merged commit 54b8f9c into rolling Jul 22, 2024
3 checks passed
@mjcarroll mjcarroll deleted the asoragna/commonize-executor branch July 22, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees

@mjcarroll mjcarroll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alsora @ahcorde @mjcarroll