Skip to content

Add parameter to configure number of thread#1708

Merged
fujitatomoya merged 7 commits intoros2:masterfrom
wep21:feature/add-param-for-thread-num
Dec 13, 2021
Merged

Add parameter to configure number of thread#1708
fujitatomoya merged 7 commits intoros2:masterfrom
wep21:feature/add-param-for-thread-num

Conversation

@wep21
Copy link
Copy Markdown
Contributor

@wep21 wep21 commented Jul 6, 2021

No description provided.

@wep21
Copy link
Copy Markdown
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
Copy Markdown
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 requested changes Jul 8, 2021
View reviewed changes
ivanpauno
ivanpauno reviewed Jul 9, 2021
View reviewed changes
@fujitatomoya fujitatomoya mentioned this pull request Sep 17, 2021
Closed
@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Nov 3, 2021

@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
ivanpauno
ivanpauno reviewed Nov 16, 2021
View reviewed changes
@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
Copy Markdown
Member

ivanpauno commented Dec 7, 2021

CI:

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Dec 7, 2021

@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
Copy Markdown
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
Copy Markdown
Contributor Author

wep21 commented Dec 7, 2021

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Dec 7, 2021

CI(retry):

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

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Dec 7, 2021

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

wep21 commented Dec 7, 2021

@ivanpauno I have finished resolving conflicts and rebase.

@wep21
Copy link
Copy Markdown
Contributor Author

wep21 commented Dec 13, 2021

@ivanpauno friendly ping

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Dec 13, 2021

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

fujitatomoya commented Dec 13, 2021

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

fujitatomoya commented Dec 13, 2021

@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