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.

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

Add logger service description #3477

Merged

Conversation

Barry-Xu-2018
Copy link
Contributor

Depend on
ros2/demos#611

@fujitatomoya
Copy link
Collaborator

overall looks good to me, note that this doc should be merged in rolling and iron only.

@clalancette clalancette self-assigned this May 4, 2023
@fujitatomoya
Copy link
Collaborator

@clalancette now ros2/demos#611 is merged, this is good to go now?

@Barry-Xu-2018
Copy link
Contributor Author

@clalancette friendly ping.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left a bunch of things to fix inline.

But my bigger concern here is that the way that this is currently structured, it doesn't exactly give the user something to run. It describes how to implement it, but not really how to run the demos. I think I'd rather see show how to run use_logger_service, and then make the service calls on it to get/set the level.

In the future there will be a generalized approach to external configuration of loggers at runtime (similar to how `rqt_logger_level <https://wiki.ros.org/rqt_logger_level>`__ in ROS 1 allows logger configuration via remote procedural calls).
**This concept is not yet officially supported in ROS 2.**
In the meantime, this demo provides an **example** service that can be called externally to request configuration of logger levels for known names of loggers in the process.
You can use logger service to implement external configuration of loggers at runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can use logger service to implement external configuration of loggers at runtime.
You can use the node logger service to externally configure loggers at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


The demo previously started is already running this example service.
To set the level of the demo's logger back to ``INFO``\ , call the service with:
The following code shows how to enable logger service while creating node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following code shows how to enable logger service while creating node.
The following code shows how to enable the logger service while creating the node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

# Create a node with logger service enabled
node = Node('NodeWithLoggerService', enable_logger_service=True)

You will find 2 services under node `NodeWithLoggerService` by running `ros2 service list`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You will find 2 services under node `NodeWithLoggerService` by running `ros2 service list`
If you run one of the nodes as configured above, you will find 2 services when running ``ros2 service list``:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


* get_logger_levels

Use this service to get logger levels by specified logger names.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use this service to get logger levels by specified logger names.
Use this service to get logger levels for specified logger names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 269 to 271
Run `ros2 srvice call` to get logger levels for `NodeWithLoggerService` and `rcl`.
.. code-block:: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Run `ros2 srvice call` to get logger levels for `NodeWithLoggerService` and `rcl`.
.. code-block:: bash
Run ``ros2 service call`` to get logger levels for ``NodeWithLoggerService`` and `rcl``.
.. code-block:: bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 283 to 286
Run `ros2 service call` to set logger levels for `NodeWithLoggerService` and `rcl`.
.. code-block:: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Run `ros2 service call` to set logger levels for `NodeWithLoggerService` and `rcl`.
.. code-block:: bash
Run ``ros2 service call`` to set logger levels for ``NodeWithLoggerService`` and ``rcl``.
.. code-block:: bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

rcl_interfaces.srv.SetLoggerLevels_Response(results=[rcl_interfaces.msg.SetLoggerLevelsResult(successful=True, reason=''), rcl_interfaces.msg.SetLoggerLevelsResult(successful=True, reason='')])


There are demo code on how to set/get logger level via logger service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
There are demo code on how to set/get logger level via logger service.
There is also demo code showing how to set or get the logger level via the logger service.


You should see debug output from ``rcl`` start to show.
Currently, there is a limitation that `get_logger_levels` and `set_logger_levels` services are not thread-safe.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Currently, there is a limitation that `get_logger_levels` and `set_logger_levels` services are not thread-safe.
Currently, there is a limitation that ``get_logger_levels`` and ``set_logger_levels`` services are not thread-safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor Author

@Barry-Xu-2018 Barry-Xu-2018 left a comment

Choose a reason for hiding this comment

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

Thanks for your comments.

But my bigger concern here is that the way that this is currently structured, it doesn't exactly give the user something to run. It describes how to implement it, but not really how to run the demos. I think I'd rather see show how to run use_logger_service, and then make the service calls on it to get/set the level.

In the document, it described how to enable the logger service and how to get/set logger by CLI. If users want to use code to change logger, demo are reference. Demo is easy to run (No special operation). Users can look at the code of demo to understand how to use code to get/set logger via logger service. Anyway, I will add how to run demo in document.

In the future there will be a generalized approach to external configuration of loggers at runtime (similar to how `rqt_logger_level <https://wiki.ros.org/rqt_logger_level>`__ in ROS 1 allows logger configuration via remote procedural calls).
**This concept is not yet officially supported in ROS 2.**
In the meantime, this demo provides an **example** service that can be called externally to request configuration of logger levels for known names of loggers in the process.
You can use logger service to implement external configuration of loggers at runtime.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


The demo previously started is already running this example service.
To set the level of the demo's logger back to ``INFO``\ , call the service with:
The following code shows how to enable logger service while creating node.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

# Create a node with logger service enabled
node = Node('NodeWithLoggerService', enable_logger_service=True)

You will find 2 services under node `NodeWithLoggerService` by running `ros2 service list`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


* get_logger_levels

Use this service to get logger levels by specified logger names.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 269 to 271
Run `ros2 srvice call` to get logger levels for `NodeWithLoggerService` and `rcl`.
.. code-block:: bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


* set_logger_levels

Use this service to set logger levels by specified logger names.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 283 to 286
Run `ros2 service call` to set logger levels for `NodeWithLoggerService` and `rcl`.
.. code-block:: bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


You should see debug output from ``rcl`` start to show.
Currently, there is a limitation that `get_logger_levels` and `set_logger_levels` services are not thread-safe.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@Barry-Xu-2018
Copy link
Contributor Author

@clalancette Please review again a6e18b9

@fujitatomoya
Copy link
Collaborator

@clalancette friendly ping on this.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A couple more things to fix, then I think this will be ready to merge.

@clalancette clalancette added the backport-iron backport at reviewers discretion; from rolling to iron label Jul 12, 2023
@Barry-Xu-2018
Copy link
Contributor Author

@clalancette Updated base on your comments at cdb07a4

Barry-Xu-2018 and others added 5 commits July 13, 2023 08:32
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette force-pushed the topic-add-logger-service-description branch from cdb07a4 to 0b14190 Compare July 13, 2023 12:35
@clalancette clalancette merged commit c60d0e1 into ros2:rolling Jul 13, 2023
3 checks passed
mergify bot pushed a commit that referenced this pull request Jul 13, 2023
* Add logger service description

Signed-off-by: Barry Xu <barry.xu@sony.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit c60d0e1)
clalancette pushed a commit that referenced this pull request Jul 13, 2023
* Add logger service description

Signed-off-by: Barry Xu <barry.xu@sony.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit c60d0e1)

Co-authored-by: Barry Xu <barry.xu@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-iron backport at reviewers discretion; from rolling to iron
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants