Skip to content

Have the EventsExecutor use more common code #2570

Merged
mjcarroll merged 11 commits intorollingfrom
asoragna/commonize-executor
Jul 22, 2024
Merged

Have the EventsExecutor use more common code #2570
mjcarroll merged 11 commits intorollingfrom
asoragna/commonize-executor

Conversation

@alsora
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

@alsora alsora Jun 29, 2024

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
Copy Markdown
Collaborator Author

@alsora alsora Jun 29, 2024

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 <alberto.soragna@gmail.com>
@alsora
move mutex lock to retrieve_entity utility
a625d0d 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
use entities_need_rebuild_ atomic bool in events-executors
e4ff7f7 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
remove duplicated set_on_ready_callback for notify_waitable
1cb74be 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
use mutex from base class rather than a new recursive mutex
52a147c 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
use current_collection_ member in events-executor
26b7892 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
delay adding notify waitable to collection
3ff6f20 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
postpone clearing the current collection
f73001f 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
commonize notify waitable and collection
69c1399 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
commonize add/remove node/cbg methods
97c24e8 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@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 <alberto.soragna@gmail.com>
@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
Copy Markdown
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
Copy Markdown
Contributor

ahcorde commented Jul 15, 2024

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

@alsora
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

alsora commented Jul 16, 2024

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

@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Jul 18, 2024

  • Linux Build Status

@mjcarroll
Copy link
Copy Markdown
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
@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

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

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