Add test package for single and multi thread#362
Open
Conversation
Signed-off-by: ical-Jeon <junbs95@gmail.com>
clalancette
requested changes
Jul 20, 2023
Contributor
clalancette
left a comment
There was a problem hiding this comment.
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:
- 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).
- 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
- 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?
- Although we aren't totally consistent about this, our style is make class names CamelCase and to make function/method names snake_case.
- Our class member variables are always suffixed with a
_(that's the case in some of this code, but not all of it). - For installation, in general the installation directory should match the name of the package, not a different name.
- A license and a package description need to be filled in in the package.xml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
SingleThreadedExecutorReentrantcallback groupMutuallyExclusivecallback groupIf you have any comments or guidance, please let me know 😊