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

Implement logger service #3

Merged
merged 4 commits into from
Apr 4, 2023

Conversation

Barry-Xu-2018
Copy link
Owner

No description provided.

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Owner Author

Barry-Xu-2018 commented Apr 2, 2023

@iuhilnehc-ynos
Please help to review it.
This PR is based on ros2#2122 and use new class NodeBuiltinExecutor for internal executor. After review, I will remake PR for ros2#2122.
The get/set logger lock isn't considered. There are many ways to get/set logger. I think It is better to set lock in rcutils.

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.

the first review is finished

rclcpp/CMakeLists.txt Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/node_interfaces/node_base.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/node_interfaces/node_logging.hpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/node.cpp Outdated Show resolved Hide resolved
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Owner Author

@iuhilnehc-ynos Please check again

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.

2nd review

rclcpp/include/rclcpp/node.hpp Outdated Show resolved Hide resolved
private:
RCLCPP_DISABLE_COPY(NodeBuiltinExecutor)
class NodeBuiltinExecutorImpl;
std::shared_ptr<NodeBuiltinExecutorImpl> impl_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use std::unique_ptr instead of std::shared_ptr.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems you haven't updated it.

rclcpp/include/rclcpp/node_builtin_executor.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/node_builtin_executor.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/node_builtin_executor.hpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/node_builtin_executor.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/node_builtin_executor.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/node_builtin_executor.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/node_builtin_executor.cpp Outdated Show resolved Hide resolved
{
int ret = 0;
auto result = rcl_interfaces::msg::SetLoggerLevelsResult();
for (auto & l : request->levels) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (auto & l : request->levels) {
for (auto & level : request->levels) {

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, the meaning of levels in ros2/rcl_interfaces#154 is vague.
Because it seems like a level array, the level might be treated as the uint8 level in the LoggerLevel.

@iuhilnehc-ynos
Copy link
Collaborator

I don't know which way node_builtin_executor_ should be processed while creating a sub-node in the https://github.com/ros2/rclcpp/blob/432bf21f0261bab209b37ccfe6da550f02751a22/rclcpp/src/rclcpp/node.cpp#L230-L245.
Let's put it to the community.

Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 merged commit 95982cb into topic-logging_service Apr 4, 2023
node_base_,
node_topics_,
node_services_,
node_logging_,
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos Apr 4, 2023

Choose a reason for hiding this comment

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

Please remove node_logging_.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. I have removed it before pushing to github rclcpp

@@ -1087,6 +1088,8 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface,
rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr node_time_source_;
rclcpp::node_interfaces::NodeWaitablesInterface::SharedPtr node_waitables_;

rclcpp::NodeBuiltinExecutor::SharedPtr node_builtin_executor_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be updated as well.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. Updated before pushing to github rclcpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants