Skip to content

Decouple rosout publisher init from node init. #1065

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
Merged
fujitatomoya merged 4 commits into from
Sep 22, 2023
Merged

Decouple rosout publisher init from node init. #1065

fujitatomoya merged 4 commits into from
Sep 22, 2023

Conversation

@fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Apr 27, 2023 •
edited
Loading

fujitatomoya
fujitatomoya commented Apr 27, 2023
View reviewed changes
This was referenced Apr 27, 2023
Merged
Merged
Merged
Merged
Closed
@clalancette clalancette self-assigned this May 4, 2023
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 15, 2023

@iuhilnehc-ynos can you review those PRs?

@iuhilnehc-ynos
Copy link
Collaborator

iuhilnehc-ynos commented Jun 16, 2023

LGTM.

There is a tiny issue I can think of,
How can we notify users, whose applications only depend on rcl rather than rclc/rclcpp/rclpy, to add the rosout publisher manually? I don't know if we should consider it or not.

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 16, 2023 •
edited
Loading

How can we notify users, whose applications only depend on rcl rather than rclc/rclcpp/rclpy, to add the rosout publisher manually? I don't know if we should consider it or not.

good question, let me check if there is at least doc mentions that with this change.

edit: i think that is clear that user can call this function after rcl_node_init which creates rcl_node_t.

rcl/rcl/include/rcl/logging_rosout.h

Lines 98 to 99 in 230ae2f

* Calling this for an rcl_node_t will create a new publisher on that node that will be
* used by the logging system to publish all log messages from that Node's logger.

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 16, 2023

CI:

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

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 17, 2023

CI (retry):

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

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 18, 2023 •
edited
Loading

the other failures are unrelated, ModuleNotFoundError: No module named 'rclpy.type_hash'.

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 18, 2023

CI:

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

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/bugfix-20230427-rclcpp-issue-2147 branch from 91f7580 to 3f706e6 Compare June 19, 2023 18:22
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 19, 2023

CI(rebase)

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

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 20, 2023

  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 20, 2023

windows has been meeting some CI instability, i am not sure what is wrong...

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 20, 2023 •
edited
Loading

CI(windows only w/o rclc):

  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 21, 2023

https://ci.ros2.org/job/ci_windows/19758/testReport/ is unrelated.

clalancette
clalancette requested changes Jun 21, 2023
View reviewed changes
@fujitatomoya fujitatomoya mentioned this pull request Sep 11, 2023
Open
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/bugfix-20230427-rclcpp-issue-2147 branch from 3f706e6 to 1113c68 Compare September 11, 2023 19:22
fujitatomoya
fujitatomoya commented Sep 11, 2023
View reviewed changes
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Sep 18, 2023

either @iuhilnehc-ynos or @Barry-Xu-2018 could you do review on the related PRs?

iuhilnehc-ynos
iuhilnehc-ynos reviewed Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

Two minor comments.

iuhilnehc-ynos
iuhilnehc-ynos reviewed Sep 20, 2023
View reviewed changes
@fujitatomoya
Decouple rosout publisher init from node init.
cef9193 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Decouple rosout publisher fini from node fini.
ce0471e 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
address review comment.
5abc429 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
add node_option check.
cefce8a 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/bugfix-20230427-rclcpp-issue-2147 branch from d29753f to cefce8a Compare September 20, 2023 18:22
fujitatomoya
fujitatomoya commented Sep 20, 2023
View reviewed changes
Copy link
Collaborator Author

@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.

CC: @iuhilnehc-ynos

because of 7b9c1ec, i needed to rebase but it actually makes it simplified.

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Sep 20, 2023

CI:

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

@iuhilnehc-ynos

This comment was marked as off-topic.

Sign in to view
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Sep 21, 2023 •
edited
Loading

my bad 😓 sorry about that.

CI:

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

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Sep 21, 2023

https://ci.ros2.org/job/ci_windows/20261/ fails with unrelated things in rclc, so for windows i will restart the CI w/o rclc.

CI(windows, w/o rclc):

  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Sep 22, 2023

@clalancette i will go ahead to merge this with @iuhilnehc-ynos 's approval.

@fujitatomoya fujitatomoya merged commit 1260197 into rolling Sep 22, 2023
@delete-merged-branch delete-merged-branch bot deleted the fujitatomoya/bugfix-20230427-rclcpp-issue-2147 branch September 22, 2023 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clalancette clalancette clalancette requested changes

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

@audrow audrow Awaiting requested review from audrow

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

@clalancette clalancette

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fujitatomoya @iuhilnehc-ynos @clalancette