Add support for logging service. #2122

llapx · 2023-03-08T01:43:39Z

No description provided.

llapx · 2023-03-08T01:51:34Z

#ros2/rcl_interfaces#154

llapx · 2023-03-08T07:42:38Z

Rebased to the latest rolling.

Barry-Xu-2018 · 2023-03-21T12:26:16Z

This PR depend on #2122

Barry-Xu-2018 · 2023-03-21T12:28:53Z

There is another way to avoid changing the existing interface of NodeBase.
Create a new class NodeBuiltinExecutor. It is initialized at Node construct. start_parameter_services, start_parameter_event_publisher, use_clock_thread and enable_log_service in node options are handled in this class. That is, all builitin pubisher, service, etc are located in class NodeBuiltinExecutor. Node option control if it is created.
Of course, this leads to big change. So maybe this design is better to be implemented in refactor action instead of implementing this PR.

fujitatomoya

@llapx thanks for working on this, 1st review completed.

rclcpp/include/rclcpp/node.hpp

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

rclcpp/src/rclcpp/node.cpp

rclcpp/include/rclcpp/node_options.hpp

rclcpp/src/rclcpp/time_source.cpp

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

rclcpp/src/rclcpp/node_interfaces/node_logging.cpp

rclcpp/test/rclcpp/test_logging_service.cpp

fujitatomoya · 2023-03-22T17:12:10Z

@clalancette @mjcarroll @wjwwood @alsora @gbiggs @iuhilnehc-ynos

I would like to have opinion and comments on the followings(only applies to rclcpp).

currently time source (/clock subscription) has SingleThreadedExecutor if ROS time is enabled. and that executor is dedicated to time source. But this PR tries to share the executor between time source and logging level service this PR adds. i think that keep the dedicated executor for time source would be necessary for user experience, and probably add one more SingleThreadedExecutor for logger level service? Or user can configure if they want to have dedicated executor for time source? what do you think?
via logger level service, user might want to know the effective logger level as well? (Or, either rcutils_logging_get_logger_effective_level or rcutils_logging_get_logger_level?) i am inclined to take rcutils_logging_get_logger_level, since this is actual level of the logger in the hash map, so that user can figure out what level should be applied for based on their requirement.

thanks,

fujitatomoya · 2023-04-03T21:15:28Z

@Barry-Xu-2018 since we have not received the response to #2122 (comment), probably we can keep the current implementation with this PR, which is sharing single threaded executor within time source and logging level service if enabled. what do you think?

@iuhilnehc-ynos please chime in if you have comments on this.

thanks,

Barry-Xu-2018 · 2023-04-04T01:18:09Z

@fujitatomoya

since we have not received the response to #2122 (comment), probably we can keep the current implementation with this PR, which is sharing single threaded executor within time source and logging level service if enabled. what do you think?

Yes. But I refactor code to avoid many changes on existing interface.
Barry-Xu-2018#3
For the first version (f4199fe6613734f4b64934ac654e6176a1c75ee8), I used NodeBuiltinExecutor to launch logger service and clock thread. But find time_source has problem for some case.
After discussing with @iuhilnehc-ynos, we think we should use NodeBuiltinExecutor only for logger service (Otherwise, there are many changes for time_source).
We can create new PR to refactor time_source to use NodeBuiltinExecutor.
What do you think ?

About the second point, we incline to use rcutils_logging_get_logger_level.

Barry-Xu-2018 · 2023-04-04T06:14:54Z

0074d6d introduce a new class NodeBuiltinExecutor.
NodeBuiltinExecutor take charge of running all internal(built-In) services, subscription, etc.
Now only logger service is launched.
I don't want to have too many changes in this PR which isn't related to logger service.
Next, clock thread and related work in time source will be moved to this class.

fujitatomoya · 2023-04-04T16:59:47Z

@Barry-Xu-2018 @iuhilnehc-ynos

We had a quick discussion on this today.

in addition to #2122 (comment), we can just rely on user thread to spin just like parameter services ParameterService do. This can be good enough to handle the logging services in the node. I did not think we had discuss on this before, but sounds good enough to me. what do you think?

in case of handling these services with internal thread just like time source, SingleThreadedExecutor can be shared with time source, so that we can avoid the situation to extra thread in the node.

CC: @wjwwood @alsora

fujitatomoya · 2023-04-04T17:00:48Z

in addition to #2122 (comment), we can just rely on user thread to spin just like parameter services ParameterService do. This can be good enough to handle the logging services in the node. I did not think we had discuss on this before, but sounds good enough to me. what do you think?

besides, this is the current implementation of ros2/rclpy#1102

Barry-Xu-2018 · 2023-04-05T13:21:22Z

in addition to #2122 (comment), we can just rely on user thread to spin just like parameter services ParameterService do. This can be good enough to handle the logging services in the node. I did not think we had discuss on this before, but sounds good enough to me. what do you think?

Yes. We can put logger service in user thread like parameter services.
From our previous consideration, we want to move all internal services (such as parameter service, clock service, log service, etc) from user thread to internal thread. So we created NodeBuiltinExecutor.
Of course, it's not appropriate to include such changes for this purpose in this PR.

fujitatomoya · 2023-04-05T19:08:10Z

Of course, it's not appropriate to include such changes for this purpose in this PR.

I think this can be handles as different issue and PRs, but currently i see that user thread could be most appropriate.

Barry-Xu-2018 · 2023-04-06T03:55:20Z

I updated codes.
@fujitatomoya @iuhilnehc-ynos please review again.

fujitatomoya

lgtm

fujitatomoya · 2023-04-06T16:33:13Z

CI:

Linux
Linux-aarch64
Windows

Barry-Xu-2018 · 2023-04-07T03:27:03Z

Rebase was done.

wjwwood

Lgtm, I was iffy about the service interface being passed separately from the constructor but on reflection I think that's actually for the best since the feature is optional.

fujitatomoya · 2023-04-17T19:05:48Z

CI:

Linux
Linux-aarch64
Windows

Signed-off-by: Lei Liu <Lei.Liu.AP@sony.com>

Signed-off-by: Barry Xu <barry.xu@sony.com>

clalancette · 2023-04-17T20:32:02Z

This needed a rebase, which I've done now. I think it also needs an updated .repos file, which I've also made. Here's another CI:

Linux
Linux-aarch64
Windows

clalancette · 2023-04-17T20:35:19Z

(also, Rpr is going to fail since we haven't released rcl_interfaces yet; we'll do that tomorrow)

clalancette · 2023-04-18T02:02:20Z

@fujitatomoya @llapx @Barry-Xu-2018 So unfortunately it looks like something is going on with Windows here. In particular, some of the tests are crashing that didn't crash in the nightlies, and didn't seem to crash with the other code that went in today. I honestly don't know what is going on here, but I don't think we should put this in like it is. Can you please take a look and try to see what is happening?

Barry-Xu-2018 · 2023-04-18T03:25:55Z

@clalancette Okay. I will check failed test cases.

fujitatomoya · 2023-04-18T04:58:22Z

actually there are 5 test failure, all of them looks not related to our PRs, but hard to guarantee that at this very last moment.
@Barry-Xu-2018 any thoughts?

starting windows CI again:

Windows

Barry-Xu-2018 · 2023-04-18T05:05:59Z

@fujitatomoya
Yeah. These failed tests are all the program died or did not respond.
The logger service isn't enabled, change of this PR should not affect test result.
So I suspect it's an issue with the CI system. We have to run it again.

fujitatomoya · 2023-04-18T05:39:24Z

@clalancette @Yadunund

in our opinion, that is so unlikely those are related to our PR, since our code is not even enabled. (builtin logging level service is opt-in.)
so I would say that we can merge this, and we will be there if anything happens to our code as we do.

on the other hand, we understand that you do not take this at very last moment unless CI is all green. (CI windows has been restarted, but will not be completed in time.)

* Add support for logging service. * Update to not modify interfaces and not change time_source * Use unique_ptr for NodeBuiltinExecutorImpl * Use user thread to run logger service * Update code for lifecycle_node Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Lei Liu <Lei.Liu.AP@sony.com>

llapx requested review from ivanpauno, hidmic and wjwwood as code owners March 8, 2023 01:43

This was referenced Mar 8, 2023

Add interfaces for logging service. ros2/rcl_interfaces#154

Merged

External logging level configuration ros2/ros2#1355

Closed

llapx force-pushed the topic-logging_service branch 2 times, most recently from 907f1f2 to 0f0c3c4 Compare March 8, 2023 07:42

fujitatomoya self-requested a review March 14, 2023 05:02

fujitatomoya self-assigned this Mar 14, 2023

fujitatomoya added the enhancement New feature or request label Mar 14, 2023

fujitatomoya reviewed Mar 22, 2023

View reviewed changes

Barry-Xu-2018 mentioned this pull request Apr 2, 2023

Implement logger service Barry-Xu-2018/rclcpp#3

Merged

Barry-Xu-2018 force-pushed the topic-logging_service branch from 7d4bccc to 0074d6d Compare April 4, 2023 05:01

fujitatomoya approved these changes Apr 6, 2023

View reviewed changes

iuhilnehc-ynos approved these changes Apr 7, 2023

View reviewed changes

Barry-Xu-2018 force-pushed the topic-logging_service branch from 22f0d1f to 0ca98ec Compare April 7, 2023 03:26

wjwwood approved these changes Apr 17, 2023

View reviewed changes

llapx and others added 6 commits April 17, 2023 20:29

Add support for logging service.

4525d92

Signed-off-by: Lei Liu <Lei.Liu.AP@sony.com>

Update to not modify interfaces and not change time_source

e6019bb

Signed-off-by: Barry Xu <barry.xu@sony.com>

Use unique_ptr for NodeBuiltinExecutorImpl

a48e1da

Signed-off-by: Barry Xu <barry.xu@sony.com>

Remove unused header file and use correct return value declarations

bc2c980

Signed-off-by: Barry Xu <barry.xu@sony.com>

Use user thread to run logger service

0171bdf

Signed-off-by: Barry Xu <barry.xu@sony.com>

Update code for lifecycle_node

c6fb623

Signed-off-by: Barry Xu <barry.xu@sony.com>

clalancette force-pushed the topic-logging_service branch from 0ca98ec to c6fb623 Compare April 17, 2023 20:29

Barry-Xu-2018 mentioned this pull request Apr 18, 2023

Add demos for using logger service ros2/demos#611

Merged

clalancette merged commit 3610b68 into ros2:rolling Apr 18, 2023

christophebedard mentioned this pull request Oct 28, 2024

Fix NodeOptions assignment operator #2656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for logging service. #2122

Add support for logging service. #2122

llapx commented Mar 8, 2023

llapx commented Mar 8, 2023

llapx commented Mar 8, 2023

Barry-Xu-2018 commented Mar 21, 2023

Barry-Xu-2018 commented Mar 21, 2023 •

edited

Loading

fujitatomoya left a comment

fujitatomoya commented Mar 22, 2023

fujitatomoya commented Apr 3, 2023

Barry-Xu-2018 commented Apr 4, 2023 •

edited

Loading

Barry-Xu-2018 commented Apr 4, 2023

fujitatomoya commented Apr 4, 2023

fujitatomoya commented Apr 4, 2023

Barry-Xu-2018 commented Apr 5, 2023

fujitatomoya commented Apr 5, 2023

Barry-Xu-2018 commented Apr 6, 2023

fujitatomoya left a comment

fujitatomoya commented Apr 6, 2023

Barry-Xu-2018 commented Apr 7, 2023

wjwwood left a comment

fujitatomoya commented Apr 17, 2023

clalancette commented Apr 17, 2023

clalancette commented Apr 17, 2023

clalancette commented Apr 18, 2023

Barry-Xu-2018 commented Apr 18, 2023

fujitatomoya commented Apr 18, 2023

Barry-Xu-2018 commented Apr 18, 2023

fujitatomoya commented Apr 18, 2023

Add support for logging service. #2122

Add support for logging service. #2122

Conversation

llapx commented Mar 8, 2023

llapx commented Mar 8, 2023

llapx commented Mar 8, 2023

Barry-Xu-2018 commented Mar 21, 2023

Barry-Xu-2018 commented Mar 21, 2023 • edited Loading

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Mar 22, 2023

fujitatomoya commented Apr 3, 2023

Barry-Xu-2018 commented Apr 4, 2023 • edited Loading

Barry-Xu-2018 commented Apr 4, 2023

fujitatomoya commented Apr 4, 2023

fujitatomoya commented Apr 4, 2023

Barry-Xu-2018 commented Apr 5, 2023

fujitatomoya commented Apr 5, 2023

Barry-Xu-2018 commented Apr 6, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Apr 6, 2023

Barry-Xu-2018 commented Apr 7, 2023

wjwwood left a comment

Choose a reason for hiding this comment

fujitatomoya commented Apr 17, 2023

clalancette commented Apr 17, 2023

clalancette commented Apr 17, 2023

clalancette commented Apr 18, 2023

Barry-Xu-2018 commented Apr 18, 2023

fujitatomoya commented Apr 18, 2023

Barry-Xu-2018 commented Apr 18, 2023

fujitatomoya commented Apr 18, 2023

Barry-Xu-2018 commented Mar 21, 2023 •

edited

Loading

Barry-Xu-2018 commented Apr 4, 2023 •

edited

Loading