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

Decouple rosout publisher init from node init. #1065

Merged
merged 4 commits into from Sep 22, 2023
Merged

Decouple rosout publisher init from node init. #1065

merged 4 commits into from Sep 22, 2023

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Apr 27, 2023 •
edited

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

@iuhilnehc-ynos can you review those PRs?

@iuhilnehc-ynos
Copy link
Collaborator

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

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

CI:

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

@fujitatomoya
Copy link
Collaborator Author

CI (retry):

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

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jun 18, 2023 •
edited

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

@fujitatomoya
Copy link
Collaborator Author

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

CI(rebase)

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

@fujitatomoya
Copy link
Collaborator Author

  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

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

CI(windows only w/o rclc):

  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

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

clalancette
clalancette requested changes Jun 21, 2023
View reviewed changes
rcl/src/rcl/node.c Show resolved Hide resolved
@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
Copy link
Collaborator Author

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.

rcl/test/rcl/test_graph.cpp Show resolved Hide resolved
rcl/test/rcl/test_logging_rosout.cpp Outdated Show resolved Hide resolved
@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

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

my bad 😓 sorry about that.

CI:

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

@fujitatomoya
Copy link
Collaborator Author

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

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

@fujitatomoya fujitatomoya merged commit 1260197 into rolling Sep 22, 2023
3 checks passed
@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 audrow is a code owner

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees

@clalancette clalancette

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fujitatomoya @iuhilnehc-ynos @clalancette