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 parameter to configure number of thread #1708

Merged
merged 7 commits into from
Dec 13, 2021
Merged

Add parameter to configure number of thread #1708

merged 7 commits into from
Dec 13, 2021

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented Jul 6, 2021

No description provided.

@wep21
Copy link
Contributor Author

wep21 commented Jul 6, 2021 •
edited
Loading

I added a parameter to configure number of thread instead of an argument.
@ivanpauno @wjwwood @fujitatomoya Could you review this?
#1693

fujitatomoya
fujitatomoya reviewed Jul 7, 2021
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

so is this expected to use with --ros-args -p thread_num:=2? if i am not mistaken.

i was thinking that we would want to consider to enable and provide parameter service to set/get the number of threads on rclcpp_components::ComponentManager.

@ivanpauno ivanpauno mentioned this pull request Jul 8, 2021
Closed
ivanpauno
ivanpauno reviewed Jul 9, 2021
View reviewed changes
@fujitatomoya fujitatomoya mentioned this pull request Sep 17, 2021
Closed
@ivanpauno
Copy link
Member

@wep21 frindly ping, there are still some unresolved comments here

@wep21 wep21 force-pushed the feature/add-param-for-thread-num branch from 36ee1ec to fd648c2 Compare November 7, 2021 17:55
@wep21 wep21 requested a review from ivanpauno December 7, 2021 04:46
@wep21 wep21 force-pushed the feature/add-param-for-thread-num branch from afb94bc to 7611d4d Compare December 7, 2021 05:56
@ivanpauno
Copy link
Member

CI:

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

@fujitatomoya
Copy link
Collaborator

@wep21 it seems that we need to rebase, could you do that? (see https://ci.ros2.org/job/ci_linux/15733/console)

fujitatomoya
fujitatomoya approved these changes Dec 7, 2021
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@wep21 wep21 force-pushed the feature/add-param-for-thread-num branch from 7611d4d to 4ef00f8 Compare December 7, 2021 16:25
@wep21
Copy link
Contributor Author

wep21 commented Dec 7, 2021

@fujitatomoya I rebased from the latest maser.
@ivanpauno Could you run ci again?

@fujitatomoya
Copy link
Collaborator

CI(retry):

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

@ivanpauno
Copy link
Member

Sorry, there are conflicts again after #1781 was merged.
@wep21 could you rebase again to resolve conflicts? Thanks!

wep21 added 7 commits December 8, 2021 05:09
Add parameter to configure number of thread
a00be46 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Address review: Declare parameter in component manager
a1e0cf0 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Add set executor
2a6ec10 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Use int64_t
d644762 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Use default node options
ca2ab04 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Address review: Fix parameter descriptor
2f4ced5 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Address review: Add default argument
c653972 
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 force-pushed the feature/add-param-for-thread-num branch from 4ef00f8 to c653972 Compare December 7, 2021 20:15
@wep21
Copy link
Contributor Author

wep21 commented Dec 7, 2021

@ivanpauno I have finished resolving conflicts and rebase.

@wep21
Copy link
Contributor Author

wep21 commented Dec 13, 2021

@ivanpauno friendly ping

@fujitatomoya
Copy link
Collaborator

let me run CI once just in case.

CI:

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

@fujitatomoya
Copy link
Collaborator

all green, i will go ahead to merge this into master.

@fujitatomoya fujitatomoya merged commit ee20dd3 into ros2:master Dec 13, 2021
@fujitatomoya
Copy link
Collaborator

@wep21 thanks for the contribution.

@wep21 wep21 deleted the feature/add-param-for-thread-num branch December 13, 2021 18:33
Bi0T1N pushed a commit to Bi0T1N/rclcpp that referenced this pull request Dec 15, 2021
@wep21
Add parameter to configure number of thread (ros2#1708)
7dadb2d 
* Add parameter to configure number of thread

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aprotyas aprotyas aprotyas left review comments

@ivanpauno ivanpauno ivanpauno approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@ivanpauno ivanpauno

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wep21 @ivanpauno @fujitatomoya @aprotyas