Skip to content

Implement logger service#3

Merged
Barry-Xu-2018 merged 4 commits intotopic-logging_servicefrom
review/topic-logging_service
Apr 4, 2023
Merged

Implement logger service#3
Barry-Xu-2018 merged 4 commits intotopic-logging_servicefrom
review/topic-logging_service

Conversation

@Barry-Xu-2018
Copy link
Copy Markdown
Owner

No description provided.

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Copy Markdown
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
Copy Markdown
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

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

@iuhilnehc-ynos Please check again

Copy link
Copy Markdown
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

private:
RCLCPP_DISABLE_COPY(NodeBuiltinExecutor)
class NodeBuiltinExecutorImpl;
std::shared_ptr<NodeBuiltinExecutorImpl> impl_;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

{
int ret = 0;
auto result = rcl_interfaces::msg::SetLoggerLevelsResult();
for (auto & l : request->levels) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr node_time_source_;
rclcpp::node_interfaces::NodeWaitablesInterface::SharedPtr node_waitables_;

rclcpp::NodeBuiltinExecutor::SharedPtr node_builtin_executor_;
Copy link
Copy Markdown
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
Copy Markdown
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