#ros2/rcl_interfaces#154

Rebased to the latest rolling.

This PR depend on #2122

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.

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

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

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

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.

@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

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

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.

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.

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

CI:

• Linux
• Linux-aarch64
• Windows

Rebase was done.

CI:

• Linux
• Linux-aarch64
• Windows

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

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

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

@clalancette Okay. I will check failed test cases.

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

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

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