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

add events-executor and timers-manager in rclcpp #2140

Closed
wants to merge 7 commits into from
Closed

add events-executor and timers-manager in rclcpp #2140

wants to merge 7 commits into from

Conversation

alsora
Copy link
Collaborator

@alsora alsora commented Mar 24, 2023

This PR introduces the events-executor based on the design here ros2/design#305 and the already-existing open-source implementation in https://github.com/irobot-ros/events-executor.

FYI @clalancette @wjwwood @mjcarroll @mauropasse @bpwilcox

@alsora
add events-executor and timers-manager in rclcpp
c7121d1 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@mjcarroll mjcarroll self-requested a review March 24, 2023 18:27
alsora
alsora commented Mar 24, 2023
View reviewed changes
rclcpp/test/rclcpp/executors/test_events_executor.cpp
EXPECT_TRUE(std::chrono::steady_clock::now() - start < 200ms);
}

// FIX THIS TEST! The entities collector is being called too many times!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: fix this test

@mjcarroll
Copy link
Member

@alsora I have some other work on-going right now as well, how you you feel about reviving this PR as well in conjunction with adding the events executor and my improvements? #1891

@alsora
Copy link
Collaborator Author

alsora commented Mar 24, 2023

Actually, given that the static executor is going to be deprecated, I was about to close #1891 and here I created a completely separate entities-collector.

However, I'm open to different directions.

@mjcarroll
Copy link
Member

here I created a completely separate entities-collector.

Let me take a look at what you have here. I refactored the entity collection features out of the base Executor class because I saw it was being reproduced already similar to what you did in #1891, but with support for the rclcpp::WaitSet.

@alsora
Copy link
Collaborator Author

alsora commented Mar 24, 2023 •
edited
Loading

Oh ok.
Anyhow it's very easy to go back to the "split implementation".
You can find it in the irobot-events-executor repo: base entities-collector class and events-executor entities-collector class

the code is a direct follow up of the #1891 PR (the base class should be exactly the same)

@mjcarroll
Copy link
Member

Doing one turn of CI for a baseline:

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

@alsora
add RCLCPP_PUBLIC to timers_manager public methods
9b0752f 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
prevent events-executor tests from running with connext
dabe7ad 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Collaborator Author

alsora commented Mar 27, 2023 •
edited
Loading

Disabled events-executor tests when using connext dds (events-executor support is WIP there).
Fixed some windows errors.

New CI (rerun after 3799e31)

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

@alsora
fix copy-paste errors disabling tests for connext
4f33b20 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
add RCLCPP_PUBLIC to events-executor method
3799e31 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
do not run subscriber_triggered_to_receive_message test with static e…
5e1fc89 
…xecutor

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
make timeouts and sleeps more robust
c6d873d 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
alsora
alsora commented Mar 28, 2023
View reviewed changes
rclcpp/test/rclcpp/executors/test_events_executor.cpp
EXPECT_TRUE(std::chrono::steady_clock::now() - start < 1s);
}

TEST_F(TestEventsExecutor, destroy_entities)
Copy link
Collaborator Author

@alsora alsora Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test is flaky.
It fails approximately 1/5 due to a crash, we need to investigate.

@alsora
Copy link
Collaborator Author

alsora commented Mar 28, 2023

New CI

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

@mjcarroll mjcarroll mentioned this pull request Mar 28, 2023
Merged
3 tasks
@mjcarroll
Copy link
Member

@alsora I have opened #2142, where I have used the EntityCollector and NotifyWaitable, can we refactor between these two implementations for common functionality?

@alsora
Copy link
Collaborator Author

alsora commented Mar 28, 2023

I have opened #2142, where I have used the EntityCollector and NotifyWaitable, can we refactor between these two implementations for common functionality?

Ok, I can review that PR and then take care of it; is it your plan to first merge that one and then we can proceed with this one?

@mjcarroll
Copy link
Member

is it your plan to first merge that one and then we can proceed with this one?

Maybe we can meet in the middle on the common types in another PR and then the events executor and waitset can be independent.

@mjcarroll mjcarroll mentioned this pull request Mar 29, 2023
Merged
@alsora alsora mentioned this pull request Mar 30, 2023
Merged
@alsora
Copy link
Collaborator Author

alsora commented Apr 4, 2023

Closing this PR in favor of the new one #2155

@alsora alsora closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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

@mjcarroll mjcarroll Awaiting requested review from mjcarroll

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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