Skip to content

Decouple rosout publisher init from node init.#1065

Merged
fujitatomoya merged 4 commits intorollingfrom
fujitatomoya/bugfix-20230427-rclcpp-issue-2147
Sep 22, 2023
Merged

Decouple rosout publisher init from node init.#1065
fujitatomoya merged 4 commits intorollingfrom
fujitatomoya/bugfix-20230427-rclcpp-issue-2147

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.

3 participants

@fujitatomoya @iuhilnehc-ynos @clalancette