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 test package for single and multi thread #362

Open
wants to merge 1 commit into
base: foxy
Choose a base branch
from

Conversation

icsl-Jeon
Copy link

@icsl-Jeon icsl-Jeon commented May 20, 2023

Hello. My name is Felipe Jeon, and I am glad to open a PR regarding comparisons on SingleThreadedExecutor and MultiThreadedExecutor (reentrant and mutually exclusive).
I attached detailed analysis on the link inside README.md.

Especially, I covered

  • Round robin of SingleThreadedExecutor
  • Thread scheduling of Reentrant callback group
  • Mutex and MutuallyExclusive callback group

If you have any comments or guidance, please let me know 😊

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.

First of all, thanks for submitting this and sorry that it has taken so long to look into it.

I have a few comments around the structure of this PR:

  1. In the README, please don't point to an external post for documentation about the threading. If there is something we want to explain about how this works, it should be done right here in the README (or possibly on https://docs.ros.org, but I think here in the README makes more sense).
  2. All of the files need license and copyright headings, like in https://github.com/ros2/examples/blob/1d97c4fc7445554f6f85f63305d424fc017212a0/rclcpp/executors/multithreaded_executor/multithreaded_executor.cpp
  3. I'm not totally sure of the structure of this, and how it fits in with our other executor examples. That is, how much does this overlap with https://github.com/ros2/examples/tree/rolling/rclcpp/executors/multithreaded_executor ? Maybe we should think about combining these into the same package there?
  4. Although we aren't totally consistent about this, our style is make class names CamelCase and to make function/method names snake_case.
  5. Our class member variables are always suffixed with a _ (that's the case in some of this code, but not all of it).
  6. For installation, in general the installation directory should match the name of the package, not a different name.
  7. A license and a package description need to be filled in in the package.xml

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.

2 participants